| Summary: | WebCore/benchmarks/parser/html-parser.html spends a lot of time in deprecatedParseURL | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||
| Component: | Tools / Tests | Assignee: | 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: |
|
||||||||
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.
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.
http://trac.webkit.org/changeset/13005 was where that code was added. Looks like that should be a < instead of a >. Will prepare a patch. PLT speedup here we come... :) Created attachment 60793 [details]
Patch
Comment on attachment 60793 [details]
Patch
nice
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. Comment on attachment 60793 [details]
Patch
I'm guessing that I broke this.
The incorrect optimization was brand new, just added 8 weeks ago <http://trac.webkit.org/changeset/59281/trunk/WebCore/css/CSSHelper.cpp>. 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. :) 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. Comment on attachment 60793 [details] Patch Clearing flags on attachment: 60793 Committed r62771: <http://trac.webkit.org/changeset/62771> All reviewed patches have been landed. Closing bug. |
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).