[PATCH rebase 0/2] Avoid unncessary rebases

[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

[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

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1500 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[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

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[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...

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