Bug 92079 – Read tag names and attributes from the saved tokens where possible.

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

[Kwang Yul Seo]

Read tag names and attributes from the saved tokens, not from the DOM. Also added convenient methods such as hasLocalName, hasTagName, localName, isElementNode and isDocumentFragmentNode to HTMLStackItem class.

[Kwang Yul Seo]

Created attachment 153990 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #1)
> Created an attachment (id=153990) [details]
> Patch

This patch is quite big, but mostly mechanical changes.

[Kwang Yul Seo]

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

[Adam Barth]

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.

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

Created attachment 154164 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #6)
> Created an attachment (id=154164) [details]
> Patch

Rebased to the latest revision.

[Kwang Yul Seo]

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.

[Adam Barth]

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.

[Adam Barth]

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.

[Adam Barth]

Comment on attachment 154164 [details]
Patch

r=me pending performance measurement.

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

(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

[Kwang Yul Seo]

Created attachment 154243 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kwang Yul Seo]

Created attachment 154253 [details]
Patch

[Kwang Yul Seo]

(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

[Adam Barth]

Comment on attachment 154253 [details]
Patch

That's great.

[Kwang Yul Seo]

Comment on attachment 154253 [details]
Patch

Clearing flags on attachment: 154253

Committed r123577: <http://trac.webkit.org/changeset/123577>

[Kwang Yul Seo]

All reviewed patches have been landed.  Closing bug.