Bug 102852 – [V8] 7% regression in dromaeo_domcorequery/dom_query_getElementsByTagName__not_in_document

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ojan Vafai]

http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_domcorequery/report.html?rev=168911&graph=dom_query_getElementsByTagName__not_in_document_&history=50

Regression range looks to be: http://trac.webkit.org/log/?verbose=on&rev=135212&stop_rev=135193

Chromium side regression range is http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&mode=html&range=168612:168679, but it seems unlikely to me that this is a chromium-side regression.

http://trac.webkit.org/changeset/135208/ seems like the most likely culprit.

[Adam Barth]

Rolling out my patch now.

[Adam Barth]

The rollout has a conflict.  :*(

[Adam Barth]

I'm just going to try to fix this directly rather than rolling out.  There have been a bunch of dependent patches landed already.

[Adam Barth]

Created attachment 175310 [details]
Patch

[Kentaro Hara]

Comment on attachment 175310 [details]
Patch

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

Looks reasonable.

> Source/WebCore/bindings/v8/DOMDataStore.cpp:65
> +        V8DOMWindowShell* shell = V8DOMWindowShell::isolated(v8::Context::GetEntered());
> +        if (UNLIKELY(!!shell))
> +            return shell->world()->isolatedWorldDOMDataStore();

Maybe you can simplify the code by using worldForEnteredContextIfIsolated()?

[Adam Barth]

Comment on attachment 175310 [details]
Patch

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

>> Source/WebCore/bindings/v8/DOMDataStore.cpp:65
>> +            return shell->world()->isolatedWorldDOMDataStore();
> 
> Maybe you can simplify the code by using worldForEnteredContextIfIsolated()?

That has an extra v8::Context::InContext() check that we don't need.

[Kentaro Hara]

(In reply to comment #6)
> > Maybe you can simplify the code by using worldForEnteredContextIfIsolated()?
> 
> That has an extra v8::Context::InContext() check that we don't need.

Makes sense.

[WebKit Review Bot]

Comment on attachment 175310 [details]
Patch

Clearing flags on attachment: 175310

Committed r135339: <http://trac.webkit.org/changeset/135339>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

That patch didn't seem to have healed it.

[Adam Barth]

It actually made it slightly worse.  I suspect bug 102854 is the path to salvation here, but I need to look at the profile.

[Adam Barth]

As expected bug 102854 causes these functions to drop off the profile entirely.  I just need to fix the worker issue.

I'm realize we're not using the normal "revert and re-land approach"...  We should either finish bug 102854 soon to verify that it heals the bot or we should figure out what would be involved in backing all these changes out.

[Adam Barth]

Fascinating!  This bot is now healed:

http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_domcorequery/report.html?rev=169350&graph=dom_query_getElementsByTagName__not_in_document_&history=200

Here's what happened:

1) http://trac.webkit.org/changeset/135339 actually did fix the regression.  We didn't see the bot healed because http://trac.webkit.org/changeset/135338 introduced a new regression.

2) http://trac.webkit.org/changeset/135440 rolled out http://trac.webkit.org/changeset/135338, and we are now able to observe that the bot is indeed healed.