Bug 101173 - Shadow DOM should be able to be disabled per context.
Summary: Shadow DOM should be able to be disabled per context.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 94082
  Show dependency treegraph
 
Reported: 2012-11-04 18:19 PST by Hajime Morrita
Modified: 2012-11-26 02:03 PST (History)
7 users (show)

See Also:


Attachments
Patch (11.50 KB, patch)
2012-11-04 18:29 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2012-11-04 18:19:24 PST
Basically this disables reverts http://trac.webkit.org/changeset/131549
because Chromium wants to temporally disable it.
Comment 1 Hajime Morrita 2012-11-04 18:29:01 PST
Created attachment 172254 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-04 19:10:11 PST
Comment on attachment 172254 [details]
Patch

Clearing flags on attachment: 172254

Committed r133429: <http://trac.webkit.org/changeset/133429>
Comment 3 WebKit Review Bot 2012-11-04 19:10:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Ojan Vafai 2012-11-07 22:16:31 PST
This patch looks like it caused a bunch of perf/memory regressions:

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?rev=166198&graph=CreateNodes&history=100

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/intl1/report.html?rev=166182&graph=V8.MemoryHeapSampleTotalUsed_0.75&trace=V8.MemoryHeapSampleTotalUsed_0.75&history=150

http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?rev=165925&graph=ws_single_peak_r&history=100

The only other patch in the regression range was rolled out in https://bugs.webkit.org/show_bug.cgi?id=101533 and the regression didn't go away. 

It's hard for me to see where the regression would come from. The only thing I can think of is maybe the implementation of V8EnabledPerContext versus V8EnabledAtRuntime? That's just a wild guess.

Is it possible at this point to do a rollback so we can be 100% sure it was this patch?
Comment 5 Shinya Kawanaka 2012-11-07 22:20:40 PST
Can we think that regression comes from chromium side instead of WebKit side?
This is also a wild guess, though.
Comment 6 Ojan Vafai 2012-11-07 22:28:42 PST
(In reply to comment #5)
> Can we think that regression comes from chromium side instead of WebKit side?
> This is also a wild guess, though.

I considered that, but on the non-webkit canary bots, the same regression happened when this webkit revision was pulled into chromium deps and not at the same chromium revisions as on the webkit canary bots. So, I don't see how it could be a chromium side change. It's certainly possible I got confused and misread the graphs though. Feel free to double-check my conclusions. :)
Comment 7 Shinya Kawanaka 2012-11-08 01:48:44 PST
Hmm... I would like to hear morrita's comment.
Comment 8 Dimitri Glazkov (Google) 2012-11-08 09:01:10 PST
Reverted r133429 for reason:

Speculative roll out, investigating perf regression.

Committed r133904: <http://trac.webkit.org/changeset/133904>
Comment 9 Ojan Vafai 2012-11-08 11:10:39 PST
Looks like this was the cause of the regression. All the graphs listed have recovered their losses.
Comment 10 Hajime Morrita 2012-11-26 02:03:30 PST
This was rolled out at r133904 and we have no plan to land this again. Closing.