Bug 92240

Created attachment 154328 [details] Patch

Comment on attachment 154328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154328&action=review > Source/WebCore/html/parser/HTMLStackItem.h:89 > + } else > + ASSERT_NOT_REACHED(); Generally we prefer to use switch statements for situations like this so that the compiler can tell us when we've forgotten an enum value. In those cases, we leave off the "default" case. > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1635 > + RefPtr<HTMLStackItem> itemForContextElement; This point is that normally we don't need to hold a reference to a stack item because m_tree hold it for us, but in the case of the root node, we need to create an extra item? I think it would be cleaner if item where a RefPtr. That would mean an extra ref()/deref() in the non-root case, but I don't think this function is that hot.

Comment on attachment 154328 [details] Patch This is fine, pending the style issue and an HTML parser benchmark run. :)

(In reply to comment #3) > (From update of attachment 154328 [details]) > This is fine, pending the style issue and an HTML parser benchmark run. :) It seems this patch slightly improves the performance even with an extra ref()/deref() in the non-root case. * 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= 2839.0 ms median= 2838.0 ms, stdev= 3.30151480384 ms, min= 2833.0 ms, max= 2847.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= 2818.9 ms median= 2818.0 ms, stdev= 4.21781934179 ms, min= 2812.0 ms, max= 2830.0 ms

(In reply to comment #2) > > Source/WebCore/html/parser/HTMLStackItem.h:89 > > + } else > > + ASSERT_NOT_REACHED(); > > Generally we prefer to use switch statements for situations like this so that the compiler can tell us when we've forgotten an enum value. In those cases, we leave off the "default" case. Okay. That's a good convention. > > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1635 > > + RefPtr<HTMLStackItem> itemForContextElement; > > This point is that normally we don't need to hold a reference to a stack item because m_tree hold it for us, but in the case of the root node, we need to create an extra item? > > I think it would be cleaner if item where a RefPtr. That would mean an extra ref()/deref() in the non-root case, but I don't think this function is that hot. Yeah, it is much cleaner. I confirmed that there is no observable performance penalty here.

Committed r123671: <http://trac.webkit.org/changeset/123671>