Bug 78052 – [V8] Allow bindings for attributes on DOM nodes to also set a named hidden reference

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore JavaScript

[Erik Arvidsson]

[V8] Allow bindings for attributes on DOM nodes to also set a named hidden reference

[Erik Arvidsson]

Created attachment 125947 [details]
Patch

[Nate Chapin]

Comment on attachment 125947 [details]
Patch

Ur doin it rite.

[WebKit Review Bot]

Comment on attachment 125947 [details]
Patch

Clearing flags on attachment: 125947

Committed r107035: <http://trac.webkit.org/changeset/107035>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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

[Erik Arvidsson]

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

[Ryosuke Niwa]

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

[Erik Arvidsson]

(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());
}

[Erik Arvidsson]

This was rolled back due to the perf impact.

[Erik Arvidsson]

Created attachment 127495 [details]
Patch

[Erik Arvidsson]

Created attachment 127497 [details]
Patch

[WebKit Review Bot]

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

[Erik Arvidsson]

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.

[anton muhin]

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.

[Sam Weinig]

V8 is no longer supported in WebKit.