Bug 92065

Created attachment 153966 [details] Patch

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

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

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

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

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

Looks fine to me.

(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>&"

(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 ;-)

Created attachment 153983 [details] Patch

(In reply to comment #11) > Created an attachment (id=153983) [details] > Patch Changed to use the type "const Vector<Attribute>&".

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

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

Created attachment 154038 [details] Patch

(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

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

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

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

Comment on attachment 154038 [details] Patch Clearing flags on attachment: 154038 Committed r123537: <http://trac.webkit.org/changeset/123537>

All reviewed patches have been landed. Closing bug.