Make ShaderTranslatorCache thread safe

Make ShaderTranslatorCache per thread refcounted

After crbug.com/332146 is fixed, in Android WebView, GLES2Decoder can be
used on two threads. Make sure a ShaderTranslatorCache instance is only
used on a single thread by refcounting an instance between all decoders.

Also protect ShInitialize by using the thread safety guarantees of
LazyInstance.

For tests, a new instance of ShaderTranslatorCache is passed for each
ContextGroup.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261427

[boliu, 2014-03-29 00:05:45]

Hmm....any potential perf impact? Maybe we can only lock the initialization and
make the cache per thread?

[boliu, 2014-03-29 00:13:32]

On 2014/03/29 00:05:45, boliu wrote:
> Hmm....any potential perf impact? Maybe we can only lock the initialization
and
> make the cache per thread?

Oh wait, need to make ShaderTranslator thread safe too, I think? Maybe making
the cache class thread local is better way to go?

[no sievers, 2014-03-31 20:27:38]

On 2014/03/29 00:13:32, boliu wrote:
> On 2014/03/29 00:05:45, boliu wrote:
> > Hmm....any potential perf impact? Maybe we can only lock the initialization
> and
> > make the cache per thread?
> 
> Oh wait, need to make ShaderTranslator thread safe too, I think? Maybe making
> the cache class thread local is better way to go?

What we can do is make ShaderTranslatorCache not be a Singleton.

Instead we have once cache per service thread. It can be owned by
GpuChannelManager and then passed around (to ContextGroup/ShaderManager) similar
to MailboxManager.

For in-process, maybe InProcessCommandBuffer::Service can provide this?

[no sievers, 2014-03-31 20:30:19]

On 2014/03/31 20:27:38, sievers wrote:
> On 2014/03/29 00:13:32, boliu wrote:
> > On 2014/03/29 00:05:45, boliu wrote:
> > > Hmm....any potential perf impact? Maybe we can only lock the
initialization
> > and
> > > make the cache per thread?
> > 
> > Oh wait, need to make ShaderTranslator thread safe too, I think? Maybe
making
> > the cache class thread local is better way to go?
> 
> What we can do is make ShaderTranslatorCache not be a Singleton.
> 
> Instead we have once cache per service thread. It can be owned by
> GpuChannelManager and then passed around (to ContextGroup/ShaderManager)
similar
> to MailboxManager.
> 
> For in-process, maybe InProcessCommandBuffer::Service can provide this?

Actually what's the cost of just having separate translators per ContextGroup?

[boliu, 2014-03-31 20:32:37]

On 2014/03/31 20:30:19, sievers wrote:
> Actually what's the cost of just having separate translators per ContextGroup?

I haz no idea. I was hoping you'd know?

[boliu, 2014-04-02 02:00:12]

Refcounting the cache and making the cache per thread, kinda big-ish patch now

[boliu, 2014-04-02 02:00:55]

Oh wait, hold off review, still missing actually protecting
InitializeShaderTranslator.

[boliu, 2014-04-02 03:15:46]

Ok, now ShInitialize should be safe too. PTAL.

[no sievers, 2014-04-02 18:30:19]

https://codereview.chromium.org/217813004/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/217813004/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/in_process_command_buffer.h:133: virtual
gles2::ShaderTranslatorCache* shader_translator_cache() = 0;
nit: return scoped_refptr<>

https://codereview.chromium.org/217813004/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/shader_translator_cache.h (right):

https://codereview.chromium.org/217813004/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_translator_cache.h:19: // This class can is
not thread safe and can only be created and destroyed
nit: 'can is' -> 'is'

https://codereview.chromium.org/217813004/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_translator_cache.h:20: // on a single thread.
But it is safe to use two indepdent instances on two
nit: typo 'indepdent'

https://codereview.chromium.org/217813004/diff/40001/mojo/services/gles2/comm...
File mojo/services/gles2/command_buffer_impl.cc (right):

https://codereview.chromium.org/217813004/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.cc:99: new
gpu::gles2::ShaderTranslatorCache,
We don't actually have to create one per decoder, but can at least store it in
the ContextGroup, or even better maybe ShaderManager (owned by ContextGroup).

piman@ might further know if there is a good place for mojo to share this more
widely, in case creation cost in terms of time and memory for the translator is
still an issue nowadays.

[boliu, 2014-04-02 21:10:12]

+piman for opinion on whether it's fine for perf to make ShaderTranslatorCache
per-ContextGroup rather than per-thread. Per-ContextGroup allows removing a
bunch of plumbing in this CL.

[no sievers, 2014-04-02 21:27:21]

On 2014/04/02 21:10:12, boliu wrote:
> +piman for opinion on whether it's fine for perf to make ShaderTranslatorCache
> per-ContextGroup rather than per-thread. Per-ContextGroup allows removing a
> bunch of plumbing in this CL.

The bug with the ShaderTranslator initializing slowly and using 700k (?) per
instance is very old.
If this has been improved in Angle already, things would get a lot easier (we
just make this per ContextGroup).

[piman, 2014-04-02 21:58:27]

On 2014/04/02 21:10:12, boliu wrote:
> +piman for opinion on whether it's fine for perf to make ShaderTranslatorCache
> per-ContextGroup rather than per-thread. Per-ContextGroup allows removing a
> bunch of plumbing in this CL.

I'm kinda worried about the memory impact. It doesn't seem worth it to save just
a parameter in a handful of functions.

[piman, 2014-04-02 21:58:33]

https://codereview.chromium.org/217813004/diff/40001/mojo/services/gles2/comm...
File mojo/services/gles2/command_buffer_impl.cc (right):

https://codereview.chromium.org/217813004/diff/40001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.cc:99: new
gpu::gles2::ShaderTranslatorCache,
On 2014/04/02 18:30:19, sievers wrote:
> We don't actually have to create one per decoder, but can at least store it in
> the ContextGroup, or even better maybe ShaderManager (owned by ContextGroup).
> 
> piman@ might further know if there is a good place for mojo to share this more
> widely, in case creation cost in terms of time and memory for the translator
is
> still an issue nowadays.

Currently, mojo support is still in its infancy. We don't support ContextGroups
yet (see TODOs above).
It's fine to create one per ContextGroup in mojo for now, just add a TODO(piman)
about it only needing to be per-thread?

[boliu, 2014-04-02 22:09:59]

Ok, added the TODO in mojo. And previous comments should all be addressed too.

PTAL!

[no sievers, 2014-04-02 22:48:00]

LGTM, thanks for fixing this!

[boliu, 2014-04-02 23:00:06]

+viettrungluu for mojo owners

[viettrungluu, 2014-04-02 23:03:42]

mojo OWNERS lgtm

[boliu, 2014-04-02 23:05:50]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-04-02 23:09:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/217813004/100001

[boliu, 2014-04-02 23:22:20]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2014-04-03 00:13:33]

Heh, missed a compile error even though I built All on linux. Turns out
linux_gpu builds with internal_gles2_conform_tests set to 1.

Then realized the conformances tests are segfaulting. The issue is in the test
harness, there is one decoder which "owns" the context group, which "owns" the
translator cache. Decoder destroys group in Destroy method, but the translators
are only destroyed in the decoder destructor. Bam, use after free. Fixed by
releasing the translators in Destroy as well.

sievers: should review diff between PS6 and PS8.

[no sievers, 2014-04-03 00:45:37]

On 2014/04/03 00:13:33, boliu wrote:
> Heh, missed a compile error even though I built All on linux. Turns out
> linux_gpu builds with internal_gles2_conform_tests set to 1.
> 
> Then realized the conformances tests are segfaulting. The issue is in the test
> harness, there is one decoder which "owns" the context group, which "owns" the
> translator cache. Decoder destroys group in Destroy method, but the
translators
> are only destroyed in the decoder destructor. Bam, use after free. Fixed by
> releasing the translators in Destroy as well.
> 
> sievers: should review diff between PS6 and PS8.

how about making the cache unregister itself as destruction observer for
translators when it gets deleted?

[boliu, 2014-04-03 01:06:16]

On 2014/04/03 00:45:37, sievers wrote:
> how about making the cache unregister itself as destruction observer for
> translators when it gets deleted?

sievers agreed offline that ensuring the cache must outlive all translators is
better way to go :)

[no sievers, 2014-04-03 01:07:53]

lgtm

[boliu, 2014-04-03 01:08:32]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-04-03 01:10:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/217813004/130001

[commit-bot: I haz the power, 2014-04-03 02:09:39]

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

[commit-bot: I haz the power, 2014-04-03 02:09:40]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[boliu, 2014-04-03 04:32:35]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-04-03 04:35:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/217813004/150001

[piman, 2014-04-03 05:55:09]

lgtm

https://codereview.chromium.org/217813004/diff/150001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/217813004/diff/150001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3419: // Need to release these
before releasing |group_| which may owns the
nit: "may owns" -> either "owns" or "may own"

[commit-bot: I haz the power, 2014-04-03 10:12:25]

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

[commit-bot: I haz the power, 2014-04-03 10:12:25]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[boliu, 2014-04-03 12:01:55]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-04-03 12:03:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/217813004/170001

[commit-bot: I haz the power, 2014-04-03 12:08:32]

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

[commit-bot: I haz the power, 2014-04-03 12:08:32]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[boliu, 2014-04-03 12:09:52]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-04-03 12:10:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/217813004/170001

[commit-bot: I haz the power, 2014-04-03 12:55:08]

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

[commit-bot: I haz the power, 2014-04-03 12:55:09]

Retried try job too often on ios_rel_device_ninja for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[boliu, 2014-04-03 13:07:38]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-04-03 13:07:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/217813004/170001

[commit-bot: I haz the power, 2014-04-03 15:37:13]

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

[commit-bot: I haz the power, 2014-04-03 15:37:14]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[boliu, 2014-04-03 15:38:31]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-04-03 15:39:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/217813004/170001

[commit-bot: I haz the power, 2014-04-03 15:58:15]

Change committed as 261427