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.
Created attachment 102467 [details]
This is a 1.6% win on SunSpider and a 5.5% win on V8.
Comment on 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
(In reply to comment #2)
> (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
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?
Created attachment 102583 [details]
This patch is a work-in-progress but it would be a good thing for others to run some tests on it.
Created attachment 102593 [details]
This is a 4.1% speed-up in SunSpider and a 7.5% speed-up on V8.
Attachment 102593 [details] did not pass style-queue:
Total errors found: 2 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on 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.
> +2011-07-31 Filip Pizlo <firstname.lastname@example.org>
> + Reviewed by NOBODY (OOPS!).
> + * heap/Heap.cpp:
> + (JSC::Heap::collect):
Accidental change log entry?
(In reply to comment #7)
> (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.
> > +2011-07-31 Filip Pizlo <email@example.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...
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.
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.
Created attachment 102673 [details]
Comment on attachment 102673 [details]
Clearing flags on attachment: 102673
Committed r92224: <http://trac.webkit.org/changeset/92224>
All reviewed patches have been landed. Closing bug.