Add optimization for CHROMIUM_subscribe_uniform extension.

Add optimization for CHROMIUM_subscribe_uniform extension.

We only pass ValueState updates via IPC if the Gpu Service has
signalled that a Valuebuffer is subscribed to the respective target.

BUG=422978
Committed: https://crrev.com/9b3c1cb1d97de13944e5dd5d36af4aaca6ceebc3
Cr-Commit-Position: refs/heads/master@{#308389}

[orglofch, 2014-12-04 21:54:57]

orglofch@chromium.org changed reviewers:
+ bajones@chromium.org, piman@chromium.org, tsepez@chromium.org

[orglofch, 2014-12-04 21:54:58]

ptal.

[Tom Sepez, 2014-12-04 22:01:58]

Looks like some stray comments in the description above?

Review URL: https://codereview.chromium.org/698053002

Cr-Commit-Position: refs/heads/master@{#302833}

[Tom Sepez, 2014-12-04 22:08:10]

Messages LGTM.

https://codereview.chromium.org/780133002/diff/1/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/780133002/diff/1/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.cc:367: BrowserThread::IO, FROM_HERE,
nit: indentation.

https://codereview.chromium.org/780133002/diff/1/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.h (right):

https://codereview.chromium.org/780133002/diff/1/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.h:90: CONTENT_EXPORT static void
SendUpdateValueStateOnIO(
nit: I'd prefer if this were named SendUpdateValueStateOnIOThread(). Your call.

[bajones, 2014-12-04 23:23:14]

gpu/command_buffer LGTM, with a couple of questions.

https://codereview.chromium.org/780133002/diff/1/gpu/command_buffer/service/c...
File gpu/command_buffer/service/context_group_unittest.cc (right):

https://codereview.chromium.org/780133002/diff/1/gpu/command_buffer/service/c...
gpu/command_buffer/service/context_group_unittest.cc:14: #include
"gpu/command_buffer/service/valuebuffer_manager.h"
Is this include necessary?

https://codereview.chromium.org/780133002/diff/1/gpu/command_buffer/tests/gl_...
File gpu/command_buffer/tests/gl_manager.cc (right):

https://codereview.chromium.org/780133002/diff/1/gpu/command_buffer/tests/gl_...
gpu/command_buffer/tests/gl_manager.cc:30: #include
"gpu/command_buffer/service/valuebuffer_manager.h"
Is this include necessary?

[orglofch, 2014-12-04 23:37:11]

https://codereview.chromium.org/780133002/diff/1/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/780133002/diff/1/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.cc:367: BrowserThread::IO, FROM_HERE,
On 2014/12/04 22:08:10, Tom Sepez wrote:
> nit: indentation.

Done.

https://codereview.chromium.org/780133002/diff/1/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.h (right):

https://codereview.chromium.org/780133002/diff/1/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.h:90: CONTENT_EXPORT static void
SendUpdateValueStateOnIO(
On 2014/12/04 22:08:10, Tom Sepez wrote:
> nit: I'd prefer if this were named SendUpdateValueStateOnIOThread(). Your
call.

Done.

https://codereview.chromium.org/780133002/diff/1/gpu/command_buffer/service/c...
File gpu/command_buffer/service/context_group_unittest.cc (right):

https://codereview.chromium.org/780133002/diff/1/gpu/command_buffer/service/c...
gpu/command_buffer/service/context_group_unittest.cc:14: #include
"gpu/command_buffer/service/valuebuffer_manager.h"
On 2014/12/04 23:23:14, bajones wrote:
> Is this include necessary?

Yep, it's passing a SubscriptionRefset (albeit a NULL one) to the ContextGroup,
which is defined in valuebuffer_manager.h and requires the definition since it's
being passed as a scoped_refptr.

https://codereview.chromium.org/780133002/diff/1/gpu/command_buffer/tests/gl_...
File gpu/command_buffer/tests/gl_manager.cc (right):

https://codereview.chromium.org/780133002/diff/1/gpu/command_buffer/tests/gl_...
gpu/command_buffer/tests/gl_manager.cc:30: #include
"gpu/command_buffer/service/valuebuffer_manager.h"
On 2014/12/04 23:23:14, bajones wrote:
> Is this include necessary?

Same here.

[piman, 2014-12-05 21:04:36]

https://codereview.chromium.org/780133002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/780133002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:912: process_->GetID(),
GL_MOUSE_POSITION_CHROMIUM, state);
I was hoping we could filter here, on the UI thread. Otherwise we'll still post
tasks when no-one is subscribed (common case), that seems wasteful.

https://codereview.chromium.org/780133002/diff/20001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel_manager.h (right):

https://codereview.chromium.org/780133002/diff/20001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel_manager.h:173:
scoped_refptr<gpu::gles2::SubscriptionRefSet> subscription_ref_set_;
Do we want a global one for all channels, or one per channel? If only one
channel subscribes, we may not want to send the IPC on all of them.

https://codereview.chromium.org/780133002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/context_group.h (right):

https://codereview.chromium.org/780133002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/context_group.h:25: }
Are those 5 lines leftover from a previous version?
It would be a layering violation. Please remove.

[orglofch, 2014-12-06 21:49:04]

https://codereview.chromium.org/780133002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/780133002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:912: process_->GetID(),
GL_MOUSE_POSITION_CHROMIUM, state);
On 2014/12/05 21:04:36, piman (Very slow to review) wrote:
> I was hoping we could filter here, on the UI thread. Otherwise we'll still
post
> tasks when no-one is subscribed (common case), that seems wasteful.

Ah I had thought the main concern was IPC overhead. Done.

https://codereview.chromium.org/780133002/diff/20001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel_manager.h (right):

https://codereview.chromium.org/780133002/diff/20001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel_manager.h:173:
scoped_refptr<gpu::gles2::SubscriptionRefSet> subscription_ref_set_;
On 2014/12/05 21:04:36, piman (Very slow to review) wrote:
> Do we want a global one for all channels, or one per channel? If only one
> channel subscribes, we may not want to send the IPC on all of them.

Done.

https://codereview.chromium.org/780133002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/context_group.h (right):

https://codereview.chromium.org/780133002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/context_group.h:25: }
On 2014/12/05 21:04:36, piman (Very slow to review) wrote:
> Are those 5 lines leftover from a previous version?
> It would be a layering violation. Please remove.

Yep, Done.

[piman, 2014-12-10 06:57:11]

https://codereview.chromium.org/780133002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/780133002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:2356: if
(subscription_set_.find(GL_MOUSE_POSITION_CHROMIUM)
s/GL_MOUSE_POSITION_CHROMIUM/target/ in this method?

https://codereview.chromium.org/780133002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:2362:
process_->GetID(), GL_MOUSE_POSITION_CHROMIUM, state));
Actually, I'm realizing, this doesn't make much sense to have a per-process map
that has per-RWH values.

The event is relative to the renderer. You can have different renderers in
different windows, and moving the mouse between those renderers would send
spurious values to the other ones.

That's kind of why you have a strange map lookup in GpuProcessHostUIShim.

[orglofch, 2014-12-10 20:41:15]

Patchset #4 (id:60001) has been deleted

[orglofch, 2014-12-10 21:49:52]

Patchset #4 (id:80001) has been deleted

[orglofch, 2014-12-10 21:54:49]

https://codereview.chromium.org/780133002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/780133002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:2356: if
(subscription_set_.find(GL_MOUSE_POSITION_CHROMIUM)
On 2014/12/10 06:57:10, piman (Very slow to review) wrote:
> s/GL_MOUSE_POSITION_CHROMIUM/target/ in this method?

Done.

https://codereview.chromium.org/780133002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:2362:
process_->GetID(), GL_MOUSE_POSITION_CHROMIUM, state));
On 2014/12/10 06:57:10, piman (Very slow to review) wrote:
> Actually, I'm realizing, this doesn't make much sense to have a per-process
map
> that has per-RWH values.
> 
> The event is relative to the renderer. You can have different renderers in
> different windows, and moving the mouse between those renderers would send
> spurious values to the other ones.
> 
> That's kind of why you have a strange map lookup in GpuProcessHostUIShim.

Done. Moved to RenderProcessHost.

[piman, 2014-12-12 01:47:38]

LGTM because I don't want to hold this up, but see comment

https://codereview.chromium.org/780133002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/780133002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:904:
process_->SendUpdateValueState(GL_MOUSE_POSITION_CHROMIUM, state);
So, you still have the problem that the RPH receives updates from separate RWH
that will end up impacting all RenderWidgets associated with that process (move
the mouse in renderer A, you get updates to values in renderer B).

Really the way to solve that is to have each RW request updates for its own
routing id, and keeping those separate (i.e. not merge the value state map
between all command buffers in a channel, or maybe tag the subscription with the
RW routing id?).

I'm fine if it's a follow up, but can you add a TODO / file a bug?

[orglofch, 2014-12-12 02:17:58]

https://codereview.chromium.org/780133002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/780133002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:904:
process_->SendUpdateValueState(GL_MOUSE_POSITION_CHROMIUM, state);
On 2014/12/12 01:47:38, piman (Very slow to review) wrote:
> So, you still have the problem that the RPH receives updates from separate RWH
> that will end up impacting all RenderWidgets associated with that process
(move
> the mouse in renderer A, you get updates to values in renderer B).
> 
> Really the way to solve that is to have each RW request updates for its own
> routing id, and keeping those separate (i.e. not merge the value state map
> between all command buffers in a channel, or maybe tag the subscription with
the
> RW routing id?).
> 
> I'm fine if it's a follow up, but can you add a TODO / file a bug?

I guess I'm missing an understanding of what a RW maps to physically. For
example, can we have multiple RWH tiled in a single window such that they're
functionally different windows? If so does the GL context know which RW it's in
so it can communicate which pending_state to use? Otherwise we should simply use
the most recent value_states.

Is there something in the Gpu Process which maps to a RWH the same way RPH maps
to GpuChannel?

I've added the TODO and I'll try to follow up this weekend.

[piman, 2014-12-12 02:41:40]

https://codereview.chromium.org/780133002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/780133002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:904:
process_->SendUpdateValueState(GL_MOUSE_POSITION_CHROMIUM, state);
On 2014/12/12 02:17:58, orglofch wrote:
> On 2014/12/12 01:47:38, piman (Very slow to review) wrote:
> > So, you still have the problem that the RPH receives updates from separate
RWH
> > that will end up impacting all RenderWidgets associated with that process
> (move
> > the mouse in renderer A, you get updates to values in renderer B).
> > 
> > Really the way to solve that is to have each RW request updates for its own
> > routing id, and keeping those separate (i.e. not merge the value state map
> > between all command buffers in a channel, or maybe tag the subscription with
> the
> > RW routing id?).
> > 
> > I'm fine if it's a follow up, but can you add a TODO / file a bug?
> 
> I guess I'm missing an understanding of what a RW maps to physically. For
> example, can we have multiple RWH tiled in a single window such that they're
> functionally different windows? If so does the GL context know which RW it's
in
> so it can communicate which pending_state to use? Otherwise we should simply
use
> the most recent value_states.
> 
> Is there something in the Gpu Process which maps to a RWH the same way RPH
maps
> to GpuChannel?
> 
> I've added the TODO and I'll try to follow up this weekend.

a RW is the contents of a tab. You can have several RW per renderer process
(RPH), e.g. if they're from the same origin. Those tabs might be in different
windows.

The problem about which RW the context corresponds to is that it can change. For
example in JavaScript you can move the canvas element between related documents
in the same origin (e.g. opened by window.open), or, actually, remove it from
all DOMs while still having the context, i.e. the context doesn't correspond to
any RW at all.

So even at the API level in webgl, "subscribing" to the mouse position doesn't
completely make sense unless you give a document the mouse position is relative
to.
Or you use an absolute mouse position (i.e. screen-relative instead of
page-relative), in which case the value would be the same regardless of which RW
it comes from (that's what I originally thought the API would give you).

[orglofch, 2014-12-12 02:58:55]

https://codereview.chromium.org/780133002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/780133002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:904:
process_->SendUpdateValueState(GL_MOUSE_POSITION_CHROMIUM, state);
On 2014/12/12 02:41:40, piman (Very slow to review) wrote:
> On 2014/12/12 02:17:58, orglofch wrote:
> > On 2014/12/12 01:47:38, piman (Very slow to review) wrote:
> > > So, you still have the problem that the RPH receives updates from separate
> RWH
> > > that will end up impacting all RenderWidgets associated with that process
> > (move
> > > the mouse in renderer A, you get updates to values in renderer B).
> > > 
> > > Really the way to solve that is to have each RW request updates for its
own
> > > routing id, and keeping those separate (i.e. not merge the value state map
> > > between all command buffers in a channel, or maybe tag the subscription
with
> > the
> > > RW routing id?).
> > > 
> > > I'm fine if it's a follow up, but can you add a TODO / file a bug?
> > 
> > I guess I'm missing an understanding of what a RW maps to physically. For
> > example, can we have multiple RWH tiled in a single window such that they're
> > functionally different windows? If so does the GL context know which RW it's
> in
> > so it can communicate which pending_state to use? Otherwise we should simply
> use
> > the most recent value_states.
> > 
> > Is there something in the Gpu Process which maps to a RWH the same way RPH
> maps
> > to GpuChannel?
> > 
> > I've added the TODO and I'll try to follow up this weekend.
> 
> a RW is the contents of a tab. You can have several RW per renderer process
> (RPH), e.g. if they're from the same origin. Those tabs might be in different
> windows.
> 
> The problem about which RW the context corresponds to is that it can change.
For
> example in JavaScript you can move the canvas element between related
documents
> in the same origin (e.g. opened by window.open), or, actually, remove it from
> all DOMs while still having the context, i.e. the context doesn't correspond
to
> any RW at all.
> 
> So even at the API level in webgl, "subscribing" to the mouse position doesn't
> completely make sense unless you give a document the mouse position is
relative
> to.
> Or you use an absolute mouse position (i.e. screen-relative instead of
> page-relative), in which case the value would be the same regardless of which
RW
> it comes from (that's what I originally thought the API would give you).

Ok I see what you're saying, I was confused because this didn't seem to make
sense with regards to the current API. 

Seems like this change is more substantial than I had originally thought so I'll
create a tracking bug.

[orglofch, 2014-12-15 17:45:43]

The CQ bit was checked by orglofch@chromium.org

[commit-bot: I haz the power, 2014-12-15 17:46:19]

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

[commit-bot: I haz the power, 2014-12-15 19:27:41]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2014-12-15 19:28:42]

Patchset 5 (id:??) landed as
https://crrev.com/9b3c1cb1d97de13944e5dd5d36af4aaca6ceebc3
Cr-Commit-Position: refs/heads/master@{#308389}