Reland: BackgroundHTMLParser: Introduce ParsedChunkQueue to pass ParsedChunks to main thread

Reland: BackgroundHTMLParser: Introduce ParsedChunkQueue to pass ParsedChunks to main thread

Fixed shutdown race issue: ParsedChunkQueue is now ThreadSafeRefCounted.

---

Before this CL, each ParsedChunk from the background parser thread was
posted as separate tasks, which wasted time in main thread on poor CPU
devices (esp. mobiles).

This CL attempts to minimize postTask by passing ParsedChunks via
introduced ParsedChunkQueue class and only using postTask to notify
that it is not empty.

BUG=540988
Committed: https://crrev.com/63f797324ae8302ddd405bf4f7b9a03ca3926f8d
Cr-Commit-Position: refs/heads/master@{#356261}

Committed: https://crrev.com/36769005e024e32de8436b573f4652e59acc2f93
Cr-Commit-Position: refs/heads/master@{#356739}

[kouhei (in TOK), 2015-10-13 02:24:46]

kouhei@chromium.org changed reviewers:
+ kinuko@chromium.org, tzik@chromium.org, yoav@yoav.ws

[kouhei (in TOK), 2015-10-13 02:24:47]

Still prototypy, but works.
tzik: Would you measure performance gain from this patch on your N4?

[tzik, 2015-10-13 04:07:59]

On 2015/10/13 02:24:47, kouhei (catching-up) wrote:
> Still prototypy, but works.
> tzik: Would you measure performance gain from this patch on your N4?

Measured.
This seems to cut >10ms of main HTML parsing for a site.
Here are the tracing results. The diff is a series of
HTMLDocumentParser::didReceiveParsedChunkFromBackgroundParser items above the
first HTMLParserThread task.
before: https://drive.google.com/open?id=0BwpGjpkRI2N-cXJRc2F6dEc4SUU
after: https://drive.google.com/open?id=0BwpGjpkRI2N-TnRZVm5WRi1rY2c

[kouhei (in TOK), 2015-10-13 05:01:06]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-13 05:02:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394663004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394663004/20001

[kouhei (in TOK), 2015-10-13 05:36:39]

tzik: Thanks for measuring!

yoav, kinuko: Would you take a look?

[kinuko, 2015-10-13 10:21:41]

Looking reasonable.  Should we be wary of the queue length in such cases?  (we
would be putting the same # of chunks regardless, though)  Should we collect UMA
to see how often / how many tasks this could be saving in real world usage?

Issue description nit: IPC tasks -> just tasks, not IPC

tzik: would be nice to note 10ms out of how long

https://codereview.chromium.org/1394663004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right):

https://codereview.chromium.org/1394663004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:106: void
hasPendingParsedChunks();
nit: notifyPendingParsedChunk ? This method name makes me feel this returns
boolean.

[kouhei (in TOK), 2015-10-19 03:50:42]

On 2015/10/13 10:21:41, kinuko wrote:
> Looking reasonable.  Should we be wary of the queue length in such cases?  (we
> would be putting the same # of chunks regardless, though)  Should we collect
UMA
> to see how often / how many tasks this could be saving in real world usage?
> 
> Issue description nit: IPC tasks -> just tasks, not IPC
Done.

> tzik: would be nice to note 10ms out of how long
10ms out of 475ms in the measured case.

>
https://codereview.chromium.org/1394663004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right):
> 
>
https://codereview.chromium.org/1394663004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:106: void
> hasPendingParsedChunks();
> nit: notifyPendingParsedChunk ? This method name makes me feel this returns
> boolean.
Done.

[kouhei (in TOK), 2015-10-19 04:00:28]

On 2015/10/19 03:50:42, kouhei wrote:
> On 2015/10/13 10:21:41, kinuko wrote:
> > Looking reasonable.  Should we be wary of the queue length in such cases? 
(we
> > would be putting the same # of chunks regardless, though)
I don't think it would be an issue here. The same task item was queued in the
main thread task queue before the patch, and now its queued in ParsedChunkQueue.

[kinuko, 2015-10-19 09:18:03]

ok. lgtm

I'm still interested in how many chunks we could be queuing (in whichever queue)
but it could be tracked separately from this change.

[Yoav Weiss, 2015-10-19 12:35:26]

On 2015/10/19 09:18:03, kinuko wrote:
> ok. lgtm
> 
> I'm still interested in how many chunks we could be queuing (in whichever
queue)
> but it could be tracked separately from this change.

Test failure seems relevant.

[esprehn, 2015-10-20 23:46:50]

Description was changed from

==========
BackgroundHTMLParser: Introduce ParsedChunkQueue to pass ParsedChunks to main
thread

Before this CL, each ParsedChunk from the background parser thread was posted as
separate tasks, which wasted time in main thread on poor CPU devices (esp.
mobiles).
This CL attempts to minimize postTask by passing ParsedChunks via introduced
ParsedChunkQueue class and only using postTask to notify that it is not empty.

BUG=540988
==========

to

==========
BackgroundHTMLParser: Introduce ParsedChunkQueue to pass ParsedChunks to main
thread

Before this CL, each ParsedChunk from the background parser thread was
posted as separate tasks, which wasted time in main thread on poor CPU
devices (esp. mobiles).
This CL attempts to minimize postTask by passing ParsedChunks via
introduced ParsedChunkQueue class and only using postTask to notify
that it is not empty.

BUG=540988
==========

[esprehn, 2015-10-20 23:46:55]

Description was changed from

==========
BackgroundHTMLParser: Introduce ParsedChunkQueue to pass ParsedChunks to main
thread

Before this CL, each ParsedChunk from the background parser thread was
posted as separate tasks, which wasted time in main thread on poor CPU
devices (esp. mobiles).
This CL attempts to minimize postTask by passing ParsedChunks via
introduced ParsedChunkQueue class and only using postTask to notify
that it is not empty.

BUG=540988
==========

to

==========
BackgroundHTMLParser: Introduce ParsedChunkQueue to pass ParsedChunks to main
thread

Before this CL, each ParsedChunk from the background parser thread was
posted as separate tasks, which wasted time in main thread on poor CPU
devices (esp. mobiles).

This CL attempts to minimize postTask by passing ParsedChunks via
introduced ParsedChunkQueue class and only using postTask to notify
that it is not empty.

BUG=540988
==========

[esprehn, 2015-10-20 23:48:19]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-10-20 23:48:20]

https://codereview.chromium.org/1394663004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.cpp (right):

https://codereview.chromium.org/1394663004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.cpp:23: void
ParsedChunkQueue::takeAll(Vector<OwnPtr<HTMLDocumentParser::ParsedChunk>>* vec)
vector, don't abbreviate in blink. :)

https://codereview.chromium.org/1394663004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.h (right):

https://codereview.chromium.org/1394663004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.h:25: void
takeAll(Vector<OwnPtr<HTMLDocumentParser::ParsedChunk>>*);
reference

[kouhei (in TOK), 2015-10-26 07:42:03]

PTAL. LayoutTest failure fixed.

https://codereview.chromium.org/1394663004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.cpp (right):

https://codereview.chromium.org/1394663004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.cpp:23: void
ParsedChunkQueue::takeAll(Vector<OwnPtr<HTMLDocumentParser::ParsedChunk>>* vec)
On 2015/10/20 23:48:20, esprehn wrote:
> vector, don't abbreviate in blink. :)

Done.

https://codereview.chromium.org/1394663004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.h (right):

https://codereview.chromium.org/1394663004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.h:25: void
takeAll(Vector<OwnPtr<HTMLDocumentParser::ParsedChunk>>*);
On 2015/10/20 23:48:20, esprehn wrote:
> reference

Done.

[Yoav Weiss, 2015-10-26 07:50:56]

LGTM % nits

https://codereview.chromium.org/1394663004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp
(right):

https://codereview.chromium.org/1394663004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:296: bool
firstChunk = m_parsedChunkQueue->enqueue(chunk.release());
firstChunk is not really accurate here. Can we change to isEmpty or something
similar?

https://codereview.chromium.org/1394663004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.cpp (right):

https://codereview.chromium.org/1394663004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.cpp:16: MutexLocker
locker(m_mutex);
Can you add a comment as to why the mutex is needed? (so, which multiple threads
are calling these methods)

[kouhei (in TOK), 2015-10-27 05:57:30]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2015-10-27 05:57:31]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/1394663004/#ps100001
(title: "add comment / isEmpty")

[kouhei (in TOK), 2015-10-27 05:57:37]

Thanks!

https://codereview.chromium.org/1394663004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp
(right):

https://codereview.chromium.org/1394663004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:296: bool
firstChunk = m_parsedChunkQueue->enqueue(chunk.release());
On 2015/10/26 07:50:56, Yoav Weiss wrote:
> firstChunk is not really accurate here. Can we change to isEmpty or something
> similar?

Done.

https://codereview.chromium.org/1394663004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.cpp (right):

https://codereview.chromium.org/1394663004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/ParsedChunkQueue.cpp:16: MutexLocker
locker(m_mutex);
On 2015/10/26 07:50:56, Yoav Weiss wrote:
> Can you add a comment as to why the mutex is needed? (so, which multiple
threads
> are calling these methods)

Done.

[commit-bot: I haz the power, 2015-10-27 05:57:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394663004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394663004/100001

[commit-bot: I haz the power, 2015-10-27 06:58:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-27 06:58:48]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kouhei (in TOK), 2015-10-27 08:45:30]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2015-10-27 08:45:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394663004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394663004/100001

[commit-bot: I haz the power, 2015-10-27 10:10:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-27 10:11:27]

Patchset 6 (id:??) landed as
https://crrev.com/63f797324ae8302ddd405bf4f7b9a03ca3926f8d
Cr-Commit-Position: refs/heads/master@{#356261}

[kouhei (in TOK), 2015-10-27 11:58:16]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1422233002/ by kouhei@chromium.org.

The reason for reverting is: Failed MSAN:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests....

[kouhei (in TOK), 2015-10-28 04:28:31]

Description was changed from

==========
BackgroundHTMLParser: Introduce ParsedChunkQueue to pass ParsedChunks to main
thread

Before this CL, each ParsedChunk from the background parser thread was
posted as separate tasks, which wasted time in main thread on poor CPU
devices (esp. mobiles).

This CL attempts to minimize postTask by passing ParsedChunks via
introduced ParsedChunkQueue class and only using postTask to notify
that it is not empty.

BUG=540988

Committed: https://crrev.com/63f797324ae8302ddd405bf4f7b9a03ca3926f8d
Cr-Commit-Position: refs/heads/master@{#356261}
==========

to

==========
Reland: BackgroundHTMLParser: Introduce ParsedChunkQueue to pass ParsedChunks to
main thread

Fixed shutdown race issue: ParsedChunkQueue is now ThreadSafeRefCounted.

---

Before this CL, each ParsedChunk from the background parser thread was
posted as separate tasks, which wasted time in main thread on poor CPU
devices (esp. mobiles).

This CL attempts to minimize postTask by passing ParsedChunks via
introduced ParsedChunkQueue class and only using postTask to notify
that it is not empty.

BUG=540988

Committed: https://crrev.com/63f797324ae8302ddd405bf4f7b9a03ca3926f8d
Cr-Commit-Position: refs/heads/master@{#356261}
==========

[kouhei (in TOK), 2015-10-28 04:28:38]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-28 04:28:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394663004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394663004/120001

[kouhei (in TOK), 2015-10-28 04:29:45]

kinuko: PTAL

[commit-bot: I haz the power, 2015-10-28 07:11:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-28 07:12:00]

Dry run: This issue passed the CQ dry run.

[kinuko (google), 2015-10-29 02:00:56]

lgtm

[kouhei (in TOK), 2015-10-29 02:17:47]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2015-10-29 02:17:47]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/1394663004/#ps120001
(title: "ParsedChunkQueue should be ThreadSafeRefCounted")

[commit-bot: I haz the power, 2015-10-29 02:18:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394663004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394663004/120001

[Yoav Weiss, 2015-10-29 02:20:23]

Are you sure RefPtr is thread safe? Has that recently changed?

[kouhei (in TOK), 2015-10-29 02:21:10]

On 2015/10/29 02:20:23, Yoav Weiss wrote:
> Are you sure RefPtr is thread safe? Has that recently changed?

We use thread safe version here: ThreadSafeRefCounted<ParsedChunkQueue>

[Yoav Weiss, 2015-10-29 02:23:21]

On 2015/10/29 02:21:10, kouhei wrote:
> On 2015/10/29 02:20:23, Yoav Weiss wrote:
> > Are you sure RefPtr is thread safe? Has that recently changed?
> 
> We use thread safe version here: ThreadSafeRefCounted<ParsedChunkQueue>

Fair enough. LGTM

[commit-bot: I haz the power, 2015-10-29 02:23:45]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-29 02:24:35]

Patchset 7 (id:??) landed as
https://crrev.com/36769005e024e32de8436b573f4652e59acc2f93
Cr-Commit-Position: refs/heads/master@{#356739}