Bug 108027 – Enable the preload scanner on the background parser thread

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Enable the preload scanner on the background parser thread

[Eric Seidel (no email)]

Created attachment 184903 [details]
Patch

[Adam Barth]

Comment on attachment 184903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184903&action=review

IMHO, we should use CompactHTMLTokens rather than HTMLTokens.  This patch means we're creating 2x the number of strings on the background thread (because both the preload scanner and the compact HTML token make strings for every tag and attribute).  Our profiles tell us that we're spending a significant amount of time creating strings, which means we don't want to double that work.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:476
> +    KURL documentURL = document()->url().copy();

Is this a thread-safe copy?

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:95
> +            String attributeName(iter->m_name.data(), iter->m_name.size());

Here we're making another copy of every attribute name and value.  If we used CompactHTMLToken, then we could use the String we've already created.

[Eric Seidel (no email)]

(In reply to comment #2)
> (From update of attachment 184903 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184903&action=review
> 
> IMHO, we should use CompactHTMLTokens rather than HTMLTokens.  This patch means we're creating 2x the number of strings on the background thread (because both the preload scanner and the compact HTML token make strings for every tag and attribute).  Our profiles tell us that we're spending a significant amount of time creating strings, which means we don't want to double that work.

That's fine.  I took this path in order to keep the main-thread working as much as it was as possible.

Either we'll need to templatize the code, fork HTMLPreloadScanner, or use CompactHTMLTokens on teh main thread.  Likely we'll end up with the later, but it's slightly tricky as you'll want to do the compacting as late as possible so as to avoid making them if possible.  On the parser thread we obviously never avoid making them.

> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:476
> > +    KURL documentURL = document()->url().copy();
> 
> Is this a thread-safe copy?


    // Makes a deep copy. Helpful only if you need to use a KURL on another
    // thread.  Since the underlying StringImpl objects are immutable, there's
    // no other reason to ever prefer copy() over plain old assignment.
    KURL copy() const;


> > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:95
> > +            String attributeName(iter->m_name.data(), iter->m_name.size());
> 
> Here we're making another copy of every attribute name and value.  If we used CompactHTMLToken, then we could use the String we've already created.

Agreed.  The having to satisfy two masters is the problem.  I chose not to fork the preload scanner, but instead redesign it so that it was threadsafe while still useable from the main thread.

[Adam Barth]

It seems like you could have the shared logic work in terms of Strings and then have a controller-like code that calls the shared code starting from either an HTMLToken or a CompactHTMLToken.  You've looked at this code in more detail than I have, so maybe there's some reason why that wouldn't work.

[Eric Seidel (no email)]

(In reply to comment #4)
> It seems like you could have the shared logic work in terms of Strings and then have a controller-like code that calls the shared code starting from either an HTMLToken or a CompactHTMLToken.  You've looked at this code in more detail than I have, so maybe there's some reason why that wouldn't work.

Yes, I think there is some potential there.  We can discuss in person tomrorow.  I'm not planning on landing this tonight.

[Adam Barth]

Created attachment 188437 [details]
WIP: Compiles but crashes every test

[Adam Barth]

Created attachment 188452 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 188452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188452&action=review

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:565
> +    config->preloadScanner = adoptPtr(new TokenPreloadScanner(document()->url().copy()));

Why create this from the main thread?

[Adam Barth]

(In reply to comment #8)
> (From update of attachment 188452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188452&action=review
> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:565
> > +    config->preloadScanner = adoptPtr(new TokenPreloadScanner(document()->url().copy()));
> 
> Why create this from the main thread?

Mostly to match the XSSAuditor and to be ready for future changes (like supporting rewinding the TokenPreloadScanner on document.write).

[Adam Barth]

Note: We need to move to a struct in any case because we'd otherwise need to send the documentURL.

[Tony Gentilcore]

Comment on attachment 188452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188452&action=review

> Source/WebCore/html/parser/BackgroundHTMLParser.h:87
> +    explicit BackgroundHTMLParser(PassRefPtr<WeakReference<BackgroundHTMLParser> >, PassOwnPtr<Configuration>);

Remove explicit, this has 2 args.

[Adam Barth]

Thanks for the review!

[Tony Gentilcore]

It would be nice to rebase so EWS can chomp on this.

[Adam Barth]

Yep!  Will do after bug 109861 lands.

[Eric Seidel (no email)]

Its not clear to me that sending everything over in one hunk is the right design. I think we may want to issue preloads sooner.  But this is definitely fine to get us going.  Thank you for finishing this.

[Adam Barth]

Created attachment 188612 [details]
Patch

[Adam Barth]

> Its not clear to me that sending everything over in one hunk is the right design. I think we may want to issue preloads sooner.  But this is definitely fine to get us going.  Thank you for finishing this.

Yeah, this patch is just a first step.  We'll definitely want to iterate from here.

[WebKit Review Bot]

Comment on attachment 188612 [details]
Patch

Clearing flags on attachment: 188612

Committed r143051: <http://trac.webkit.org/changeset/143051>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.