Bug 125377 – REGRESSION: 2x regression on Dromaeo DOM query tests

• Component: Bindings
• Status: RESOLVED FIXED

[Ryosuke Niwa]

We should fix that.

[Ryosuke Niwa]

Created attachment 218645 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 218646 [details]
De-duped code

[Filip Pizlo]

Comment on attachment 218645 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=218645&action=review

Can you create microbenchmarks for the common case and the worst case?  I know that Dromaeo already has such a microbenchmark but it might be good to have something more surgical.  And I'd like to see the microbenchmark for the case where we have to clearImpurePropertySlots.  My gut tells me that you should instead be having impure property slot caches register a watchpoint that add/removeDocumentNamedItem fires.  That way the cost of repeated add/removeDocumentNamedItem won't have O(numWorlds) performance.

I think we need to determine if this approach is performant enough.

Also, do we already have test coverage for this?  I don't think we have test coverage for the remove case, and actually, I'm not sure that the remove case needs to clearImpurePropertySlots.

> Source/JavaScriptCore/runtime/JSObject.h:126
> +    // FIXME: We should be able to clear just a single slot instead.

File a bug for the FIXME, and reference the bug.

> Source/JavaScriptCore/runtime/JSObject.h:130
> +    void clearImpurePropertySlots(VM& vm)
> +    {
> +        setStructure(vm, Structure::impureNameGetterTransition(vm, structure()));
> +    }

This method should be out-of-line, in JSObject.cpp.

> Source/WebCore/html/HTMLDocument.cpp:336
>      m_documentNamedItem.add(name, item, *this);
> +
> +    Vector<Ref<DOMWrapperWorld>> worlds;
> +    ScriptController::getAllWorlds(worlds);
> +    for (size_t i = 0; i < worlds.size(); ++i)
> +        clearImpurePropertySlots(worlds[i].get(), this);

How often does this happen in practice?  How often can it happen in the worst case?  How much slower does this make us when that worst case happens?

> Source/WebCore/html/HTMLDocument.cpp:346
>      m_documentNamedItem.remove(name, item);
> +
> +    Vector<Ref<DOMWrapperWorld>> worlds;
> +    ScriptController::getAllWorlds(worlds);
> +    for (size_t i = 0; i < worlds.size(); ++i)
> +        clearImpurePropertySlots(worlds[i].get(), this);

Ditto.

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 218645 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218645&action=review
> 
> Can you create microbenchmarks for the common case and the worst case?

We already have one: PerformanceTests/Bindings/get-element-by-id.html

> And I'd like to see the microbenchmark for the case where we have to clearImpurePropertySlots.

I've created PerformanceTests/Bindings/update-name-getter.html

> My gut tells me that you should instead be having impure property slot caches register a watchpoint that add/removeDocumentNamedItem fires.  That way the cost of repeated add/removeDocumentNamedItem won't have O(numWorlds) performance.

That'll be great.  Can we talk about this in person sometime next week?  It turns out that my current patch improves get-element-by-id.html by 3x but slows down PerformanceTests/Bindings/update-name-getter.html by 3x so it's not that great as is.

> I think we need to determine if this approach is performant enough.

Unfortunately, it isn't as you've suspected :(

[Ryosuke Niwa]

Created attachment 218668 [details]
Added a microbenchmark

[Ryosuke Niwa]

Created attachment 218669 [details]
Perf. test results

[Ryosuke Niwa]

*** Bug 122628 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Created attachment 219042 [details]
New watchpoint approach work in progress

[Filip Pizlo]

Comment on attachment 219042 [details]
New watchpoint approach work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=219042&action=review

> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.cpp:58
> +    if (m_watchpointSetToUnregisterAtDestruction)
> +        m_codeBlock->vm()->unregisterWatchpointSetForImpureProperty(m_propertyNameForUnregisteringWatchpointSet, *m_watchpointSetToUnregisterAtDestruction);
>  }

This isn't needed.

> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:34
> +#include "Identifier.h"

This isn't needed.

> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:96
> +    void unregisterWatchpointSetAtDestruction(const Identifier& propertyName, InlineWatchpointSet& watchpointSet)
> +    {
> +        ASSERT(!m_watchpointSetToUnregisterAtDestruction);
> +        m_watchpointSetToUnregisterAtDestruction = &watchpointSet;
> +        // FIXME: We should store AtomicString once we've merged it with Identifier.
> +        m_propertyNameForUnregisteringWatchpointSet = propertyName.string();
> +    }
> +

Get rid of this.

> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:113
> +    InlineWatchpointSet* m_watchpointSetToUnregisterAtDestruction;
> +    String m_propertyNameForUnregisteringWatchpointSet;

Get rid of this.

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:41
> +#include <wtf/text/WTFString.h>

Get rid of this.

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:224
> +
> +    void unregisterWatchpointSetAtDestruction(const Identifier& propertyName, InlineWatchpointSet& watchpointSet)
> +    {
> +        watchpoints->unregisterWatchpointSetAtDestruction(propertyName, watchpointSet);
> +    }

You don't need this.

> Source/JavaScriptCore/jit/Repatch.cpp:260
> +        if (prototypeStructure->typeInfo().newImpurePropertyFiresWatchpoints()) {
> +            InlineWatchpointSet& set = currStructure->transitionWatchpointSet();
> +            stubInfo.unregisterWatchpointSetAtDestruction(propertyName, set);
> +            vm->registerWatchpointSetForImpureProperty(propertyName, set);
> +        }

Don't use the transitionWatchpointSet().  That set is for transitions.  You're not doing transitions.

Instead you should say:

if (prototypeStructure->typeInfo().newImpurePropertyFiresWatchpoints()) {
    Watchpoint* watchpoint = stubInfo.addWatchpoint(codeBlock);
    vm->registerWatchpointForImpureProperty(propertyName, watchpoint);
}

What you're doing is basically wrong, or at least highly circuitous.  It's wrong because:

- A watchpoint set should be associate with some conceptual event.  The structure's transition watchpoint set (currStructure->transitionWatchpointSet()) corresponds to a transition.  It's fired whenever there is a transition, and people register watchpoints on transitions with that watchpoint set.  You're reusing that set for your own purposes, which don't involve transitions.  That's just plain wrong.  You're making it so that if an impure property gets added, a bunch of listeners will think there had been a transition (even though no transition occurred).  You'll also fire the impure property thing whenever a transition occurs, even though a transition doesn't necessarily imply an impure property.  You're conflating unrelated things.

- As a result of your reuse of the transition watchpoint set, you create a mess for yourself trying to manage the lifecycle of the transition watchpoint set.  It's true that this is hard, but it's only hard because you're using an existing set instead of creating a new one.

- You shouldn't be using an InlineWatchpointSet at all.

Even if what you were doing wasn't outright wrong, it's clear that you're creating a *ton* of infrastructure for lifecycle management.  That infrastructure already exists.  It's called a watchpoint set.  The reason why you're unable to leverage that existing infrastructure is that you're reusing an existing watchpoint set which has its lifecycle tied to a GC'd object.  So, yeah, that's going to be hard.  But you don't need to use that watchpoint set.  Just create your own watchpoint set and you won't have to write half the code in this patch.

> Source/JavaScriptCore/runtime/VM.cpp:795
> +void VM::registerWatchpointSetForImpureProperty(const Identifier& propertyName, InlineWatchpointSet& watchpointSet)

The signature of this method should be:

void VM::registerWatchpointForImpureProperty(const Identifier& propertyName, Watchpoint* watchpoint)

> Source/JavaScriptCore/runtime/VM.cpp:798
> +    auto result = m_impurePropertyWatchpointSetSets.add(propertyName.string(), HashSet<InlineWatchpointSet*>());
> +    result.iterator->value.add(&watchpointSet);

This should check if the map has a WatchpointSet* for the property name.  It should create one if one didn't already exit.  Then it should add the watchpoint to that set.

Even if there was a way to make your approach work, it just feels awfully wrong: you have a map of sets of sets!  The whole point of a watchpoint set is to be a set of watchpoints.  You're creating a set of watchpoint sets, where those sets are borrowed from some unrelated Structure*, and then you're placing watchpoints into those sets. :-/  That's highly unusual.

> Source/JavaScriptCore/runtime/VM.cpp:809
> +void VM::unregisterWatchpointSetForImpureProperty(const String& propertyName, InlineWatchpointSet& watchpointSet)
> +{
> +    auto result = m_impurePropertyWatchpointSetSets.find(propertyName);
> +    ASSERT(result != m_impurePropertyWatchpointSetSets.end());
> +    if (result->value.size() == 1)
> +        m_impurePropertyWatchpointSetSets.remove(result);
> +    else
> +        result->value.remove(&watchpointSet);
> +}

Kill this method.

> Source/JavaScriptCore/runtime/VM.cpp:817
> +    for (auto it = result->value.begin(), end = result->value.end(); it != end; ++it)
> +        (*it)->fireAll();

You don't need this loop, since result->value should be a RefPtr<WatchpointSet>.  Then you call fireAll().  Then you should do m_impurePropertyWatchpointSets.remove(propertyName).  The reason for this is that WatchpointSets are one-shot.  So, after you fire it, it invalidates all watchpoints and then invalidates itself.  fireAll() is idempotent: fireAll() a second time will do nothing.

Also removing it saves memory.

> Source/JavaScriptCore/runtime/VM.h:523
> +        HashMap<String, HashSet<InlineWatchpointSet*>> m_impurePropertyWatchpointSetSets;

This should be:

HashMap<String, RefPtr<WatchpointSet>> m_impurePropertyWatchpointSets;

[Ryosuke Niwa]

Created attachment 219055 [details]
Work in progress 2

[Build Bot]

Comment on attachment 219055 [details]
Work in progress 2

Attachment 219055 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/47008368

New failing tests:
js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slot-traps.html

[Build Bot]

Created attachment 219062 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 219055 [details]
Work in progress 2

Attachment 219055 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/48138231

New failing tests:
js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slot-traps.html

[Build Bot]

Created attachment 219065 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 219055 [details]
Work in progress 2

Attachment 219055 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/48108242

New failing tests:
js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slot-traps.html

[Build Bot]

Created attachment 219067 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 219055 [details]
Work in progress 2

Attachment 219055 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/48088262

New failing tests:
js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slot-traps.html

[Build Bot]

Created attachment 219076 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Ryosuke Niwa]

Created attachment 219144 [details]
Patch

[Ryosuke Niwa]

Created attachment 219145 [details]
Reverted unrelated whitespace changes

[Darin Adler]

Comment on attachment 219145 [details]
Reverted unrelated whitespace changes

View in context: https://bugs.webkit.org/attachment.cgi?id=219145&action=review

> Source/JavaScriptCore/runtime/VM.cpp:785
> +    auto result = m_impurePropertyWatchpointSets.find(propertyName);
> +    if (result == m_impurePropertyWatchpointSets.end())
> +        return;
> +
> +    ASSERT(result->value);
> +    result->value->fireAll();
> +
> +    m_impurePropertyWatchpointSets.remove(result);

It is important to leave it in m_impurePropertyWatchpointSets while calling fireAll? If not, we could use take here instead of find/remove.

[Ryosuke Niwa]

Comment on attachment 219145 [details]
Reverted unrelated whitespace changes

View in context: https://bugs.webkit.org/attachment.cgi?id=219145&action=review

This patch is still wrong for this case:

document.foo = function () {};
function f() { return document.foo; }
for (var i = 0; i < 400; ++i) {
    if (i == 350) {
        var img = new Image();
        img.name = "foo";
        document.body.appendChild(img);
    }
    f(); // should be the image element for the last 50 runs.
}

>> Source/JavaScriptCore/runtime/VM.cpp:785
>> +    m_impurePropertyWatchpointSets.remove(result);
> 
> It is important to leave it in m_impurePropertyWatchpointSets while calling fireAll? If not, we could use take here instead of find/remove.

That's a good point. Will do.

[Ryosuke Niwa]

Created attachment 219148 [details]
Work in progress 3

[Ryosuke Niwa]

Created attachment 219286 [details]
Fixes the bug

[Filip Pizlo]

Comment on attachment 219286 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=219286&action=review

R=me on the code changes but you should make the tests do the right thing for concurrent JIT.  Otherwise the DFG tests will turn into flaky tests, instead of failing reliably, if someone breaks this again in the future.  We don't want flaky tests.

> LayoutTests/js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slot-traps-2.html:25
> +description("Tests what happens when you make prototype chain accesses with impure GetOwnPropertySlot traps in the way.");
> +
> +var obj = {};
> +obj.__proto__ = document;
> +
> +function f() {
> +    return obj.getElementsByTagName;
> +}
> +
> +var expected = "\"function\"";
> +for (var i = 0; i < 400; ++i) {
> +    if (i == 350) {
> +        var img = new Image();
> +        img.name = "getElementsByTagName";
> +        document.body.appendChild(img);
> +        expected = "\"object\"";
> +    }
> +    shouldBe("typeof f()", expected);
> +}

Can you rewrite these tests to use dfgIncrement?  That will make them relevant even in the case of concurrent JIT.

Or you can use dfgShouldBe().

> LayoutTests/js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slot-traps-4.html:26
> +var expected = "\"function\"";
> +for (var i = 0; i < 400; ++i) {
> +    if (i == 350) {
> +        var img = new Image();
> +        img.name = "foo";
> +        document.body.appendChild(img);
> +        expected = "\"object\"";
> +    }
> +    shouldBe("typeof f()", expected);
> +}

Ditto.

> LayoutTests/js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps-2.html:25
> +var expected = "\"function\"";
> +for (var i = 0; i < 40; ++i) {
> +    if (i == 35) {
> +        var img = new Image();
> +        img.name = "getElementsByTagName";
> +        document.body.appendChild(img);
> +        expected = "\"object\"";
> +    }
> +    shouldBe("typeof f()", expected);
> +}

Ditto.

[Filip Pizlo]

(In reply to comment #25)
> (From update of attachment 219286 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219286&action=review
> 
> R=me on the code changes but you should make the tests do the right thing for concurrent JIT.  Otherwise the DFG tests will turn into flaky tests, instead of failing reliably, if someone breaks this again in the future.  We don't want flaky tests.
> 
> > LayoutTests/js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slot-traps-2.html:25
> > +description("Tests what happens when you make prototype chain accesses with impure GetOwnPropertySlot traps in the way.");
> > +
> > +var obj = {};
> > +obj.__proto__ = document;
> > +
> > +function f() {
> > +    return obj.getElementsByTagName;
> > +}
> > +
> > +var expected = "\"function\"";
> > +for (var i = 0; i < 400; ++i) {
> > +    if (i == 350) {
> > +        var img = new Image();
> > +        img.name = "getElementsByTagName";
> > +        document.body.appendChild(img);
> > +        expected = "\"object\"";
> > +    }
> > +    shouldBe("typeof f()", expected);
> > +}
> 
> Can you rewrite these tests to use dfgIncrement?  That will make them relevant even in the case of concurrent JIT.
> 
> Or you can use dfgShouldBe().
> 
> > LayoutTests/js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slot-traps-4.html:26
> > +var expected = "\"function\"";
> > +for (var i = 0; i < 400; ++i) {
> > +    if (i == 350) {
> > +        var img = new Image();
> > +        img.name = "foo";
> > +        document.body.appendChild(img);
> > +        expected = "\"object\"";
> > +    }
> > +    shouldBe("typeof f()", expected);
> > +}
> 
> Ditto.
> 
> > LayoutTests/js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps-2.html:25
> > +var expected = "\"function\"";
> > +for (var i = 0; i < 40; ++i) {
> > +    if (i == 35) {
> > +        var img = new Image();
> > +        img.name = "getElementsByTagName";
> > +        document.body.appendChild(img);
> > +        expected = "\"object\"";
> > +    }
> > +    shouldBe("typeof f()", expected);
> > +}
> 
> Ditto.

Well I guess this last test isn't a DFG test so it doesn't need dfgShouldBe()/dfgIncrement().  But the DFG tests need some story for if the concurrent JIT decides to take longer to compile the code.

[Ryosuke Niwa]

Created attachment 219288 [details]
Fixed tests

[WebKit Commit Bot]

Comment on attachment 219288 [details]
Fixed tests

Clearing flags on attachment: 219288

Committed r160628: <http://trac.webkit.org/changeset/160628>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 219288 [details]
Fixed tests

View in context: https://bugs.webkit.org/attachment.cgi?id=219288&action=review

> Source/JavaScriptCore/ChangeLog:23
> +        (JSC::repatchByIdSelfAccess): Throw away the byte code when a new impure property is added on any
> +        object in the prototype chain via StructureStubClearingWatchpoint.

See below.

> Source/WebCore/ChangeLog:10
> +        The bug was caused by JSC not JIT'ing property accesses on document because of its having
> +        custom named getter (named properties).  This resulted in resolution of methods on document
> +        such as getElementById to happen inside the interpreter.

This isn't right. Previously, we did JIT those property accesses, we just failed to optimize them. They didn't run in the interpreter.

> LayoutTests/ChangeLog:8
> +        Added more regression tests for throwing away byte code when a new named property appears.

You don't really mean "throwing away byte code", do you? Are you actually evicting the bytecode from the bytecode cache -- or are you just throwing away a certain optimized code block?

[Peng Xinchao]

I don't make sure the patch's result. But the result of my testing In x86_64 GTK , the patch don't improve DROMAEO/DOM performance.

[Ryosuke Niwa]

(In reply to comment #31)
> I don't make sure the patch's result. But the result of my testing In x86_64 GTK , the patch don't improve DROMAEO/DOM performance.

We're definitely seeing 2x improvement on Dromaeo.  Perhaps you don't have JIT enabled?