RESOLVED FIXED 92240
Read tag names and attributes from the saved tokens in HTMLTreeBuilder::resetInsertionModeAppropriately.
https://bugs.webkit.org/show_bug.cgi?id=92240
Summary Read tag names and attributes from the saved tokens in HTMLTreeBuilder::reset...
Kwang Yul Seo
Reported Wednesday, July 25, 2012 1:16:34 PM UTC
This is a follow-up patch for r123577.
Attachments
Patch (7.42 KB, patch)
2012-07-25 05:24 PDT, Kwang Yul Seo
abarth: review+
abarth: commit-queue-
Kwang Yul Seo
Comment 1 Wednesday, July 25, 2012 1:24:08 PM UTC
Adam Barth
Comment 2 Wednesday, July 25, 2012 4:56:00 PM UTC
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.
Adam Barth
Comment 3 Wednesday, July 25, 2012 4:56:28 PM UTC
Comment on attachment 154328 [details] Patch This is fine, pending the style issue and an HTML parser benchmark run. :)
Kwang Yul Seo
Comment 4 Wednesday, July 25, 2012 11:43:43 PM UTC
(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
Kwang Yul Seo
Comment 5 Wednesday, July 25, 2012 11:46:42 PM UTC
(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.
Kwang Yul Seo
Comment 6 Thursday, July 26, 2012 12:00:21 AM UTC
Note You need to log in before you can comment on or make changes to this bug.