Bug 97974

Summary: [V8] ScriptWrappable should hold the wrapper handle directly (Dromaeo/dom-modify and dom-traverse get ~2.5% faster)
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Severity: Normal CC: caseq, dglazkov, eric, haraken, japhet, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 100033    
Bug Blocks:    
Description Flags
Work in progress---does not compile
perf test results (first five are with patch; last five are without)
Patch none

Description Adam Barth 2012-09-29 21:02:30 PDT
[V8] ScriptWrappable should hold the wrapper handle directly (Dromaeo/dom-traverse gets 3% faster)
Comment 1 Adam Barth 2012-09-29 21:04:15 PDT
Created attachment 166376 [details]
Comment 2 Dimitri Glazkov (Google) 2012-09-29 21:49:48 PDT
Comment on attachment 166376 [details]

u so purple
Comment 3 Adam Barth 2012-09-29 22:04:38 PDT
Comment on attachment 166376 [details]

Yeah, I need to merge to HEAD.
Comment 4 Adam Barth 2012-09-29 22:07:20 PDT
Created attachment 166378 [details]
Comment 5 Adam Barth 2012-09-29 23:13:07 PDT
Comment on attachment 166378 [details]

Looks like I made everything crash.  :)
Comment 6 Adam Barth 2012-09-30 08:54:31 PDT
Created attachment 166386 [details]
Work in progress---does not compile
Comment 7 Adam Barth 2012-10-01 12:59:38 PDT
Created attachment 166528 [details]
Comment 8 Eric Seidel (no email) 2012-10-01 15:57:14 PDT
Comment on attachment 166528 [details]

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

> Source/WebCore/bindings/v8/IntrusiveDOMWrapperMap.h:95
> +        // OOPS! Figure out what to do here.
> +        // MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::Binding);
> +        // info.addMember(m_table);

Other than this OOPS it looks fine. :)
Comment 9 Adam Barth 2012-10-01 16:14:36 PDT
Created attachment 166566 [details]
Comment 10 Adam Barth 2012-10-01 16:14:48 PDT
Now with less "oops"
Comment 11 Kentaro Hara 2012-10-01 16:49:49 PDT
Comment on attachment 166566 [details]

OK. Great improvement.
Comment 12 WebKit Review Bot 2012-10-01 17:15:53 PDT
Comment on attachment 166566 [details]

Clearing flags on attachment: 166566

Committed r130103: <http://trac.webkit.org/changeset/130103>
Comment 13 WebKit Review Bot 2012-10-01 17:15:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Kentaro Hara 2012-10-01 22:51:05 PDT
Comment 16 Ojan Vafai 2012-10-01 23:11:49 PDT
This is hitting an assert on the chromium debug bots.


STDERR: ASSERTION FAILED: wrapper == value
STDERR: third_party/WebKit/Source/WebCore/bindings/v8/IntrusiveDOMWrapperMap.h(80) : virtual bool WebCore::IntrusiveDOMWrapperMap::removeIfPresent(WebCore::Node*, v8::Persistent<v8::Object>)
STDERR: 	WebCore::IntrusiveDOMWrapperMap::removeIfPresent() [0x7f26c88926cc]
STDERR: 	WebCore::DOMDataStore::weakNodeCallback() [0x7f26c884f729]
STDERR: 	v8::internal::GlobalHandles::Node::PostGarbageCollectionProcessing() [0x7f26c582a8aa]
STDERR: 	v8::internal::GlobalHandles::PostGarbageCollectionProcessing() [0x7f26c5829384]
STDERR: 	v8::internal::Heap::PerformGarbageCollection() [0x7f26c583b27c]
STDERR: 	v8::internal::Heap::CollectGarbage() [0x7f26c583a596]
STDERR: 	v8::internal::Heap::CollectGarbage() [0x7f26c57a6f2b]
STDERR: 	v8::internal::AbortIncrementalMarkingAndCollectGarbage() [0x7f26c583a7d3]
STDERR: 	v8::internal::Heap::ReserveSpace() [0x7f26c583a8f3]
STDERR: 	v8::internal::Deserializer::DeserializePartial() [0x7f26c5a4fb2e]
STDERR: 	v8::internal::Snapshot::NewContextFromSnapshot() [0x7f26c5a57344]
STDERR: 	v8::internal::Genesis::Genesis() [0x7f26c578b726]
STDERR: 	v8::internal::Bootstrapper::CreateEnvironment() [0x7f26c5780896]
STDERR: 	v8::Context::New() [0x7f26c575429c]
STDERR: 	WebCore::V8DOMWindowShell::createContext() [0x7f26c889988a]
STDERR: 	WebCore::V8DOMWindowShell::initializeIfNeeded() [0x7f26c889916c]
STDERR: 	WebCore::ScriptController::evaluateInIsolatedWorld() [0x7f26c886d341]
STDERR: 	WebKit::WebFrameImpl::executeScriptInIsolatedWorld() [0x7f26c77f29d7]
STDERR: 	DRTTestRunner::evaluateScriptInIsolatedWorld() [0x4994e6]
STDERR: 	CppBoundClass::MemberCallback<>::run() [0x4a71e8]
STDERR: 	CppBoundClass::invoke() [0x4e646c]
STDERR: 	CppNPObject::invoke() [0x4e5b4b]
STDERR: 	WebCore::npObjectInvokeImpl() [0x7f26c88a5d2b]
STDERR: 	WebCore::npObjectMethodHandler() [0x7f26c88a5ed2]
STDERR: 	v8::internal::HandleApiCallHelper<>() [0x7f26c579bf26]
STDERR: 	v8::internal::Builtin_Impl_HandleApiCall() [0x7f26c5796b85]
STDERR: 	v8::internal::Builtin_HandleApiCall() [0x7f26c5796b56]
Comment 17 Adam Barth 2012-10-01 23:26:55 PDT
/me will fix.
Comment 18 Adam Barth 2012-10-01 23:31:55 PDT
Fixed in http://trac.webkit.org/changeset/130125
Comment 19 Ojan Vafai 2012-10-02 11:09:18 PDT
Committed r130189: <http://trac.webkit.org/changeset/130189>
Comment 20 Ojan Vafai 2012-10-02 11:10:04 PDT
This caused regressions on the Dromaeo tests and was rolled out in http://trac.webkit.org/changeset/130189.



Comment 21 Ojan Vafai 2012-10-02 11:51:24 PDT
Ignore the dom_attr regression. That was the v8 roll.
Comment 22 Adam Barth 2012-10-05 13:30:50 PDT
Created attachment 167375 [details]
Comment 23 Adam Barth 2012-10-22 14:45:04 PDT
Created attachment 169991 [details]
Comment 24 Adam Barth 2012-10-22 14:51:07 PDT
Created attachment 169992 [details]
perf test results (first five are with patch; last five are without)

Summary of perf results (average of five runs):

gc-forest: 3.60% worse
gc-mini-tree: 2.81% worse
gc-tree: 8.05% worse
dom-attr: 1.00% better
dom-modify: 2.48% better
dom-query: 1.40% worse
dom-traverse: 2.82% better

Note: These numbers include both the patch in this bug and the patch in bug 100033, which is also needed.

Essentially, we're gaining DOM performance at the cost of some GC performance.  This already seems like a good trade-off, but it will be a no-brainer trade-off after bug 98725 lands because most of the GC load will be shifted to the minor GC phase, which isn't slowed down at all.

Also, there is a memory savings because we don't need to maintain an extra pointer's worth of data for every wrapped node anymore.
Comment 25 Adam Barth 2012-10-22 15:33:12 PDT
Created attachment 170000 [details]
Comment 26 Eric Seidel (no email) 2012-10-22 15:49:35 PDT
Have we verified already that this is "no brainer after 98725"?
Comment 27 Adam Barth 2012-10-22 15:54:23 PDT
> Have we verified already that this is "no brainer after 98725"?

I haven't.  I haven't made a build with all the dependent patches for bug 98725, but the point of that bug is to shrink the time spent in major GC by a factor of 10, which makes this slowdown even less relevant.
Comment 28 Eric Seidel (no email) 2012-10-22 15:55:50 PDT
(In reply to comment #27)
> > Have we verified already that this is "no brainer after 98725"?
> I haven't.  I haven't made a build with all the dependent patches for bug 98725, but the point of that bug is to shrink the time spent in major GC by a factor of 10, which makes this slowdown even less relevant.

I see.  It would be good to make sure we know to check back after the dust settles to make sure we end up faster in the ways we care about, that's all.
Comment 29 Eric Seidel (no email) 2012-10-22 15:56:38 PDT
Comment on attachment 170000 [details]

OK.  LGTM.  Same disclaimer as always for v8 patches:  You may still want to talk to kentaro. :)
Comment 30 Adam Barth 2012-10-22 15:57:28 PDT
<abarth> eseidel: DOM operations occur massively more often than garbage collection.  speeding up DOM operations is a much greater benefit than the cost of slowing down major gc by a couple percent
<eseidel> abarth: I agree. a lso be good to put that on the bug :)
Comment 31 Kentaro Hara 2012-10-22 23:54:31 PDT
Great patch!

Would you wait to land the patch for a couple of days? I'm now looking into a crash bug (http://code.google.com/p/chromium/issues/detail?id=155942), and I'm suspecting that the patches around bug 98567 (which this patch depends on) might be the culprit.
Comment 32 Kentaro Hara 2012-10-23 07:33:54 PDT
(In reply to comment #31)
> Great patch!
> Would you wait to land the patch for a couple of days? I'm now looking into a crash bug (http://code.google.com/p/chromium/issues/detail?id=155942), and I'm suspecting that the patches around bug 98567 (which this patch depends on) might be the culprit.

hmm, it looks like bug 98567 is not the culprit. (As I commented in Chromium issue 155942, the bug does not happen in WebKit ToT.) Sorry for the noise.
Comment 33 Adam Barth 2012-10-23 11:14:22 PDT
Comment on attachment 170000 [details]

Clearing flags on attachment: 170000

Committed r132245: <http://trac.webkit.org/changeset/132245>
Comment 34 Adam Barth 2012-10-23 11:14:26 PDT
All reviewed patches have been landed.  Closing bug.