Last Comment Bug 700357 - Do GC_SHRINK when we have lots of decommitable arenas
: Do GC_SHRINK when we have lots of decommitable arenas
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla10
Assigned To: Terrence Cole [:terrence]
:
:
Mentors:
Depends on: 670596 704510 711623
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-07 10:12 PST by Terrence Cole [:terrence]
Modified: 2011-12-16 14:41 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1: gcshrink when we have many decommitable arenas (6.50 KB, patch)
2011-11-07 13:48 PST, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review
v2: With review feedback. (8.33 KB, patch)
2011-11-07 16:21 PST, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2011-11-07 10:12:13 PST
Now that we can really take advantage of a GC_SHRINK, we should run it more frequently.  We should keep a heuristic counter of decommitable arenas and run GC_SHRINK when this number gets "big".
Comment 1 Terrence Cole [:terrence] 2011-11-07 13:48:56 PST
Created attachment 572603 [details] [diff] [review]
v1: gcshrink when we have many decommitable arenas
Comment 2 Bill McCloskey (:billm) 2011-11-07 14:46:58 PST
Comment on attachment 572603 [details] [diff] [review]
v1: gcshrink when we have many decommitable arenas

Review of attachment 572603 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: js/src/jscntxt.h
@@ +422,5 @@
>      size_t              gcLastBytes;
>      size_t              gcMaxBytes;
>      size_t              gcMaxMallocBytes;
>      uint32              gcEmptyArenaPoolLifespan;
> +    uint32              gcNumFreeArenas;

Please make this volatile and put in a comment that we access it without holding the GC lock, but that it's okay because a race won't affect correctness.

::: js/src/jsgc.cpp
@@ +547,5 @@
>      Chunk *chunk = static_cast<Chunk *>(AllocChunk());
>      if (!chunk)
>          return NULL;
>      chunk->init();
> +    rt->gcNumFreeArenas += ArenasPerChunk;

Could you move this into Chunk::init (right after the loop that initializes each arena)?
Comment 3 Gregor Wagner [:gwagner] 2011-11-07 14:58:07 PST
Please make sure a GC_SHRINK GC doesn't happen during animations like FOTN or ro.me.
The main reason for the weak trigger behavior were regressions during animations.
Comment 4 Terrence Cole [:terrence] 2011-11-07 16:21:23 PST
Created attachment 572672 [details] [diff] [review]
v2: With review feedback.
Comment 5 Terrence Cole [:terrence] 2011-11-07 17:20:37 PST
https://hg.mozilla.org/mozilla-central/rev/410deae0fe13
Comment 6 Bill McCloskey (:billm) 2011-11-07 17:22:38 PST
(In reply to Gregor Wagner from comment #3)
> Please make sure a GC_SHRINK GC doesn't happen during animations like FOTN
> or ro.me.
> The main reason for the weak trigger behavior were regressions during
> animations.

FOTN does not do any shrinking GCs with or without the patch.
ro.me does three shrinking GCs without the patch and four with the patch. However, I didn't notice any significant difference in the time taken for shrinking versus non-shrinking GCs. Mostly I think this is because even the normal GCs are so expensive on this benchmark. So I don't think this is really a regression.
Comment 7 Daniel Cater 2011-11-08 06:16:46 PST
"There are plans in place to also run these when clicking the Minimize Memory button in about:memory, and when we are under memory pressure"

Which bugs will fix those 2 issues?
Comment 8 Justin Lebar (not reading bugmail) 2011-11-08 06:19:06 PST
> Which bugs will fix those 2 issues?

Bug 699279, which can be found as one of the "depends on" bugs of bug 670596.

Note You need to log in before you can comment on or make changes to this bug.