[webkit-reviews] review granted: [Bug 224609] HTML parser should yield more aggressively : [Attachment 429150] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 224609: HTML parser should yield more aggressively
https://bugs.webkit.org/show_bug.cgi?id=224609

Attachment 429150: patch

https://bugs.webkit.org/attachment.cgi?id=429150&action=review




--- Comment #12 from Darin Adler <darin at apple.com> ---
Comment on attachment 429150
  --> https://bugs.webkit.org/attachment.cgi?id=429150
patch

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

> Source/WebCore/ChangeLog:26
> +	   Consider yielding before script execution after 16ms has elapsed and
at least 256 tokens have been parsed.

This is very straightforward to read in the code.

> Source/WebCore/ChangeLog:28
> +	   Only yield for synchronous scripts.
> +	   Don't yield on very short inline scripts (this is an imperfect way
to try to guess the execution cost).

This is harder to understand, so some comment would be good with the constant
that defines a short inline script and the code that’s checks a script is
synchronous. I really like the "this is an imperfect way to try to guess the
execution cost" remark, the core of a classically helpful "why" comment, and
similarly, a comment that says why we don't need to yield for async scripts
would be a plus.

> Source/WebCore/html/parser/HTMLParserScheduler.cpp:123
> +    static const auto elapsedTimeLimit = 16_ms;
> +    static const auto tokenLimit = 256;
> +    static const auto inlineScriptLengthLimit = 1024;

Should use constexpr.