[PATCH rebase 0/2] Avoid unncessary rebases

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH rebase 0/2] Avoid unncessary rebases

Jon TURNEY
Add some dignostics which report why a rebase is taking place.

Use that information to fix some errors causing unnecessary rebases

In those cases, we were often rebasing a DLL to the same address, so we could
also drop the rebase if the base address isn't actually changing, but we don't
seme to keep around the infomation to do that, currently.

Jon Turney (2):
  Make verbose give a reason why a rebase is needed
  Fix some errors which cause unnecessary rebases

 rebase.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 46 insertions(+), 13 deletions(-)

--
2.16.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] Make verbose give a reason why a rebase is needed

Jon TURNEY
---
 rebase.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/rebase.c b/rebase.c
index 6f98d37..0aad1b2 100644
--- a/rebase.c
+++ b/rebase.c
@@ -649,7 +649,16 @@ merge_image_info ()
  {
   /* Reuse the old address if possible. */
   if (match->slot_size < img_info_list[i].slot_size)
-    match->base = 0;
+    {
+      if (verbose)
+        fprintf (stderr, "rebasing %s because it won't fit in it's old slot size\n", img_info_list[i].name);
+      match->base = 0;
+    }
+  else
+    {
+      if (verbose)
+        fprintf (stderr, "rebasing %s because it's not located at it's old slot\n", img_info_list[i].name);
+    }
   match->flag.needs_rebasing = 1;
  }
       /* Unconditionally overwrite old with new size. */
@@ -668,8 +677,12 @@ merge_image_info ()
       img_info_list[i--] = img_info_list[--img_info_size];
     }
   else if (!img_info_list[i].flag.cannot_rebase)
-    /* Not in database yet.  Set base to 0 to choose a new one. */
-    img_info_list[i].base = 0;
+    {
+      /* Not in database yet.  Set base to 0 to choose a new one. */
+      img_info_list[i].base = 0;
+      if (verbose)
+ fprintf (stderr, "rebasing %s because not in database yet\n", img_info_list[i].name);
+    }
  }
     }
   if (!img_info_rebase_start || force_rebase_flag)
@@ -682,7 +695,11 @@ merge_image_info ()
   if (i < img_info_rebase_start)
     set_cannot_rebase (&img_info_list[i]);
   if (!img_info_list[i].flag.cannot_rebase)
-    img_info_list[i].base = 0;
+    {
+      img_info_list[i].base = 0;
+      if (verbose)
+ fprintf (stderr, "rebasing %s because forced or database missing\n", img_info_list[i].name);
+    }
  }
       img_info_rebase_start = 0;
     }
@@ -725,6 +742,8 @@ merge_image_info ()
      in the first place. */
   if (cur_base != img_info_list[i].base)
     {
+      if (verbose)
+ fprintf (stderr, "rebasing %s because it's base has changed (due to being reinstalled?)\n", img_info_list[i].name);
       img_info_list[i].flag.needs_rebasing = 1;
       /* Set cur_base to the old base to simplify subsequent tests. */
       cur_base = img_info_list[i].base;
@@ -733,17 +752,29 @@ merge_image_info ()
      anymore, rebase this DLL from scratch. */
   if (i + 1 < img_info_rebase_start
       && cur_base + slot_size + offset >= img_info_list[i + 1].base)
-    img_info_list[i].base = 0;
+    {
+      img_info_list[i].base = 0;
+      if (verbose)
+ fprintf (stderr, "rebasing %s because it won't fit in it's old slot without overlapping next DLL\n", img_info_list[i].name);
+    }
   /* Does the previous DLL reach into the address space of this
      DLL?  This happens if the previous DLL is not rebaseable. */
   else if (i > 0 && cur_base < img_info_list[i - 1].base
        + img_info_list[i + 1].slot_size)
-    img_info_list[i].base = 0;
+    {
+      img_info_list[i].base = 0;
+      if (verbose)
+ fprintf (stderr, "rebasing %s because previous DLL now overlaps\n", img_info_list[i].name);
+    }
   /* Does the file match the base address requirements?  If not,
      rebase from scratch. */
   else if ((down_flag && cur_base + slot_size + offset >= image_base)
    || (!down_flag && cur_base < image_base))
-    img_info_list[i].base = 0;
+    {
+      img_info_list[i].base = 0;
+      if (verbose)
+ fprintf (stderr, "rebasing %s because it's base address is outside the expected area\n", img_info_list[i].name);
+    }
  }
       /* Unconditionally overwrite old with new size. */
       img_info_list[i].size = cur_size;
@@ -900,6 +931,8 @@ collect_image_info (const char *pathname)
   img_info_list[img_info_size].slot_size
     = roundup2 (img_info_list[img_info_size].size, ALLOCATION_SLOT);
   img_info_list[img_info_size].flag.needs_rebasing = 1;
+  if (verbose)
+    fprintf (stderr, "rebasing %s because filename given on command line\n", img_info_list[img_info_size].name);
   img_info_list[img_info_size].flag.cannot_rebase = 0;
   /* This back and forth from POSIX to Win32 is a way to get a full path
      more thoroughly.  For instance, the difference between /bin and
--
2.16.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] Fix some errors which cause unnecessary rebases

Jon TURNEY
In reply to this post by Jon TURNEY
Also add some parentheses for clarity
---
 rebase.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/rebase.c b/rebase.c
index 0aad1b2..1c8fe7a 100644
--- a/rebase.c
+++ b/rebase.c
@@ -750,8 +750,8 @@ merge_image_info ()
     }
   /* However, if the DLL got bigger and doesn't fit into its slot
      anymore, rebase this DLL from scratch. */
-  if (i + 1 < img_info_rebase_start
-      && cur_base + slot_size + offset >= img_info_list[i + 1].base)
+  if ((i + 1 < img_info_rebase_start)
+      && (cur_base + slot_size + offset > img_info_list[i + 1].base))
     {
       img_info_list[i].base = 0;
       if (verbose)
@@ -759,8 +759,8 @@ merge_image_info ()
     }
   /* Does the previous DLL reach into the address space of this
      DLL?  This happens if the previous DLL is not rebaseable. */
-  else if (i > 0 && cur_base < img_info_list[i - 1].base
-       + img_info_list[i + 1].slot_size)
+  else if ((i > 0) && (cur_base < img_info_list[i - 1].base
+       + img_info_list[i - 1].slot_size))
     {
       img_info_list[i].base = 0;
       if (verbose)
@@ -768,8 +768,8 @@ merge_image_info ()
     }
   /* Does the file match the base address requirements?  If not,
      rebase from scratch. */
-  else if ((down_flag && cur_base + slot_size + offset >= image_base)
-   || (!down_flag && cur_base < image_base))
+  else if ((down_flag && (cur_base + slot_size + offset > image_base))
+   || (!down_flag && (cur_base < image_base)))
     {
       img_info_list[i].base = 0;
       if (verbose)
--
2.16.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Fix some errors which cause unnecessary rebases

Corinna Vinschen-2
On Feb  9 11:59, Jon Turney wrote:
> Also add some parentheses for clarity

I'm not a friend of excessive bracketing, especially when the evaluation
order is unambiguous.  This also covers what this patch is *really* meant
to fix, so I'd prefer a patch with only the fix.


Thanks,
Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Make verbose give a reason why a rebase is needed

Corinna Vinschen-2
In reply to this post by Jon TURNEY
On Feb  9 11:59, Jon Turney wrote:

> ---
>  rebase.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/rebase.c b/rebase.c
> index 6f98d37..0aad1b2 100644
> --- a/rebase.c
> +++ b/rebase.c
> @@ -649,7 +649,16 @@ merge_image_info ()
>   {
>    /* Reuse the old address if possible. */
>    if (match->slot_size < img_info_list[i].slot_size)
> -    match->base = 0;
> +    {
> +      if (verbose)
> +        fprintf (stderr, "rebasing %s because it won't fit in it's old slot size\n", img_info_list[i].name);
> +      match->base = 0;
> +    }
> +  else
> +    {
> +      if (verbose)
This would ideally be an

                  else if (verbose)
                    fprintf (<yadda>);

> -    img_info_list[i].base = 0;
> +    {
> +      /* Not in database yet.  Set base to 0 to choose a new one. */
> +      img_info_list[i].base = 0;
> +      if (verbose)
> + fprintf (stderr, "rebasing %s because not in database yet\n", img_info_list[i].name);

In the "reuse old address" case you have

  if (verbose)
    printf
  set var = 0

here and in later cases you have

  set var = 0;
  if (verbose)
    printf

I'd prefer to have these in the same order.

With this minor tweak patch is ok.


Thanks,
Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH rebase 0/2] Avoid unncessary rebases

Jon TURNEY
In reply to this post by Jon TURNEY
On 09/02/2018 11:59, Jon Turney wrote:
> Add some dignostics which report why a rebase is taking place.
>
> Use that information to fix some errors causing unnecessary rebases
>

After these fixes, 'rebase -s' is relatively quick, with a warm disk cache.

But (i) we still read and extract the ImageBase out of every DLL in the
database, in case it's changed, and (ii) rebaselst from autorebase seems
to pass in a list of every DLL, not just the ones which have been
added/removed by packaging changes, so I'm not entirely sure this is
working as intended, or as well as it could...


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH rebase 0/2] Avoid unncessary rebases

Achim Gratz
Jon Turney writes:
> But (i) we still read and extract the ImageBase out of every DLL in
> the database, in case it's changed,

I think that's how it should be.  When you run rebase, you must consider
that something changed behind your back and fix it up or the users will
go mad.

> and (ii) rebaselst from autorebase seems to pass in a list of every
> DLL, not just the ones which have been added/removed by packaging
> changes, so I'm not entirely sure this is working as intended, or as
> well as it could...

Huh?  No, it shouldn't do that, the whole point of incremental rebase is
to only feed in those files we know have changed.  But it's very much
intentional that the files we _assume_ to be unchanged get checked by
rebase if that's true.  If you want to suss it out, look at the files in
/var/cache/rebase (these are from the latest run and the one before that
with suffix .old) and in /var/log/setup.log.full (I've made rebase
pretty verbose so one should be able to see where a problem occured just
from the log).


Regards,
Achim.
--
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Waldorf MIDI Implementation & additional documentation:
http://Synth.Stromeko.net/Downloads.html#WaldorfDocs