Make it possible to modify background HTML parser token limits.

Make it possible to modify background HTML parser token limits.

BackgroundHTMLParser currently uses hard-coded limits of:
* 10k 'outstanding' tokens, which caps the number of tokens that have
  been sent to the main parser thread but not yet ACK'd
* 1k 'pending' tokens, which specifies the number of tokens that
  should be sent to the main parser thread in each cross-thread message

The comments for these defaults indicate that there is likely room for
improvement: "These numbers have not been tuned." ... "We might want
to tune again when we're closer to turning the threaded parser on by
default." (from https://bugs.webkit.org/show_bug.cgi?id=110408).

In lab tests, we've found that for users on low bandwidth connections,
the 10k outstanding limit in particular is too aggressive and can
cause pages to fetch resources that aren't needed yet, which contend
for bandwidth with resources that are needed. This slows down page
loads.

Additionally, the 1k pending limit was chosen based on lab tests
performed on a Nexus 7, and we expect that this value may not be
optimal for users on lower end devices.

This change makes it possible to vary these values via Settings, so we
can run field trials to identify more optimal values for users on
constrained networks or low end devices.

I'll follow this change with a field trial change to vary these
values, in RenderViewImpl::Initialize (near the call to
ApplyBlinkSettings, which is where Settings is configured from
--blink-settings flag values).

BUG=497942
R=gavinp@chromium.org, jochen@chromium.org, japhet@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197074

[Bryan McQuade, 2015-06-11 17:53:57]

bmcquade@chromium.org changed reviewers:
+ gavinp@chromium.org, jochen@chromium.org

[Bryan McQuade, 2015-06-11 17:55:29]

bmcquade@chromium.org changed reviewers:
+ japhet@chromium.org

[Bryan McQuade, 2015-06-11 18:03:10]

gavinp, jochen, japhet: PTAL. Thanks!

[Nate Chapin, 2015-06-11 18:48:23]

https://codereview.chromium.org/1175183006/diff/1/Source/core/frame/Settings.in
File Source/core/frame/Settings.in (right):

https://codereview.chromium.org/1175183006/diff/1/Source/core/frame/Settings....
Source/core/frame/Settings.in:257: backgroundHtmlParserPendingTokenLimit
type=int, initial=0
Should these be unsigned?

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/Bac...
File Source/core/html/parser/BackgroundHTMLParser.cpp (right):

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/Bac...
Source/core/html/parser/BackgroundHTMLParser.cpp:104: ,
m_outstandingTokenLimit(std::max(config->outstandingTokenLimit,
config->pendingTokenLimit))
Why take the max of the two here? Should we assert that outstanding is always
larger than pending in the Configuration?

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/HTM...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLDocumentParser.cpp:786: if (document()->settings())
{
I think Document::settings should only be null here for XHR document responses.
Any thoughts on whether it's a bad thing to use the defaults for those kinds of
document?

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLDocumentParser.cpp:789: if
(document()->settings()->backgroundHtmlParserPendingTokenLimit() > 0)
if these become unsigned, drop the " > 0"

[Bryan McQuade, 2015-06-12 14:12:59]

https://codereview.chromium.org/1175183006/diff/1/Source/core/frame/Settings.in
File Source/core/frame/Settings.in (right):

https://codereview.chromium.org/1175183006/diff/1/Source/core/frame/Settings....
Source/core/frame/Settings.in:257: backgroundHtmlParserPendingTokenLimit
type=int, initial=0
On 2015/06/11 18:48:23, Nate Chapin wrote:
> Should these be unsigned?

Ah, didn't know that was supported. Changed. Thanks!

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/Bac...
File Source/core/html/parser/BackgroundHTMLParser.cpp (right):

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/Bac...
Source/core/html/parser/BackgroundHTMLParser.cpp:104: ,
m_outstandingTokenLimit(std::max(config->outstandingTokenLimit,
config->pendingTokenLimit))
On 2015/06/11 18:48:23, Nate Chapin wrote:
> Why take the max of the two here? Should we assert that outstanding is always
> larger than pending in the Configuration?

Yes, that's clearer. Agree. I've made this change.

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/HTM...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLDocumentParser.cpp:786: if (document()->settings())
{
Ah, interesting. I think varying these constants are less important for XHR'd
documents. The primary motivation for being able to change these values is to be
able to control how much we look ahead of the parser with the preload scanner,
which isn't relevant for XHR'd documents - IIUC XHR'd docs don't actually
trigger fetches for things like <scripts> until they are inserted into an HTML
document(let me know if that's not right). So I think this is ok. Let me know if
you disagree though.

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLDocumentParser.cpp:789: if
(document()->settings()->backgroundHtmlParserPendingTokenLimit() > 0)
On 2015/06/11 18:48:23, Nate Chapin wrote:
> if these become unsigned, drop the " > 0"

Done.

[Nate Chapin, 2015-06-12 17:03:00]

LGTM

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/HTM...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/1175183006/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLDocumentParser.cpp:786: if (document()->settings())
{
On 2015/06/12 14:12:59, Bryan McQuade wrote:
> Ah, interesting. I think varying these constants are less important for XHR'd
> documents. The primary motivation for being able to change these values is to
be
> able to control how much we look ahead of the parser with the preload scanner,
> which isn't relevant for XHR'd documents - IIUC XHR'd docs don't actually
> trigger fetches for things like <scripts> until they are inserted into an HTML
> document(let me know if that's not right). So I think this is ok. Let me know
if
> you disagree though.

I'm not terribly familiar with our XHR document logic, but in general I agree
that documents without a frame() shouldn't be able to request subresources.

[Bryan McQuade, 2015-06-13 00:44:44]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2015-06-13 00:45:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1175183006/20001

[commit-bot: I haz the power, 2015-06-13 00:48:37]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197074