diff mbox

Fix cp_binding_level reuse logic

Message ID 1453941893-2946-1-git-send-email-patrick@parcs.ath.cx
State New
Headers show

Commit Message

Patrick Palka Jan. 28, 2016, 12:44 a.m. UTC 
In begin_scope(), we are accidentally clearing the entire
free_binding_level store whenever we reuse a single GC-alloced
cp_binding_level structure from it.  This happens because we erroneously
update free_binding_level _after_ the pointer pointing to the next
available structure has been cleared, rather than doing it the other way
around.  This patch reorders the two operations.

This bug was introduced in commit 43959b95a / r118903 back in 2006,
during a refactoring of XNEW/memset -> XCNEW etc.

Using a dummy test file of mine (which simply #includes almost all of
the C++ stdlib headers), according to -ftime-report and
/usr/bin/time -v, this patch reduces the amount of GGC memory
allocated from 150MB to 148MB and reduces the maximum RSS from 130MB to
128MB, in each case a reduction of 1.5% or so.  Of course, this is because we now
reuse more and allocate fewer cp_binding_level structures.

Bootstrapped + regtested on x86_64-pc-linux-gnu, OK to commit at this
stage?  Or for stage 1?

gcc/cp/ChangeLog:

	* name-lookup.c (begin_scope): After reusing a cp_binding_level
	structure, update free_binding_level before the structure's
	level_chain field gets cleared.
---
 gcc/cp/name-lookup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jason Merrill Jan. 28, 2016, 3:39 p.m. UTC | #1 
On 01/27/2016 07:44 PM, Patrick Palka wrote:
> In begin_scope(), we are accidentally clearing the entire
> free_binding_level store whenever we reuse a single GC-alloced
> cp_binding_level structure from it.  This happens because we erroneously
> update free_binding_level _after_ the pointer pointing to the next
> available structure has been cleared, rather than doing it the other way
> around.  This patch reorders the two operations.
>
> This bug was introduced in commit 43959b95a / r118903 back in 2006,
> during a refactoring of XNEW/memset -> XCNEW etc.
>
> Using a dummy test file of mine (which simply #includes almost all of
> the C++ stdlib headers), according to -ftime-report and
> /usr/bin/time -v, this patch reduces the amount of GGC memory
> allocated from 150MB to 148MB and reduces the maximum RSS from 130MB to
> 128MB, in each case a reduction of 1.5% or so.  Of course, this is because we now
> reuse more and allocate fewer cp_binding_level structures.
>
> Bootstrapped + regtested on x86_64-pc-linux-gnu, OK to commit at this
> stage?  Or for stage 1?

I suppose this qualifies as a regression fix.  OK.

Jason
diff mbox

Patch

diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
index c52d236..92d99aa 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -1557,8 +1557,8 @@  begin_scope (scope_kind kind, tree entity)
   if (!ENABLE_SCOPE_CHECKING && free_binding_level)
     {
       scope = free_binding_level;
-      memset (scope, 0, sizeof (cp_binding_level));
       free_binding_level = scope->level_chain;
+      memset (scope, 0, sizeof (cp_binding_level));
     }
   else
     scope = ggc_cleared_alloc<cp_binding_level> ();