| 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 Bugs | Assignee: | Adam Barth <abarth> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| 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: | |||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Adam Barth
2012-09-29 21:02:30 PDT
Created attachment 166376 [details]
Patch
Comment on attachment 166376 [details]
Patch
u so purple
Comment on attachment 166376 [details]
Patch
Yeah, I need to merge to HEAD.
Created attachment 166378 [details]
Patch
Comment on attachment 166378 [details]
Patch
Looks like I made everything crash. :)
Created attachment 166386 [details]
Work in progress---does not compile
Created attachment 166528 [details]
Patch
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. :) Created attachment 166566 [details]
Patch
Now with less "oops" Comment on attachment 166566 [details]
Patch
OK. Great improvement.
Comment on attachment 166566 [details] Patch Clearing flags on attachment: 166566 Committed r130103: <http://trac.webkit.org/changeset/130103> All reviewed patches have been landed. Closing bug. 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 Awesome! 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] /me will fix. Committed r130189: <http://trac.webkit.org/changeset/130189> 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 Ignore the dom_attr regression. That was the v8 roll. Created attachment 167375 [details]
work-in-progress
Created attachment 169991 [details]
Patch
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. Created attachment 170000 [details]
Patch
Have we verified already that this is "no brainer after 98725"? > 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. (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 on attachment 170000 [details]
Patch
OK. LGTM. Same disclaimer as always for v8 patches: You may still want to talk to kentaro. :)
<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 :) 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. (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 on attachment 170000 [details] Patch Clearing flags on attachment: 170000 Committed r132245: <http://trac.webkit.org/changeset/132245> All reviewed patches have been landed. Closing bug. |
