Bug 107150 – BackgroundHTMLParser shouldn't create AtomicStrings

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

[Adam Barth]

BackgroundHTMLParser shouldn't make an extra copy of every tag name

[Adam Barth]

Created attachment 183236 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 183236 [details]
Patch

Do we know if this is faster?  And do we have any good way to catch that w'ere not using regular equal with these tags?  HTMLNames is really dangerous to have on this background thread.  (Also this code is still wrong wrt namespaces, but that's a separate bug.)

[Adam Barth]

Comment on attachment 183236 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Previously, we were creating an AtomicString from the HTMLToken for
> +        every start and end tag. This caused us to allocate a new StringImpl
> +        and memcpy the start and end tag names an extra time.

I guess this isn't quite true since AtomicString will atomize them.

[Adam Barth]

> Do we know if this is faster?

Tony is still upstreaming your work from last night, so I haven't been able to run the performance test.

In general, though, I don't think we should be creating AtomicStrings on the background thread.  That creates a static dependency, which will block integration with libdispatch later.

> And do we have any good way to catch that w'ere not using regular equal with these tags?

Nope.  We need to be careful with how we're using HTMLNames.

> Also this code is still wrong wrt namespaces, but that's a separate bug.

Yes, this patch does not attempt to address that bug.

[Adam Barth]

Created attachment 183239 [details]
Patch

[Adam Barth]

I've adjusted the ChangeLog to motivate the change without referencing performance.

[Eric Seidel (no email)]

Comment on attachment 183239 [details]
Patch

I'm still concerned about using HTMLNames w/o any ASSERT protections. :)  And I would still like to know what the perf impact is.  run-perf-tests parser/html-parser-srcdoc.html should show a difference.  :)  I guess we should just rename that to -threaded, and have it force on the threaded parser.

[Adam Barth]

We can wait on this patch until we finish upstreaming the branch if you like.

[Adam Barth]

Created attachment 183325 [details]
rebase to TOT

[Adam Barth]

Created attachment 183348 [details]
works---not for review

[Adam Barth]

I need to study it more carefully, but this patch does not appear to have a performance impact on the srcdoc benchmark.

[Eric Seidel (no email)]

Our benchmark probably needs improvement (we still don't really keep the main thread very busy).  But I suspect that this stuff is also rather subtle, and as you say, just need more study.

[Adam Barth]

This looks like it saves around ~10% of the time on the parser thread.  I'm going to clean it up for review.

[Adam Barth]

Running the parser-srcdoc benchmark with the patch in bug 107236 applied:

== With patch ==

Running Time    Self        Symbol Name
12129.0ms   76.4%   0.0     Main Thread  0x878fe
3628.0ms   22.8%    0.0     WebCore::HTMLParserThread::runLoop  0x8792f

== Without patch ==

Running Time    Self        Symbol Name
11846.0ms   73.9%   0.0     Main Thread  0x87b04
4100.0ms   25.5%    0.0     WebCore::HTMLParserThread::runLoop  0x87b28

That's a 2.3% savings on the main thread and an 11.5% savings on the background thread.

[Adam Barth]

Created attachment 183526 [details]
Patch

[Eric Seidel (no email)]

I assume that you also saw a total reduction in time when instruments was not running? :)

[Adam Barth]

(In reply to comment #16)
> I assume that you also saw a total reduction in time when instruments was not running? :)

== With patch ==

avg 464.88400000016554 ms
median 431.9385000017064 ms
stdev 131.61021047785155 ms

== Without patch ==

avg 477.63914999995905 ms
median 443.0915000011737 ms
stdev 138.40526205197622 ms

Result: 2.6% faster

[WebKit Review Bot]

Comment on attachment 183526 [details]
Patch

Clearing flags on attachment: 183526

Committed r140212: <http://trac.webkit.org/changeset/140212>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.