Bug 41807

Summary: WebCore/benchmarks/parser/html-parser.html spends a lot of time in deprecatedParseURL
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, darin, mjs, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 41123    
Attachments:
Description Flags
Sample while running the benchmark with the new treebuilder
none
Patch none

Eric Seidel (no email)
Reported 2010-07-07 15:38:18 PDT
326 WebCore::HTMLConstructionSite::createElement(WebCore::AtomicHTMLToken&) 185 WebCore::Element::setAttributeMap(WTF::PassRefPtr<WebCore::NamedNodeMap>, WebCore::FragmentScriptingPermission) 162 WebCore::StyledElement::attributeChanged(WebCore::Attribute*, bool) 86 WebCore::HTMLAnchorElement::parseMappedAttribute(WebCore::Attribute*) 64 WebCore::deprecatedParseURL(WebCore::String const&) 51 WebCore::deprecatedParseURL(WebCore::String const&) 10 WebCore::StringImpl::create(unsigned short const*, unsigned int) It's all spent mallocing strings for URLs. It's not clear to me why we're hitting the slow path in deprecatedParseURL. I suspect this hits both TreeBuilders equally, and may be affecting general page load times as well. I don't know if there is a non-deprecated parseURL to replace this with (or if that's even the right solution).
Attachments
Sample while running the benchmark with the new treebuilder (332.35 KB, text/plain)
2010-07-07 15:40 PDT, Eric Seidel (no email) 		no flags
Details
Patch (1.83 KB, patch)
2010-07-07 16:03 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2010-07-07 15:40:22 PDT
Created attachment 60790 [details] Sample while running the benchmark with the new treebuilder We're not really fast enough to bother sharking yet. We still have low hanging fruit. Hence the sample instead of shark sample.
Eric Seidel (no email)
Comment 2 2010-07-07 15:46:07 PDT
Um. We always hit the slow case: // Optimize for the likely case there there is nothing to strip. if (l == length) { int k; for (k = 0; k < length; k++) { if (characters[k] > '\r') break; } if (k == length) return url; } '\r' is 13. Every damn ASCII of use is > 13. That code is just wrong. I don't know what it's trying to do though.
Eric Seidel (no email)
Comment 3 2010-07-07 15:48:10 PDT
http://trac.webkit.org/changeset/13005 was where that code was added.
Eric Seidel (no email)
Comment 4 2010-07-07 15:49:53 PDT
Looks like that should be a < instead of a >. Will prepare a patch. PLT speedup here we come... :)
Eric Seidel (no email)
Comment 5 2010-07-07 16:03:46 PDT
Created attachment 60793 [details] Patch
Adam Barth
Comment 6 2010-07-07 16:07:05 PDT
Comment on attachment 60793 [details] Patch nice
Eric Seidel (no email)
Comment 7 2010-07-07 16:09:17 PDT
I should note, the original code which darin/maciej wrote was fine. It broke since then and no one seemed to notice. I didn't dig up the exact revision where code movement caused it to break though.
Darin Adler
Comment 8 2010-07-07 16:16:49 PDT
Comment on attachment 60793 [details] Patch I'm guessing that I broke this.
Darin Adler
Comment 9 2010-07-07 16:19:40 PDT
The incorrect optimization was brand new, just added 8 weeks ago <http://trac.webkit.org/changeset/59281/trunk/WebCore/css/CSSHelper.cpp>.
Eric Seidel (no email)
Comment 10 2010-07-07 16:29:36 PDT
I think this is one of the troubles of the PLT... its so fast it's hard to tell when you actually make a beneficial change. :)
Adam Barth
Comment 11 2010-07-07 16:35:54 PDT
Yeah, I wish we had more benchmarks like the parser benchmark. It's sort of too bad that it's only a small fraction of what the PTL covers.
WebKit Commit Bot
Comment 12 2010-07-08 02:01:25 PDT
Comment on attachment 60793 [details] Patch Clearing flags on attachment: 60793 Committed r62771: <http://trac.webkit.org/changeset/62771>
WebKit Commit Bot
Comment 13 2010-07-08 02:01:30 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.