Bug 56128 - REGRESSION (r80052): ~25% regression on v8-splay in-browser
Summary: REGRESSION (r80052): ~25% regression on v8-splay in-browser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-03-10 11:52 PST by Geoffrey Garen
Modified: 2011-03-29 14:50 PDT (History)
1 user (show)

See Also:


Attachments
Patch (1.64 KB, patch)
2011-03-29 13:59 PDT, Geoffrey Garen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2011-03-10 11:52:32 PST
Stephanie measured a ~25% regression on v8-splay, caused by http://trac.webkit.org/changeset/80052.
Comment 1 Geoffrey Garen 2011-03-10 13:47:47 PST
This regression is measurable in the SunSpider harness but not the v8 harness.
Comment 2 Geoffrey Garen 2011-03-10 14:34:43 PST
The extra time is in a kernel call:
	0.1%	18.3%	mach_kernel	thread_setuserstack
Comment 3 Geoffrey Garen 2011-03-10 18:56:24 PST
thread_setuserstack seems to be a mis-symbolication by Shark. Really, this is the vm_map kernel trap.

The extra calls to vm_map occur because the block size is now 8KB instead of 256KB, so we're making more kernel calls to map in a large heap.
Comment 4 Geoffrey Garen 2011-03-10 18:57:43 PST
I meant to say the block size is now 16KB.
Comment 5 Geoffrey Garen 2011-03-10 20:11:55 PST
Since this regression only shows up in the SunSpider harness, I don't think it's worth fixing. Running the v8 tests through the SunSpider harness is a Frankensteinian configuration.

In this case, what you see is the cost of constructing an 8000 node data structure and then performing only 80 modifications of it. Here, initial heap growth dominates everything else in the benchmark.

In the real v8 benchmark, the orders of magnitude are reversed, and there are orders of magnitude more modification operations than initial setup operations.
Comment 6 Stephanie Lewis 2011-03-23 16:23:46 PDT
Reopening because the regression also occurred testing in browser
Comment 7 Stephanie Lewis 2011-03-23 16:25:10 PDT
<rdar://problem/9099476>
Comment 8 Geoffrey Garen 2011-03-29 13:59:47 PDT
Created attachment 87406 [details]
Patch
Comment 9 Darin Adler 2011-03-29 14:04:00 PDT
Comment on attachment 87406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87406&action=review

> Source/JavaScriptCore/runtime/Heap.cpp:386
> -    size_t proportionalBytes = static_cast<size_t>(1.5 * m_markedSpace.size());
> +    size_t proportionalBytes = 2 * m_markedSpace.size();

It’s unclear where the constant 2 comes from. Your comments in change log talk about 1X vs. .5X so there is obviously something going on here that I just can’t spot. A comment explaining what we are accomplishing by multiplying by 2 and why 2 is the right value could be quite helpful.
Comment 10 Geoffrey Garen 2011-03-29 14:50:56 PDT
Committed r82327: <http://trac.webkit.org/changeset/82327>