Bug 41807 – WebCore/benchmarks/parser/html-parser.html spends a lot of time in deprecatedParseURL

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

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).

[Eric Seidel (no email)]

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)]

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)]

http://trac.webkit.org/changeset/13005 was where that code was added.

[Eric Seidel (no email)]

Looks like that should be a < instead of a >.  Will prepare a patch.

PLT speedup here we come... :)

[Eric Seidel (no email)]

Created attachment 60793 [details]
Patch

[Adam Barth]

Comment on attachment 60793 [details]
Patch

nice

[Eric Seidel (no email)]

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 on attachment 60793 [details]
Patch

I'm guessing that I broke this.

[Darin Adler]

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)]

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]

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 on attachment 60793 [details]
Patch

Clearing flags on attachment: 60793

Committed r62771: <http://trac.webkit.org/changeset/62771>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.