Bug 85914

Created attachment 140782 [details] Patch

Comment on attachment 140782 [details] Patch Attachment 140782 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12653396 New failing tests: http/tests/security/contentSecurityPolicy/xsl-blocked.php http/tests/security/xss-DENIED-xsl-external-entity.xml accessibility/aria-disabled.html http/tests/security/xss-DENIED-xsl-document-securityOrigin.xml fast/frames/lots-of-objects.html fast/parser/external-entities.xml compositing/sibling-positioning.html fast/loader/text-document-wrapping.html fast/events/zoom-dblclick.html http/tests/security/cookies/first-party-cookie-allow-xslt.xml fast/xsl/extra-lf-at-end.html fast/canvas/webgl/shader-precision-format.html http/tests/security/contentSecurityPolicy/xsl-unaffected-by-style-src-1.php fast/xsl/sort-unicode.xml fast/xsl/dtd-in-source-document.xml http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml http/tests/misc/location-with-space.php fast/xsl/nbsp-in-stylesheet.html http/tests/security/cookies/third-party-cookie-blocking-xslt.xml http/tests/security/cookies/assign-document-url.html fast/xsl/import-non-document-node.xhtml fast/performance/performance-now-timestamps.html

Created attachment 140801 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Odd. I'll test again locally.

It appears I'm missing a null check. Investigating.

Comment on attachment 140782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140782&action=review This seems to be missing a big aspect of the spec. I looked on the github branch and didn't see anything addressing this either. 1. You only need to add style rules / stylesheets that apply to the iframe element. This could have big performance implications. But, it's not a correctness issue, so a FIXME would be fine. 2. The root element of the iframe needs to inherit from the iframe. This is a correctness issue. Would be good to add testcases for this. "In a CSS-supporting user agent: the user agent must add all the style sheets that apply to the iframe element to the cascade of the active document of the iframe element's nested browsing context, at the appropriate cascade levels, before any style sheets specified by the document itself. In a CSS-supporting user agent: the user agent must, for the purpose of CSS property inheritance only, treat the root element of the active document of the iframe element's nested browsing context as being a child of the iframe element. (Thus inherited properties on the root element of the document in the iframe will inherit the computed values of those properties on the iframe element instead of taking their initial values.)" Other than that, the patch looks good to me. > Source/WebCore/css/StyleResolver.cpp:433 > + HTMLFrameOwnerElement* ownerElement = document()->ownerElement(); > + Vector<StyleSheetList*> ancestorSheets; > + while (ownerElement && ownerElement->hasTagName(iframeTag) && static_cast<HTMLIFrameElement*>(ownerElement)->shouldDisplaySeamlessly()) { > + ancestorSheets.append(ownerElement->document()->styleSheets()); > + ownerElement = ownerElement->document()->ownerElement(); I'd rather this code use seamlessParentIFrame to get the ownerElement. Seems like it would at least simplify the while clause.

Antti, mind taking a quick look at this to make sure it's sane?

(In reply to comment #6) > (From update of attachment 140782 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140782&action=review > > This seems to be missing a big aspect of the spec. I looked on the github branch and didn't see anything addressing this either. > > 1. You only need to add style rules / stylesheets that apply to the iframe element. This could have big performance implications. But, it's not a correctness issue, so a FIXME would be fine. Interesting. I'm not sure how that differs from what I did. My interpretation of that is that any sheet which "could" apply to the iframe, should be included. Even if that sheet doesn't affect the style of the iframe due to it's selector choices. Are you suggesting that I'm including sheets which may not match the media queries of the child or some such? > 2. The root element of the iframe needs to inherit from the iframe. This is a correctness issue. Would be good to add testcases for this. Yes. There are lots of tests for this, but that's the next change. I intentionally made that separate from this one, as it's slightly more controversial in implementation due to details of how we use a Style on the Document, instead of inheriting directly to the DocumentElement (like the spec would imply). I"ll clarify this all in my next patch. > "In a CSS-supporting user agent: the user agent must add all the style sheets that apply to the iframe element to the cascade of the active document of the iframe element's nested browsing context, at the appropriate cascade levels, before any style sheets specified by the document itself. Yes, that's the line this patch intends to implement. > In a CSS-supporting user agent: the user agent must, for the purpose of CSS property inheritance only, treat the root element of the active document of the iframe element's nested browsing context as being a child of the iframe element. (Thus inherited properties on the root element of the document in the iframe will inherit the computed values of those properties on the iframe element instead of taking their initial values.)" Yup. That's the very next patch! > Other than that, the patch looks good to me. > > > Source/WebCore/css/StyleResolver.cpp:433 > > + HTMLFrameOwnerElement* ownerElement = document()->ownerElement(); > > + Vector<StyleSheetList*> ancestorSheets; > > + while (ownerElement && ownerElement->hasTagName(iframeTag) && static_cast<HTMLIFrameElement*>(ownerElement)->shouldDisplaySeamlessly()) { > > + ancestorSheets.append(ownerElement->document()->styleSheets()); > > + ownerElement = ownerElement->document()->ownerElement(); > > I'd rather this code use seamlessParentIFrame to get the ownerElement. Seems like it would at least simplify the while clause. Ah yes! Sorry, that method was added later. I'll update the patch shortly. Thank you very much for the review!

Yes, I didn't consider the possibility that the Document had not yet been inserted into a Frame when it got a stylesheet update. I've added the appropriate null check to the next patch.

Created attachment 140809 [details] Patch for landing

(In reply to comment #8) > (In reply to comment #6) > > (From update of attachment 140782 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=140782&action=review > > > > This seems to be missing a big aspect of the spec. I looked on the github branch and didn't see anything addressing this either. > > > > 1. You only need to add style rules / stylesheets that apply to the iframe element. This could have big performance implications. But, it's not a correctness issue, so a FIXME would be fine. > > Interesting. I'm not sure how that differs from what I did. My interpretation of that is that any sheet which "could" apply to the iframe, should be included. Even if that sheet doesn't affect the style of the iframe due to it's selector choices. Are you suggesting that I'm including sheets which may not match the media queries of the child or some such? nm. I was confused. > > 2. The root element of the iframe needs to inherit from the iframe. This is a correctness issue. Would be good to add testcases for this. > > Yes. There are lots of tests for this, but that's the next change. I intentionally made that separate from this one, as it's slightly more controversial in implementation due to details of how we use a Style on the Document, instead of inheriting directly to the DocumentElement (like the spec would imply). I"ll clarify this all in my next patch. Sounds good. I didn't see it on github, but I was probably just looking for the wrong thing.

Comment on attachment 140809 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=140809&action=review > Source/WebCore/css/StyleResolver.cpp:434 > + Document* parentDocument = parentIFrame->document(); > + ancestorSheets.append(parentDocument->styleSheets()); > + childDocument = parentDocument; Nit: can't this just be: ancestorSheets.append(parentDocument->styleSheets()); childDocument = parentIFrame->document();

Comment on attachment 140809 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=140809&action=review >> Source/WebCore/css/StyleResolver.cpp:434 >> + childDocument = parentDocument; > > Nit: can't this just be: > ancestorSheets.append(parentDocument->styleSheets()); > childDocument = parentIFrame->document(); I assume you mean ancestorSheets.append(parentIframe->document()->styleSheets()); with the goal being to remove the parentDocument local?

Thanks again for the reviews! I hope to post the iframe-style-inheritance patch shortly, but we can go over that tomorrow. :)

Comment on attachment 140809 [details] Patch for landing Clearing flags on attachment: 140809 Committed r116471: <http://trac.webkit.org/changeset/116471>

All reviewed patches have been landed. Closing bug.

It appears that this patch caused 6-7% perf. regression on Dromaeo/dom-attr: http://webkit-perf.appspot.com/graph.html#tests=[[43012,2001,32196]]&sel=1336522543813.5264,1336567108024.0527,3340.308370044053,4400.8810572687225&displayrange=7&datatype=running

It's not particularly clear what the test is trying to do: http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/tests/dom-attr.html This patch should only have made StyleResolver recalc (an already expensive and presumably rare event) more expensive. But Ill investigate on monday. Thank you for the note.

(In reply to comment #18) > It's not particularly clear what the test is trying to do: > http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/tests/dom-attr.html > > This patch should only have made StyleResolver recalc (an already expensive and presumably rare event) more expensive. > > But Ill investigate on monday. Thank you for the note. That test appears to be *super* noisy on my machine. It seems to load the entire dromero test suite in an iframe and then sucks out the number it cares about. :( There is nothign for me to do here of use. :(

Yeah, I profiled DRT but nothing came out. It's probably caused by some memory layout / compiler optimization changes. I agree we can't do anything here.

(In reply to comment #19) > (In reply to comment #18) > > It's not particularly clear what the test is trying to do: > > http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/tests/dom-attr.html > > > > This patch should only have made StyleResolver recalc (an already expensive and presumably rare event) more expensive. > > > > But Ill investigate on monday. Thank you for the note. > > That test appears to be *super* noisy on my machine. > > It seems to load the entire dromero test suite in an iframe and then sucks out the number it cares about. :( > > There is nothign for me to do here of use. :( Seems like we should either try a speculative patch to verify the theory or rollback while you figure out what the issue is. It's a clear 6% regression and, assuming the perf dashboard is accurate, it's clearly from this patch. It sucks that the test is too noisy for local debugging, but I think that just means you need to stare at the code and try speculative changes and see how they come out on the bots. One easy way to verify your theory that the issue is in StyleResolver would be to ifdef out the suspicious lines from addStylesheetsFromSeamlessParents. As in, is it just the seamlessParentIFrame call? Seems really unlikely to me since ENABLE_IFRAME_SEAMLESS is false.

Alternatley, you could try tweaking the test locally to see if you can exaggerate the difference. For example, you could comment out all the "test" calls except 1 and try each test call separately to see which one regressed. You could also increase "num" to try and reduce the variance. I imagine that combining those two you ought to be able to get a reasonably reduced perf test case.

On my (mostly quieted) machine, I see the following output from dom-attr: 1780 1545.6 392.6500645063519 521.0957042957043 327.27105788423154 434.824975024975 avg 5001.441801711263 runs/s median 0 runs/s stdev 117.64701441386933 runs/s min 4856.96368102955 runs/s max 5131.251536886268 runs/s I'm assuming those first 6 numbers are not run results (as they are for other perf tests) as they do not corrolate with the min/max values, and if tehy were, would lead me to believe the test was amazingly noisy. Successive runs only got slower: 1686.4 1480.2 386.69210789210786 509.89390609390614 321.2131736526946 428.4850562611042 avg 4812.884243899814 runs/s median 0 runs/s stdev 98.9989274854694 runs/s min 4691.776064255106 runs/s max 4988.916731970625 runs/s 1614.4 1482.4 389.28866702160116 508.89490509490514 329.60252880851687 438.5374625374625 avg 4763.123563462485 runs/s median 0 runs/s stdev 100.25685680375 runs/s min 4650.014642044582 runs/s max 4925.60939060939 runs/s

The entire test is adding/removing attributes from dom objects: http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/tests/dom-attr.html#L5 As far as I can tell, dromero is running the tests each 5 times: http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/webrunner.js#L7 The progress reports, are the current "mean" value from Dromero: http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeorunner.js#L58 Which are not comparable with the end stats, since the end stats are the sum of the means: http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeorunner.js#L5 The only one of these subtests which could have anything to do with style, is the id changes. Which will trigger an updateId call and a setNeedsStyleRecalc() http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.cpp#L2011 http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.h#L582 http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.cpp#L706 However, the test never triggers a style recalc, so this should have no effect. The only effect from my change is that StyleResolver will take slightly longer to construct. However, the only StyleResolvers being constructed here are the ones when the initial documents (for this test) are created. There are no style updates happenign which would cause the StyleResolver to recalc. When profiling in Instruments, this test is largely dominated by jsString and AtomicString creation. This test (intentionally?) is thrashing the string tables as hard as it can. :) Again, nothing to do with style resolution or my change. I expect that what we're seeing here, is compiler noise, per rniwa. Our scores on this test are so astronomically high as to be useless. To suggest that the entire test runs in 1/5000th of a second? And that my change made it so it runs 5% slower than that is meaningless as far as I can tell. It appears it is litterally calling the testing function 5000 times in the second: http://trac.webkit.org/browser/trunk/PerformanceTests/Dromaeo/resources/dromaeo/web/webrunner.js#L85 Wow. :) So yes, we run this test incredibly fast. I'm not surprised that our speed here would vary by *huge* amounts depending on how the compiler is feeling that day. Similarly, I'm not surprised this test is super noisy. Unless you have further data, I believe we should ignore this "regression" and in general ignore any changes in dom-attr results in the future, unless they're incredibly large. :)

Created attachment 142080 [details] reduced performance test This test reliably shows a 5-6% regression from this patch. It logs the output to the console: r116448: 522 dom-attr.html:19 518 dom-attr.html:19 516 dom-attr.html:19 528 dom-attr.html:19 517 dom-attr.html:19 518 dom-attr.html:19 522 dom-attr.html:19 517 dom-attr.html:19 517 dom-attr.html:19 525 dom-attr.html:19 Avg:520 r116491: 553 dom-attr.html:19 548 dom-attr.html:19 549 dom-attr.html:19 546 dom-attr.html:19 547 dom-attr.html:19 555 dom-attr.html:19 547 dom-attr.html:19 544 dom-attr.html:19 550 dom-attr.html:19 547 dom-attr.html:19 Avg:548.6 Sometimes the first load is a little noisy, but reloading the page consistently shows the regression. Looks to be that setting IDs has gotten slower.

In the spirit of further investigation, I ran dom-attr.html in the debugger on tip of tree. I placed a break point at the callsites for the two functions I added: notifySeamlessChildDocumentsOfStylesheetUpdate and addStylesheetsFromSeamlessParents They were hit about 8 times total during page load (as expected). But then never during the actual running of the test. I don't have any explanation as to why this test result changes after my patch beyond that fact that the test appears so tight as to be testing noise, and that my change happens to be unlucky. :( I've also grepped the Dromero source to make sure that nowhere does it use the seamless attribute. I then ran the test again in the debugger, this time with breakpoints on the seamless code paths (where we actually add stylesheets from the parent docuemnts, etc.) to make sure there was no way the test was going down those paths (as my grep would suggest to be impossible). And correctly verified that the debugger never broke. I'm out of ideas. But as stated in comment #24, I believe this microbenchmark to be testing noise at this point, and thus this "regresssion" is meaningless. Comment #17 would suggest that Ryosuke agrees.

(In reply to comment #25) > Created an attachment (id=142080) [details] > reduced performance test > > This test reliably shows a 5-6% regression from this patch. It logs the output to the console: > > r116448: > 522 dom-attr.html:19 > 518 dom-attr.html:19 > 516 dom-attr.html:19 > 528 dom-attr.html:19 > 517 dom-attr.html:19 > 518 dom-attr.html:19 > 522 dom-attr.html:19 > 517 dom-attr.html:19 > 517 dom-attr.html:19 > 525 dom-attr.html:19 > Avg:520 > > r116491: > 553 dom-attr.html:19 > 548 dom-attr.html:19 > 549 dom-attr.html:19 > 546 dom-attr.html:19 > 547 dom-attr.html:19 > 555 dom-attr.html:19 > 547 dom-attr.html:19 > 544 dom-attr.html:19 > 550 dom-attr.html:19 > 547 dom-attr.html:19 > Avg:548.6 > > Sometimes the first load is a little noisy, but reloading the page consistently shows the regression. Looks to be that setting IDs has gotten slower. I'm curious how you generated these numbers? Using run-perf-tests I get a different output: run-perf-tests PerformanceTests/Dromaeo/dom-attr.html --release [/Projects/WebKit] Running Dromaeo/dom-attr.html (1 of 1) RESULT Dromaeo: dom-attr= 4829.11909288 runs/s median= 0.0 runs/s, stdev= 110.628063799 runs/s, min= 4669.4222185 runs/s, max= 4975.24575425 runs/s Was this from the console log on your machine?

(In reply to comment #27) > I'm curious how you generated these numbers? > > Using run-perf-tests I get a different output: > run-perf-tests PerformanceTests/Dromaeo/dom-attr.html --release [/Projects/WebKit] > Running Dromaeo/dom-attr.html (1 of 1) > RESULT Dromaeo: dom-attr= 4829.11909288 runs/s > median= 0.0 runs/s, stdev= 110.628063799 runs/s, min= 4669.4222185 runs/s, max= 4975.24575425 runs/s > > Was this from the console log on your machine? This was from the console log on my machine running the test case I attached.

Created attachment 142098 [details] results from running ojan's reduced test on my machine Am I reading this correctly? It looks like my numbers show the seamless change as a 10% speed-up on your benchmark? (assuming smaller numbers are better, which appears to be the case from your use of "time").

(In reply to comment #29) > Created an attachment (id=142098) [details] > results from running ojan's reduced test on my machine > > Am I reading this correctly? It looks like my numbers show the seamless change as a 10% speed-up on your benchmark? > (assuming smaller numbers are better, which appears to be the case from your use of "time"). Strange. Those are not the numbers I get running at the same revisions. I'm testing on chromium-linux FWIW. In either case, I'm increasingly convinced that the regression is not from this revision and that perf-o-matic was just wrong. I'm binary searching through revisions to find the culprit.

I don't think we should spend any more time investigating this issue. While it is possible that this patch caused a performance regression, the cause of the regression is beyond our control from what I can tell. It could be memory layout being different, some compiler optimization not working, or other odd source of perf. hits. Whatever it is, there isn't much we can do at this point. It's probably more productive to spend time on how to improve the score on that test than trying to figure out what the regression in this case.

From my instruments runs I suspect we could improve these numbers by looking closely at when we're creating atomic strings and seeing if we could avoid them. :) That said, this benchmark is doing the same thing 5 MILLLION times in .5 seconds, and thus even if we did improve it, is unlikely to have any effect on real page loads. I recommend we ignore/disable the benchmark in the future. :)

The perf regression was caused by http://trac.webkit.org/changeset/116487 and is being fixed in https://bugs.webkit.org/show_bug.cgi?id=86680. We looked, and perf-o-matic is getting data that matches the buildbot waterfall. So, either the bot is confused about what revision's build it's running, or we just got unlucky and the test high test variance just made it really seem like the seamless patch was the culprit.