Bug 97974 – [V8] ScriptWrappable should hold the wrapper handle directly (Dromaeo/dom-modify and dom-traverse get ~2.5% faster)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

[V8] ScriptWrappable should hold the wrapper handle directly (Dromaeo/dom-traverse gets 3% faster)

[Adam Barth]

Created attachment 166376 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 166376 [details]
Patch

u so purple

[Adam Barth]

Comment on attachment 166376 [details]
Patch

Yeah, I need to merge to HEAD.

[Adam Barth]

Created attachment 166378 [details]
Patch

[Adam Barth]

Comment on attachment 166378 [details]
Patch

Looks like I made everything crash.  :)

[Adam Barth]

Created attachment 166386 [details]
Work in progress---does not compile

[Adam Barth]

Created attachment 166528 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 166528 [details]
Patch

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. :)

[Adam Barth]

Created attachment 166566 [details]
Patch

[Adam Barth]

Now with less "oops"

[Kentaro Hara]

Comment on attachment 166566 [details]
Patch

OK. Great improvement.

[WebKit Review Bot]

Comment on attachment 166566 [details]
Patch

Clearing flags on attachment: 166566

Committed r130103: <http://trac.webkit.org/changeset/130103>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

This change is clearly visible on the perf charts:

http://webkit-perf.appspot.com/graph.html#tests=[[40020,2001,2389050],[40020,2001,3001],[40020,2001,173262]]&sel=1349029297326.7722,1349155069478.671,1573.6040609137056,2309.6446700507613&displayrange=7&datatype=running

[Kentaro Hara]

Awesome!

[Ojan Vafai]

This is hitting an assert on the chromium debug bots.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=plugins%2Freentrant-update-widget-positions.html%20svg%2Fas-image%2Fimg-preserveAspectRatio-support-1.html%20userscripts%2Fuser-script-top-frame-only.html%20svg%2FW3C-I18N%2Ftspan-dirLTR-ubOverride-in-rtl-context.svg%20http%2Ftests%2Fsecurity%2FisolatedWorld%2Fall-window-prototypes.html%20http%2Ftests%2Fsecurity%2FisolatedWorld%2Fevents.html%20http%2Ftests%2Fsecurity%2FisolatedWorld%2FuserGestureEvents.html%20http%2Ftests%2Fsecurity%2FjavascriptURL%2FjavascriptURL-execution-context-frame-src-htmldom.html%20http%2Ftests%2Fsecurity%2FisolatedWorld%2Fcontext-destroy.html%20http%2Ftests%2Fsecurity%2FisolatedWorld%2Fdocument-prototype.html%20http%2Ftests%2Fsecurity%2FisolatedWorld%2Fnumber-prototype.html%20http%2Ftests%2Fsecurity%2FisolatedWorld%2Fworld-reuse.html%20inspector%2Fextensions%2Fextensions-audits.html%20storage%2Fwebsql%2Ftransaction-error-callback-isolated-world.html%20

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]

[Adam Barth]

/me will fix.

[Adam Barth]

Fixed in http://trac.webkit.org/changeset/130125

[Ojan Vafai]

Committed r130189: <http://trac.webkit.org/changeset/130189>

[Ojan Vafai]

This caused regressions on the Dromaeo tests and was rolled out in http://trac.webkit.org/changeset/130189.


http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dom_perf/report.html?history=150&rev=-1&graph=Get%20Elements
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_domcoreattr/report.html?history=150&rev=-1&graph=dom_attr_element_expando___value
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_jslibtraversejquery/report.html?history=150&rev=-1&graph=jslib_traverse_jquery_jQuery___parents_x10
http://trac.webkit.org/log/?verbose=on&rev=130105&stop_rev=130077

http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_domcoremodify/report.html?history=150&rev=-1&graph=dom_modify
http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_domcoremodify/report.html?history=150&rev=-1&graph=dom_modify_createTextNode
http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_domcoremodify/report.html?history=150&rev=-1&graph=dom_modify_createElement
http://trac.webkit.org/log/?verbose=on&rev=130105&stop_rev=130100

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoremodify/report.html?history=150&rev=-1&graph=dom
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?history=150&rev=-1&graph=Template
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?history=150&rev=-1&graph=Get%20Elements
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?history=150&rev=-1&graph=CreateNodes
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?history=150&rev=-1&graph=CloneNodes
http://trac.webkit.org/log/?verbose=on&rev=130105&stop_rev=130091

[Ojan Vafai]

Ignore the dom_attr regression. That was the v8 roll.

[Adam Barth]

Created attachment 167375 [details]
work-in-progress

[Adam Barth]

Created attachment 169991 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

Created attachment 170000 [details]
Patch

[Eric Seidel (no email)]

Have we verified already that this is "no brainer after 98725"?

[Adam Barth]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Comment on attachment 170000 [details]
Patch

OK.  LGTM.  Same disclaimer as always for v8 patches:  You may still want to talk to kentaro. :)

[Adam Barth]

<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 :)

[Kentaro Hara]

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.

[Kentaro Hara]

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

[Adam Barth]

Comment on attachment 170000 [details]
Patch

Clearing flags on attachment: 170000

Committed r132245: <http://trac.webkit.org/changeset/132245>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.