Bug 92079

Created attachment 153990 [details] Patch

(In reply to comment #1) > Created an attachment (id=153990) [details] > Patch This patch is quite big, but mostly mechanical changes.

I will run the html-parser benchmark for this patch too after Bug 92065 is resolved.

What's the benefit of this patch? In these cases, there's no way for the page to influence the data we're reading (e.g., the tagName). It's only in the cases where we read back the attributes that there are problems because the attributes are mutable.

(In reply to comment #4) > What's the benefit of this patch? In these cases, there's no way for the page to influence the data we're reading (e.g., the tagName). It's only in the cases where we read back the attributes that there are problems because the attributes are mutable. Sorry I should have mentioned this in the change log. This patch is a prerequisite for Bug 90751. This patch changes the parser to avoid reading the DOM tree (elements) because we can't create elements in speculation mode. But this patch by itself has no benefit. To be precise, there is one place where it actually changes the behavior. I removed the FIXME comment in HTMLElementStack::isHTMLIntegrationPoint(HTMLStackItem*). 292 // FIXME: Technically we shouldn't read back from the DOM here. 293 // Instead, we're supposed to track this information in the element 294 // stack, which lets the parser run on its own thread. Maybe I should file a separate bug for this though it is a bit tricky because HTMLElementStack::isHTMLIntegrationPoint is used in many places.

Created attachment 154164 [details] Patch

(In reply to comment #6) > Created an attachment (id=154164) [details] > Patch Rebased to the latest revision.

Let's hold off reviewing this patch until I can show the real benefit of speculative parsing. I will file a separate bug for HTMLElementStack::isHTMLIntegrationPoint if it's possible.

Comment on attachment 154164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154164&action=review > Source/WebCore/html/parser/HTMLElementStack.cpp:292 > + String encoding = encodingAttr->value(); String -> const String& This saves a ref()/deref() pair. > Source/WebCore/html/parser/HTMLStackItem.h:86 > + QualifiedName m_name; I thought we decided that having a QualifiedName constructor here caused perf problems... > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:601 > namespace { By the way, we're not supposed to use anonymous namespaces. Instead, we should mark these functions as static. If you're interested in a doing some cleanup in a follow up patch, that's one idea.

I think it's ok to move forward with this patch. Would you be willing to run the HTML parsing benchmark? I thought we avoided adding a QualifiedName to HTMLStackItem in a previous patch because of performance.

Comment on attachment 154164 [details] Patch r=me pending performance measurement.

(In reply to comment #10) > I think it's ok to move forward with this patch. Would you be willing to run the HTML parsing benchmark? I thought we avoided adding a QualifiedName to HTMLStackItem in a previous patch because of performance. Sure. I wrote the patch before I've got to know there is a performance penalty in adding a QualifiedName to HTMLStackItem. I will try to find a workaround for this.

(In reply to comment #9) > By the way, we're not supposed to use anonymous namespaces. Instead, we should mark these functions as static. If you're interested in a doing some cleanup in a follow up patch, that's one idea. Okay. I will create a separate bug for this later.

(In reply to comment #11) > (From update of attachment 154164 [details]) > r=me pending performance measurement. There is no performance regression even though this patch added a QualifedName to HTMLStackItem. I will check if I can optimize this patch further. * Before kseo@cat:WebKit (master)> ./Tools/Scripts/run-perf-tests Parser/html-parser.html Running 1 tests Running Parser/html-parser.html (1 of 1) RESULT Parser: html-parser= 2771.3 ms median= 2770.5 ms, stdev= 4.7759815745 ms, min= 2765.0 ms, max= 2784.0 ms * After kseo@cat:WebKit (master)> ./Tools/Scripts/run-perf-tests Parser/html-parser.html Running 1 tests Running Parser/html-parser.html (1 of 1) RESULT Parser: html-parser= 2767.6 ms median= 2766.0 ms, stdev= 5.21919534028 ms, min= 2762.0 ms, max= 2781.0 ms

Created attachment 154243 [details] Patch

Comment on attachment 154243 [details] Patch Attachment 154243 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13346130 New failing tests: fast/loader/unload-form-post-about-blank.html

Created attachment 154250 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 154253 [details] Patch

(In reply to comment #18) > Created an attachment (id=154253) [details] > Patch I avoided adding a QualifiedName in HTMLStackItem. This patch increases the performance by 0.8-0.9%! kseo@cat:WebKit (master)> ./Tools/Scripts/run-perf-tests Parser/html-parser.html Running 1 tests Running Parser/html-parser.html (1 of 1) RESULT Parser: html-parser= 2746.8 ms median= 2746.5 ms, stdev= 3.5721142199 ms, min= 2742.0 ms, max= 2756.0 ms

Comment on attachment 154253 [details] Patch That's great.

Comment on attachment 154253 [details] Patch Clearing flags on attachment: 154253 Committed r123577: <http://trac.webkit.org/changeset/123577>

All reviewed patches have been landed. Closing bug.