Bug 92065 – Ensure Noah's ark without reading the DOM tree.

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

[Kwang Yul Seo]

Technically we shouldn't read attributes back from the DOM. If JavaScript changes the attributes values, we could get a slightly wrong output here.
    
Changed HTMLStackItem to store the qualified name. Read attributes from tokens saved in the active formatting element list.

[Kwang Yul Seo]

Created attachment 153966 [details]
Patch

[Kwang Yul Seo]

This patch 'slightly' changes the behavior because HTMLFormattingElementList::ensureNoahsArkCondition now reads attributes from the saved token, not from the element.

If you think we need a layout test case to cover this change, I will add one. 

BTW, where is the most appropriate directory to put the test case? fast/parser?

[Adam Barth]

Comment on attachment 153966 [details]
Patch

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

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:181
> +    Vector<Attribute> attributes = newItem->token()->attributes();

This makes a copy of the attribute vector.  Is that what you intend?  Another option is to use the type "const Vector<Attribute>&" to avoid the copy.

[Adam Barth]

Can you run the html-parser benchmark on this change?  I suspect it won't show any change.

[Adam Barth]

> BTW, where is the most appropriate directory to put the test case? fast/parser?

Yes.  fast/parser is the right place.

I'm not sure exactly how to write a test.

[Adam Barth]

Let's give Eric a chance to look at the patch.

[Adam Barth]

(I should say that it generally looks good to me.)

[Eric Seidel (no email)]

Looks fine to me.

[Kwang Yul Seo]

(In reply to comment #3)
> > Source/WebCore/html/parser/HTMLFormattingElementList.cpp:181
> > +    Vector<Attribute> attributes = newItem->token()->attributes();
> 
> This makes a copy of the attribute vector.  Is that what you intend?  Another option is to use the type "const Vector<Attribute>&" to avoid the copy.

Oops. That's not my intention. I will fix the patch to use the type "const Vector<Attribute>&"

[Kwang Yul Seo]

(In reply to comment #5)
> I'm not sure exactly how to write a test.

That's the reason why I couldn't write a test ;-)

[Kwang Yul Seo]

Created attachment 153983 [details]
Patch

[Kwang Yul Seo]

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

Changed to use the type "const Vector<Attribute>&".

[Kwang Yul Seo]

(In reply to comment #4)
> Can you run the html-parser benchmark on this change?  I suspect it won't show any change.

Sure. I will post the html-parser benchmark results.

[Kwang Yul Seo]

(In reply to comment #13)
> Sure. I will post the html-parser benchmark results.

Unfortunately this patch causes 2.3% performance regression!

* 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= 2740.75 ms
median= 2740.5 ms, stdev= 3.85843232414 ms, min= 2734.0 ms, max= 2750.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= 2804.0 ms
median= 2804.0 ms, stdev= 3.89871773792 ms, min= 2798.0 ms, max= 2814.0 ms


It seems creating a QualifiedName in HTMLStackNode is expensive. Let's see how we can improve this patch.

[Kwang Yul Seo]

Created attachment 154038 [details]
Patch

[Kwang Yul Seo]

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

* After this new patch

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= 2755.45 ms
median= 2754.0 ms, stdev= 4.76943392868 ms, min= 2748.0 ms, max= 2765.0 ms

[Kwang Yul Seo]

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=154038) [details] [details]
> > Patch
> 
> * After this new patch
> 
> 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= 2755.45 ms
> median= 2754.0 ms, stdev= 4.76943392868 ms, min= 2748.0 ms, max= 2765.0 ms

This patch has two changes:

1. Don't create a QualifedName in HTMLStackItem.
2. Compare the local name and namespace URI. If the local names are different, we don't need to compare namespace URIs. Use || short-circuits.

-        Element* candidate = entry.element();
-        if (newElement->tagQName() != candidate->tagQName())
+        HTMLStackItem* candidate = entry.stackItem().get();
+        if (newItem->localName() != candidate->localName() || newItem->namespaceURI() != candidate->namespaceURI())

[Adam Barth]

> Unfortunately this patch causes 2.3% performance regression!

Thanks for running the benchmark.  It can be hard to predict what changes will have an effect, which is why we like to be careful and run it for each change.

[Adam Barth]

Comment on attachment 154038 [details]
Patch

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

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:134
> -void HTMLFormattingElementList::tryToEnsureNoahsArkConditionQuickly(Element* newElement, Vector<Element*>& remainingCandidates)
> +void HTMLFormattingElementList::tryToEnsureNoahsArkConditionQuickly(HTMLStackItem* newItem, Vector<HTMLStackItem*>& remainingCandidates)

Yeah, this function is very hot.

> Source/WebCore/html/parser/HTMLStackItem.h:60
> +    const AtomicString& localName() const { return m_token->name(); }

I assume that m_token->name() is already an AtomicString.

[Kwang Yul Seo]

(In reply to comment #19)
> I assume that m_token->name() is already an AtomicString.

Yes, it is.

[Kwang Yul Seo]

Comment on attachment 154038 [details]
Patch

Clearing flags on attachment: 154038

Committed r123537: <http://trac.webkit.org/changeset/123537>

[Kwang Yul Seo]

All reviewed patches have been landed.  Closing bug.