WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Kwang Yul Seo
Comment 1
Wednesday, July 25, 2012 1:24:08 PM UTC
Created
attachment 154328
[details]
Patch
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
Committed
r123671
: <
http://trac.webkit.org/changeset/123671
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug