Bug 92065 - Ensure Noah's ark without reading the DOM tree.
Summary: Ensure Noah's ark without reading the DOM tree.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kwang Yul Seo
URL:
Keywords:
Depends on:
Blocks: 92057
  Show dependency treegraph
 
Reported: 2012-07-23 22:52 PDT by Kwang Yul Seo
Modified: 2012-07-24 15:51 PDT (History)
2 users (show)

See Also:


Attachments
Patch (9.49 KB, patch)
2012-07-23 22:55 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (9.50 KB, patch)
2012-07-24 01:17 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (8.67 KB, patch)
2012-07-24 06:08 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2012-07-23 22:52:39 PDT
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.
Comment 1 Kwang Yul Seo 2012-07-23 22:55:40 PDT
Created attachment 153966 [details]
Patch
Comment 2 Kwang Yul Seo 2012-07-23 23:02:14 PDT
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?
Comment 3 Adam Barth 2012-07-23 23:35:10 PDT
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.
Comment 4 Adam Barth 2012-07-23 23:35:36 PDT
Can you run the html-parser benchmark on this change?  I suspect it won't show any change.
Comment 5 Adam Barth 2012-07-23 23:36:38 PDT
> 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.
Comment 6 Adam Barth 2012-07-23 23:37:16 PDT
Let's give Eric a chance to look at the patch.
Comment 7 Adam Barth 2012-07-23 23:37:30 PDT
(I should say that it generally looks good to me.)
Comment 8 Eric Seidel (no email) 2012-07-23 23:57:06 PDT
Looks fine to me.
Comment 9 Kwang Yul Seo 2012-07-24 01:08:19 PDT
(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>&"
Comment 10 Kwang Yul Seo 2012-07-24 01:10:02 PDT
(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 ;-)
Comment 11 Kwang Yul Seo 2012-07-24 01:17:30 PDT
Created attachment 153983 [details]
Patch
Comment 12 Kwang Yul Seo 2012-07-24 01:18:09 PDT
(In reply to comment #11)
> Created an attachment (id=153983) [details]
> Patch

Changed to use the type "const Vector<Attribute>&".
Comment 13 Kwang Yul Seo 2012-07-24 01:19:16 PDT
(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.
Comment 14 Kwang Yul Seo 2012-07-24 05:35:12 PDT
(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.
Comment 15 Kwang Yul Seo 2012-07-24 06:08:48 PDT
Created attachment 154038 [details]
Patch
Comment 16 Kwang Yul Seo 2012-07-24 06:13:22 PDT
(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
Comment 17 Kwang Yul Seo 2012-07-24 06:18:09 PDT
(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())
Comment 18 Adam Barth 2012-07-24 10:40:41 PDT
> 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.
Comment 19 Adam Barth 2012-07-24 10:43:54 PDT
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.
Comment 20 Kwang Yul Seo 2012-07-24 15:14:33 PDT
(In reply to comment #19)
> I assume that m_token->name() is already an AtomicString.

Yes, it is.
Comment 21 Kwang Yul Seo 2012-07-24 15:20:00 PDT
Comment on attachment 154038 [details]
Patch

Clearing flags on attachment: 154038

Committed r123537: <http://trac.webkit.org/changeset/123537>
Comment 22 Kwang Yul Seo 2012-07-24 15:20:06 PDT
All reviewed patches have been landed.  Closing bug.