content: Add switch to enable checker-imaging.

content: Add switch to enable checker-imaging.

The change adds a cc switch to enable checker-imaging and plumbs it to
the renderer. Also since during image decode, we end up allocating
shared memory which makes a synchronous call to the IO thread, the
image worker task runner needs to allow waiting.

TBR=piman@chromium.org
BUG=686267
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2714863005
Cr-Commit-Position: refs/heads/master@{#453523}
Committed: https://chromium.googlesource.com/chromium/src/+/19376239fec9373251062c97f250d6f8e98b094e

[Khushal, 2017-02-25 00:01:22]

Description was changed from

==========
content: Add switch to enable checker-imaging.

The change adds a cc switch to enable checker-imaging and plumbs it to
the renderer. Also since during image decode, we end up allocating
shared memory which makes a synchronous call to the IO thread, the
image worker task runner needs to allow waiting.

BUG=686267
==========

to

==========
content: Add switch to enable checker-imaging.

The change adds a cc switch to enable checker-imaging and plumbs it to
the renderer. Also since during image decode, we end up allocating
shared memory which makes a synchronous call to the IO thread, the
image worker task runner needs to allow waiting.

BUG=686267
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Khushal, 2017-02-25 00:05:06]

khushalsagar@chromium.org changed reviewers:
+ enne@chromium.org, piman@chromium.org

[Khushal, 2017-02-25 00:05:07]

I'll TBR piman for the RPHI change.

[Khushal, 2017-02-25 00:05:36]

Description was changed from

==========
content: Add switch to enable checker-imaging.

The change adds a cc switch to enable checker-imaging and plumbs it to
the renderer. Also since during image decode, we end up allocating
shared memory which makes a synchronous call to the IO thread, the
image worker task runner needs to allow waiting.

BUG=686267
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
content: Add switch to enable checker-imaging.

The change adds a cc switch to enable checker-imaging and plumbs it to
the renderer. Also since during image decode, we end up allocating
shared memory which makes a synchronous call to the IO thread, the
image worker task runner needs to allow waiting.

TBR=piman@chromium.org
BUG=686267
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[enne (OOO), 2017-02-25 00:09:12]

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:251:
.WithBaseSyncPrimitives());
How did this work before?

[Khushal, 2017-02-25 00:14:01]

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:251:
.WithBaseSyncPrimitives());
On 2017/02/25 00:09:12, enne wrote:
> How did this work before?

It didn't, nobody uses this worker yet. I think on the raster threads, we allow
waiting, which is where this happens right now.
https://cs.chromium.org/chromium/src/content/renderer/categorized_worker_pool...

[enne (OOO), 2017-02-25 00:16:31]

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:251:
.WithBaseSyncPrimitives());
On 2017/02/25 at 00:09:12, enne wrote:
> How did this work before?

Oh right right, this was all within cc in previous patches.  Oops, silly
question.

Hmm.  Do we make synchronous calls to the image worker thread at this point?
Just wondering about potential deadlock.  Can this sort of thing (where thread
can make synchronous calls to, who can call it synchronously, etc) be better
documented, maybe on the init params itself or elsewhere?

[Khushal, 2017-02-25 02:48:03]

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:251:
.WithBaseSyncPrimitives());
On 2017/02/25 00:16:30, enne wrote:
> On 2017/02/25 at 00:09:12, enne wrote:
> > How did this work before?
> 
> Oh right right, this was all within cc in previous patches.  Oops, silly
> question.
> 
> Hmm.  Do we make synchronous calls to the image worker thread at this point?
> Just wondering about potential deadlock.  Can this sort of thing (where thread
> can make synchronous calls to, who can call it synchronously, etc) be better
> documented, maybe on the init params itself or elsewhere?

We do make sync calls to the worker from only the compositor thread, for
flushing pending tasks. But I think we should be fine. The worker only waits on
the IO thread, and thats not any different from what already happens with the
raster worker threads.

I can add a comment on the InitParams in cc that specifies that the compositor
thread can make sync calls to the worker, in addition to the comment here?

While I was here, I was also wondering why we use shared memory for the
discardable memory allocation. This happens when we are allocating memory for
the decode, which is used in the renderer only right?

[enne (OOO), 2017-02-27 20:45:51]

lgtm

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:251:
.WithBaseSyncPrimitives());
On 2017/02/25 at 02:48:03, Khushal wrote:
> On 2017/02/25 00:16:30, enne wrote:
> > On 2017/02/25 at 00:09:12, enne wrote:
> > > How did this work before?
> > 
> > Oh right right, this was all within cc in previous patches.  Oops, silly
> > question.
> > 
> > Hmm.  Do we make synchronous calls to the image worker thread at this point?
> > Just wondering about potential deadlock.  Can this sort of thing (where
thread
> > can make synchronous calls to, who can call it synchronously, etc) be better
> > documented, maybe on the init params itself or elsewhere?
> 
> We do make sync calls to the worker from only the compositor thread, for
flushing pending tasks. But I think we should be fine. The worker only waits on
the IO thread, and thats not any different from what already happens with the
raster worker threads.
> 
> I can add a comment on the InitParams in cc that specifies that the compositor
thread can make sync calls to the worker, in addition to the comment here?
> 
> While I was here, I was also wondering why we use shared memory for the
discardable memory allocation. This happens when we are allocating memory for
the decode, which is used in the renderer only right?

Yeah, I think making a comment that this thread behaves like the raster worker
threads would be fine.

I'm less clear on what's going on with the discardable stuff here.

[Khushal, 2017-02-27 22:57:05]

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2714863005/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:251:
.WithBaseSyncPrimitives());
On 2017/02/27 20:45:50, enne wrote:
> On 2017/02/25 at 02:48:03, Khushal wrote:
> > On 2017/02/25 00:16:30, enne wrote:
> > > On 2017/02/25 at 00:09:12, enne wrote:
> > > > How did this work before?
> > > 
> > > Oh right right, this was all within cc in previous patches.  Oops, silly
> > > question.
> > > 
> > > Hmm.  Do we make synchronous calls to the image worker thread at this
point?
> > > Just wondering about potential deadlock.  Can this sort of thing (where
> thread
> > > can make synchronous calls to, who can call it synchronously, etc) be
better
> > > documented, maybe on the init params itself or elsewhere?
> > 
> > We do make sync calls to the worker from only the compositor thread, for
> flushing pending tasks. But I think we should be fine. The worker only waits
on
> the IO thread, and thats not any different from what already happens with the
> raster worker threads.
> > 
> > I can add a comment on the InitParams in cc that specifies that the
compositor
> thread can make sync calls to the worker, in addition to the comment here?
> > 
> > While I was here, I was also wondering why we use shared memory for the
> discardable memory allocation. This happens when we are allocating memory for
> the decode, which is used in the renderer only right?
> 
> Yeah, I think making a comment that this thread behaves like the raster worker
> threads would be fine.
> 
> I'm less clear on what's going on with the discardable stuff here.

Done. Thanks!

I was curious and stumbled upon a doc which explains the discardable shared
memory design:
https://docs.google.com/document/d/1aNdOF_72_eG2KUM_z9kHdbT_fEupWhaDALaZs5D8I....

[Khushal, 2017-02-28 05:18:02]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-02-28 05:18:02]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2714863005/#ps40001
(title: "comment")

[commit-bot: I haz the power, 2017-02-28 05:18:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 06:26:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488259082117690,
"parent_rev": "e975664dab5f430b66817238898839b333822ac3", "commit_rev":
"19376239fec9373251062c97f250d6f8e98b094e"}

[commit-bot: I haz the power, 2017-02-28 06:27:07]

Description was changed from

==========
content: Add switch to enable checker-imaging.

The change adds a cc switch to enable checker-imaging and plumbs it to
the renderer. Also since during image decode, we end up allocating
shared memory which makes a synchronous call to the IO thread, the
image worker task runner needs to allow waiting.

TBR=piman@chromium.org
BUG=686267
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
content: Add switch to enable checker-imaging.

The change adds a cc switch to enable checker-imaging and plumbs it to
the renderer. Also since during image decode, we end up allocating
shared memory which makes a synchronous call to the IO thread, the
image worker task runner needs to allow waiting.

TBR=piman@chromium.org
BUG=686267
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2714863005
Cr-Commit-Position: refs/heads/master@{#453523}
Committed:
https://chromium.googlesource.com/chromium/src/+/19376239fec9373251062c97f250...
==========

[commit-bot: I haz the power, 2017-02-28 06:27:08]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/19376239fec9373251062c97f250...