Bug 65438 - JSC GC is far too conservative about growing the heap size, particularly on desktop platforms
: JSC GC is far too conservative about growing the heap size, particularly on d...
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 65605
:
  Show dependency treegraph
 
Reported: 2011-07-31 15:21 PST by
Modified: 2011-08-03 04:17 PST (History)


Attachments
the patch (2.05 KB, patch)
2011-07-31 15:25 PST, Filip Pizlo
no flags Review Patch | Details | Formatted Diff | Diff
the patch (1.14 KB, patch)
2011-08-01 16:04 PST, Filip Pizlo
no flags Review Patch | Details | Formatted Diff | Diff
the patch (12.36 KB, patch)
2011-08-01 17:00 PST, Filip Pizlo
no flags Review Patch | Details | Formatted Diff | Diff
the patch (12.24 KB, patch)
2011-08-02 12:00 PST, Filip Pizlo
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2011-07-31 15:21:54 PST
The JSC GC tried to always bring the heap size down to 512 KB, or 2x the amount of used memory (excluding external fragmentation).  But this leads to some needless GC work, particularly since (a) the GC has some large constant-time work to do like root scanning, and (b) restricting the heap to 512 KB browser-wide is probably unnecessary.  Increasing these limits - within reason - should improve performance while having a negligible effect on overall memory usage.
------- Comment #1 From 2011-07-31 15:25:18 PST -------
Created an attachment (id=102467) [details]
the patch

This is a 1.6% win on SunSpider and a 5.5% win on V8.
------- Comment #2 From 2011-07-31 15:42:46 PST -------
(From update of attachment 102467 [details])
I don't like the mac-specificness of this.  I also don't really like the magic numbers (2 * .., 3 * ...) and the compile time constant for criticalBytes could be problematic on systems with less physical ram
------- Comment #3 From 2011-07-31 15:52:49 PST -------
(In reply to comment #2)
> (From update of attachment 102467 [details] [details])
> I don't like the mac-specificness of this.  I also don't really like the magic numbers (2 * .., 3 * ...) and the compile time constant for criticalBytes could be problematic on systems with less physical ram

2 * heapsize was there before.  Would you recommend changing these to be named constants?

As for the 128MB: before, if we had a 128MB heap we would have set the watermark at 256MB.  Now we'll set it at 384MB.  What do you think of this approach: base the GC heap size growth factor on the FastMalloc reserved pages count?
------- Comment #4 From 2011-08-01 16:04:42 PST -------
Created an attachment (id=102583) [details]
the patch

This patch is a work-in-progress but it would be a good thing for others to run some tests on it.
------- Comment #5 From 2011-08-01 17:00:00 PST -------
Created an attachment (id=102593) [details]
the patch

This is a 4.1% speed-up in SunSpider and a 7.5% speed-up on V8.
------- Comment #6 From 2011-08-01 17:03:25 PST -------
Attachment 102593 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/heap/Heap.cpp:45:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/JavaScriptCore/heap/Heap.cpp:47:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #7 From 2011-08-01 17:04:24 PST -------
(From update of attachment 102593 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=102593&action=review

Would be good if steph could run tests on this, but basically looks good.

> Source/JavaScriptCore/ChangeLog:7
> +2011-07-31  Filip Pizlo  <fpizlo@apple.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +        
> +        * heap/Heap.cpp:
> +        (JSC::Heap::collect):
> +

Accidental change log entry?
------- Comment #8 From 2011-08-01 17:05:28 PST -------
(In reply to comment #7)
> (From update of attachment 102593 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102593&action=review
> 
> Would be good if steph could run tests on this, but basically looks good.
> 
> > Source/JavaScriptCore/ChangeLog:7
> > +2011-07-31  Filip Pizlo  <fpizlo@apple.com>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +        
> > +        * heap/Heap.cpp:
> > +        (JSC::Heap::collect):
> > +
> 
> Accidental change log entry?

Ooops!  Thanks for catching that.  I'll repost a new patch shortly with fixes to the style as well...
------- Comment #9 From 2011-08-01 17:18:55 PST -------
I think that the JSGlobalData are exposing too much state here (and already are), I think we should really just pass an enum value indicating whether this global data is for use by WebCore's main thread, a Worker, the API shared context, or an API allocated context.  The policies used for heap sizes & recursion limits should be encapsulated within JSC.

But I'm not particularly bothered about this right now since we already have ThreadStackType to clean up at some point - we can always revisit this.
------- Comment #10 From 2011-08-01 17:21:45 PST -------
Maciej had some comments on irc, pasting them here - I think his key suggestion is that LARGE_HEAP should be the default, and platforms should opt in to SMALL_HEAP.

[12:05am] othermaciej: olliej_: do we have a "low memory" ifdef of some kind?
[12:05am] othermaciej: would be much better to trigger small vs large heap off of that instead of MAC
[12:05am] othermaciej: it would also be useful to know the memory cost of this patch on something like membuster
[12:05am] gbarra: Given the performance constraints of our current GC, the idea that if a page rapidly generates 16Mb of objects then our heap usage may go up by 16Mb seems reasonable to me.
[12:05am] gbarra: othermaciej: *nod.  I can't think of one.
[12:06am] othermaciej: in fact LARGE_HEAP is presumably the normal case so it should be SMALL_HEAP that should be the specialized case, since it is dependent on a temporary limitation of some platforms
[12:07am] gbarra: othermaciej: seems reasonable.  I think Filips thought process was that there are more embedded CPUs than there are desktop CPUs, but I think you have a good point.
[12:08am] othermaciej: it's really not CPU-based, it is platform based
[12:08am] gbarra: *nod.
[12:08am] othermaciej: and most target platforms are not embedded
[12:08am] gbarra: true.
------- Comment #11 From 2011-08-02 12:00:47 PST -------
Created an attachment (id=102673) [details]
the patch
------- Comment #12 From 2011-08-02 13:41:26 PST -------
(From update of attachment 102673 [details])
Clearing flags on attachment: 102673

Committed r92224: <http://trac.webkit.org/changeset/92224>
------- Comment #13 From 2011-08-02 13:41:31 PST -------
All reviewed patches have been landed.  Closing bug.