Bug 65438

Created attachment 102467 [details] the patch This is a 1.6% win on SunSpider and a 5.5% win on V8.

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

(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] the patch 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] the patch 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: 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 on attachment 102593 [details] the patch 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?

(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. > > > 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...

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] the patch

Comment on attachment 102673 [details] the patch Clearing flags on attachment: 102673 Committed r92224: <http://trac.webkit.org/changeset/92224>

All reviewed patches have been landed. Closing bug.