Bug 78052

Created attachment 125947 [details] Patch

Comment on attachment 125947 [details] Patch Ur doin it rite.

Comment on attachment 125947 [details] Patch Clearing flags on attachment: 125947 Committed r107035: <http://trac.webkit.org/changeset/107035>

All reviewed patches have been landed. Closing bug.

This patch regressed DOMDivWalk: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?history=150&rev=-1&graph=DOMDivWalk http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Perf/builds/6278 The result is much easier to see on Perf-o-matic: http://webkit-perf.appspot.com/graph.html?#tests=[[6104,2001,3001]]&sel=1328173376951,1328778176951&displayrange=7&datatype=running

(In reply to comment #5) > This patch regressed DOMDivWalk: > http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?history=150&rev=-1&graph=DOMDivWalk > http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Perf/builds/6278 > > The result is much easier to see on Perf-o-matic: http://webkit-perf.appspot.com/graph.html?#tests=[[6104,2001,3001]]&sel=1328173376951,1328778176951&displayrange=7&datatype=running Ouch, that is bad. At this point in time I don't see how to fix that without reintroducing the bugs that this fixed. The problem is that when we do things like foo.bar in js we didn't use to keep a reference from the foo object to the bar object which lead to bugs where we lost the data stored on the bar object. foo.bar.baz = 42 gc() assert(foo.bar.baz === 42) Now we keep a reference if the webidl attribute is readonly and if the wrapper is a non node wrapper.

Can we at least see the the diff in the generated binding code and profile results?

(In reply to comment #7) > Can we at least see the the diff in the generated binding code and profile results? With this patch: static v8::Handle<v8::Value> childNodesAttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info) { INC_STATS("DOM.Node.childNodes._get"); Node* imp = V8Node::toNative(info.Holder()); RefPtr<NodeList> result = imp->childNodes(); v8::Handle<v8::Value> wrapper = result.get() ? V8NodeList::existingWrapper(result.get()) : v8::Handle<v8::Object>(); if (wrapper.IsEmpty()) { wrapper = toV8(result.get()); if (!wrapper.IsEmpty()) V8DOMWrapper::setNamedHiddenReference(info.Holder(), "childNodes", wrapper); } return wrapper; } without this patch: static v8::Handle<v8::Value> childNodesAttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info) { INC_STATS("DOM.Node.childNodes._get"); Node* imp = V8Node::toNative(info.Holder()); return toV8(imp->childNodes()); }

This was rolled back due to the perf impact.

Created attachment 127495 [details] Patch

Created attachment 127497 [details] Patch

Comment on attachment 127497 [details] Patch Attachment 127497 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11541403 New failing tests: fast/filesystem/workers/file-writer-write-overlapped.html fast/files/workers/worker-read-blob-sync.html fast/workers/worker-location.html storage/indexeddb/objectstore-basics-workers.html fast/filesystem/workers/file-writer-sync-write-overlapped.html storage/indexeddb/basics.html fast/filesystem/workers/simple-temporary-sync.html http/tests/workers/terminate-during-sync-operation.html

Anton, this patch has some bad perf regressions. I was wondering if you have any advice on this problem. The problem is that we do not keep a reference from a Node to its childNodes NodeList (for example, same thing applies to all non node wrappers). The current solution has been to add a custom toV8 that adds a V8 hidden property that keeps the wrapper alive. Since this was only done manually we missed a couple hundred cases ;-) However, adding the hidden property is quite slow and it regresses the DOMDivWalk.html perf test by something like 100%. The way that JSC gets away with this is by making the GC call JSNodeListOwner::isReachableFromOpaqueRoots which of course makes the GC slower. The nice thing about the JSC solution is that it does not keep alive wrappers that do not have custom properties so the NodeList wrapper will be collectable in almost all cases. What is the best approach here? Should we follow JSC and let the GC ask the wrapper if it is reachable? I assume V8 has no concept of custom properties? This path needs a lot of custom code which sucks too.

Erik, we have pretty similar tools with object groups and implicit references. Instead of using hidden references, I would use implicit refs. From benchmarks point of view, that should have negligible effect---so far prologue phase never surfaced as a major hot-spot. And that would protect you from the unpleasant case when C++ object identity changes for any reason---in this case you would still retain old nodelist with hidden properties. Overall, I think hidden references should go away completely---implicit references look like notably superior approach. But we have had only limited experience with implicit refs so far. (In reply to comment #13) > Anton, this patch has some bad perf regressions. I was wondering if you have any advice on this problem. > > The problem is that we do not keep a reference from a Node to its childNodes NodeList (for example, same thing applies to all non node wrappers). The current solution has been to add a custom toV8 that adds a V8 hidden property that keeps the wrapper alive. Since this was only done manually we missed a couple hundred cases ;-) However, adding the hidden property is quite slow and it regresses the DOMDivWalk.html perf test by something like 100%. > > The way that JSC gets away with this is by making the GC call JSNodeListOwner::isReachableFromOpaqueRoots which of course makes the GC slower. The nice thing about the JSC solution is that it does not keep alive wrappers that do not have custom properties so the NodeList wrapper will be collectable in almost all cases. > > What is the best approach here? Should we follow JSC and let the GC ask the wrapper if it is reachable? I assume V8 has no concept of custom properties? This path needs a lot of custom code which sucks too.

V8 is no longer supported in WebKit.