Bug 107150

Created attachment 183236 [details] Patch

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

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.

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

Created attachment 183239 [details] Patch

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

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.

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

Created attachment 183325 [details] rebase to TOT

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

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

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.

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

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.

Created attachment 183526 [details] Patch

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

(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

Comment on attachment 183526 [details] Patch Clearing flags on attachment: 183526 Committed r140212: <http://trac.webkit.org/changeset/140212>

All reviewed patches have been landed. Closing bug.