|Summary:||JSC GC is far too conservative about growing the heap size, particularly on desktop platforms|
|Product:||WebKit||Reporter:||Filip Pizlo <fpizlo>|
|Severity:||Normal||CC:||barraclough, fpizlo, oliver, slewis, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:||65605|
Description Filip Pizlo 2011-07-31 15:21:54 PDT
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 Filip Pizlo 2011-07-31 15:25:18 PDT
Created attachment 102467 [details] the patch This is a 1.6% win on SunSpider and a 5.5% win on V8.
Comment 2 Oliver Hunt 2011-07-31 15:42:46 PDT
Comment on attachment 102467 [details] the patch 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 Filip Pizlo 2011-07-31 15:52:49 PDT
(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?
Comment 4 Filip Pizlo 2011-08-01 16:04:42 PDT
Created attachment 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 Filip Pizlo 2011-08-01 17:00:00 PDT
Created attachment 102593 [details] the patch This is a 4.1% speed-up in SunSpider and a 7.5% speed-up on V8.
Comment 6 WebKit Review Bot 2011-08-01 17:03:25 PDT
Comment 7 Oliver Hunt 2011-08-01 17:04:24 PDT
Comment 8 Filip Pizlo 2011-08-01 17:05:28 PDT
Comment 9 Gavin Barraclough 2011-08-01 17:18:55 PDT
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 Gavin Barraclough 2011-08-01 17:21:45 PDT
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 12 WebKit Review Bot 2011-08-02 13:41:26 PDT
Comment on attachment 102673 [details] the patch Clearing flags on attachment: 102673 Committed r92224: <http://trac.webkit.org/changeset/92224>
Comment 13 WebKit Review Bot 2011-08-02 13:41:31 PDT
All reviewed patches have been landed. Closing bug.