Bug 78052 - [V8] Allow bindings for attributes on DOM nodes to also set a named hidden reference
Summary: [V8] Allow bindings for attributes on DOM nodes to also set a named hidden re...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
Depends on: 78253
Blocks: 73865
  Show dependency treegraph
 
Reported: 2012-02-07 15:53 PST by Erik Arvidsson
Modified: 2013-04-05 17:24 PDT (History)
13 users (show)

See Also:


Attachments
Patch (12.66 KB, patch)
2012-02-07 15:55 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (20.17 KB, patch)
2012-02-16 18:59 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (20.17 KB, patch)
2012-02-16 19:06 PST, Erik Arvidsson
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2012-02-07 15:53:34 PST
[V8] Allow bindings for attributes on DOM nodes to also set a named hidden reference
Comment 1 Erik Arvidsson 2012-02-07 15:55:53 PST
Created attachment 125947 [details]
Patch
Comment 2 Nate Chapin 2012-02-07 16:04:31 PST
Comment on attachment 125947 [details]
Patch

Ur doin it rite.
Comment 3 WebKit Review Bot 2012-02-07 20:23:51 PST
Comment on attachment 125947 [details]
Patch

Clearing flags on attachment: 125947

Committed r107035: <http://trac.webkit.org/changeset/107035>
Comment 4 WebKit Review Bot 2012-02-07 20:23:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Erik Arvidsson 2012-02-09 10:11:46 PST
(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.
Comment 7 Ryosuke Niwa 2012-02-09 10:45:41 PST
Can we at least see the the diff in the generated binding code and profile results?
Comment 8 Erik Arvidsson 2012-02-09 12:02:13 PST
(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());
}
Comment 9 Erik Arvidsson 2012-02-14 13:52:57 PST
This was rolled back due to the perf impact.
Comment 10 Erik Arvidsson 2012-02-16 18:59:26 PST
Created attachment 127495 [details]
Patch
Comment 11 Erik Arvidsson 2012-02-16 19:06:10 PST
Created attachment 127497 [details]
Patch
Comment 12 WebKit Review Bot 2012-02-17 05:17:34 PST
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
Comment 13 Erik Arvidsson 2012-02-17 11:07:41 PST
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.
Comment 14 anton muhin 2012-02-20 06:02:50 PST
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.
Comment 15 Sam Weinig 2013-04-05 17:23:50 PDT
V8 is no longer supported in WebKit.