cc: Remove racy initialization code in cc/base/switches.cc.

cc: Remove racy initialization code in cc/base/switches.cc.

This moves all lazy initialization code out of cc/base/switches.cc. Code
that needs the value of these switches in a critical path should not rely
on lazy initialization in cc/switches.cc but instead handle this on its own.

Thread-safe lazy initialization code of impl-side painting status has
been added to WebLayerImpl.

This also removes the cc::switches::IsGPURasterizationEnabled() function,
which for consistency should not exist unless the logic to determine the
value is more complicated than HasSwitch(kEnableGPURasterization). This
function was never used in performance sensitive code so lazy initialization
was never necessary.

BUG=330937
TBR=jamesr

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

[reveman, 2013-12-28 14:47:17]

+sohanjg

[tomhudson, 2013-12-30 11:20:00]

What triggers the potential race, being called from multiple raster worker
threads?

[reveman, 2013-12-31 00:41:04]

On 2013/12/30 11:20:00, tomhudson wrote:
> What triggers the potential race, being called from multiple raster worker
> threads?

Being called from multiple compositor threads in the same renderer process.

[danakj1, 2013-12-31 08:48:01]

On Dec 30, 2013 7:41 PM, <reveman@chromium.org> wrote:
>
> On 2013/12/30 11:20:00, tomhudson wrote:
>>
>> What triggers the potential race, being called from multiple raster
worker
>> threads?
>
>
> Being called from multiple compositor threads in the same renderer
process.

I thought there is one compositor thread shared among all compositor
instances in the process?

>
> https://codereview.chromium.org/122063002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tomhudson, 2013-12-31 10:02:33]

On 2013/12/31 08:48:01, danakj1 wrote:
> On Dec 30, 2013 7:41 PM, <mailto:reveman@chromium.org> wrote:
> >
> > On 2013/12/30 11:20:00, tomhudson wrote:
> >>
> >> What triggers the potential race, being called from multiple raster
> worker
> >> threads?
> >
> >
> > Being called from multiple compositor threads in the same renderer
> process.
> 
> I thought there is one compositor thread shared among all compositor
> instances in the process?

Yes, that doesn't sound like an actual threat?

[reveman, 2013-12-31 20:33:40]

On 2013/12/31 08:48:01, danakj1 wrote:
> On Dec 30, 2013 7:41 PM, <mailto:reveman@chromium.org> wrote:
> >
> > On 2013/12/30 11:20:00, tomhudson wrote:
> >>
> >> What triggers the potential race, being called from multiple raster
> worker
> >> threads?
> >
> >
> > Being called from multiple compositor threads in the same renderer
> process.
> 
> I thought there is one compositor thread shared among all compositor
> instances in the process?

Sorry, my previous comment was incorrect. Problem is that it's used by multiple
compositors. Both from the main threads and the compositor thread.

IMO, it seems very fragile to assume that cc/base/switches.h is never used from
more than one thread.

> 
> >
> > https://codereview.chromium.org/122063002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj1, 2014-01-01 02:38:53]

On Dec 31, 2013 3:33 PM, <reveman@chromium.org> wrote:
>
> On 2013/12/31 08:48:01, danakj1 wrote:
>>
>> On Dec 30, 2013 7:41 PM, <mailto:reveman@chromium.org> wrote:
>> >
>> > On 2013/12/30 11:20:00, tomhudson wrote:
>> >>
>> >> What triggers the potential race, being called from multiple raster
>> worker
>> >> threads?
>> >
>> >
>> > Being called from multiple compositor threads in the same renderer
>> process.
>
>
>> I thought there is one compositor thread shared among all compositor
>> instances in the process?
>
>
> Sorry, my previous comment was incorrect. Problem is that it's used by
multiple
> compositors. Both from the main threads and the compositor thread.
>
> IMO, it seems very fragile to assume that cc/base/switches.h is never
used from
> more than one thread.

Actually I don't think were expecting to check command line switches
anywhere in cc/.  Those switches should be given to the compositor via its
public API such as LayerTreeSettings. CommandLine is an awful non-const bag
of global variables and checking it every where has led to some very
confusing code elsewhere in chrome. I'd like to avoid that in cc. I think
of CommandLine checking as similar to #ifdef'd code.

>
>> >
>> > https://codereview.chromium.org/122063002/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
>
> https://codereview.chromium.org/122063002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-01-01 23:01:43]

On 2014/01/01 02:38:53, danakj1 wrote:
> On Dec 31, 2013 3:33 PM, <mailto:reveman@chromium.org> wrote:
> >
> > On 2013/12/31 08:48:01, danakj1 wrote:
> >>
> >> On Dec 30, 2013 7:41 PM, <mailto:reveman@chromium.org> wrote:
> >> >
> >> > On 2013/12/30 11:20:00, tomhudson wrote:
> >> >>
> >> >> What triggers the potential race, being called from multiple raster
> >> worker
> >> >> threads?
> >> >
> >> >
> >> > Being called from multiple compositor threads in the same renderer
> >> process.
> >
> >
> >> I thought there is one compositor thread shared among all compositor
> >> instances in the process?
> >
> >
> > Sorry, my previous comment was incorrect. Problem is that it's used by
> multiple
> > compositors. Both from the main threads and the compositor thread.
> >
> > IMO, it seems very fragile to assume that cc/base/switches.h is never
> used from
> > more than one thread.
> 
> Actually I don't think were expecting to check command line switches
> anywhere in cc/.  Those switches should be given to the compositor via its
> public API such as LayerTreeSettings. CommandLine is an awful non-const bag
> of global variables and checking it every where has led to some very
> confusing code elsewhere in chrome. I'd like to avoid that in cc. I think
> of CommandLine checking as similar to #ifdef'd code.

I agree, not checking any command line switches directly in cc would be best and
I think this patch lines up with that idea. Not sure how to deal with the
impl-side painting check done by the compositor bindings but maybe we can leave
that as this patch until impl-side is used everywhere and this is no longer
needed.

What prompted me to solve this problem is that
https://codereview.chromium.org/73923003/ is almost ready to land and it
requires the NumRasterThreads setting to become process wide rather than per
layer tree. The addition of some public API that allows this to be set once at
process startup is probably the best solution here.

> 
> >
> >> >
> >> > https://codereview.chromium.org/122063002/
> >
> >
> >> To unsubscribe from this group and stop receiving emails from it, send an
> >
> > email
> >>
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >
> >
> > https://codereview.chromium.org/122063002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2014-01-02 18:12:07]

lgtm

I think this race is benign in that multiple threads would initialize that
static to the same value, but it does seem better to be *actually* thread-safe
rather than just logically so.

[reveman, 2014-01-02 19:25:32]

On 2014/01/02 18:12:07, enne wrote:
> lgtm
> 
> I think this race is benign in that multiple threads would initialize that
> static to the same value, but it does seem better to be *actually* thread-safe
> rather than just logically so.

Yes, it's benign. If there is such a thing as a benign data race at C++ code
level (https://www.usenix.org/legacy/event/hotpar11/tech/final_files/Boehm.pdf).

[commit-bot: I haz the power, 2014-01-03 02:18:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/122063002/1

[commit-bot: I haz the power, 2014-01-03 02:33:34]

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

[commit-bot: I haz the power, 2014-01-03 03:05:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/122063002/1

[commit-bot: I haz the power, 2014-01-03 03:29:50]

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

[commit-bot: I haz the power, 2014-01-03 06:30:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/122063002/1

[commit-bot: I haz the power, 2014-01-03 07:15:10]

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

[commit-bot: I haz the power, 2014-01-03 17:11:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/122063002/1

[commit-bot: I haz the power, 2014-01-03 17:58:58]

Retried try job too often on mac_rel for step(s) chromedriver2_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-03 19:57:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/122063002/1

[commit-bot: I haz the power, 2014-01-03 20:54:35]

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

[danakj, 2014-01-06 18:13:24]

https://codereview.chromium.org/122063002/diff/1/webkit/renderer/compositor_b...
File webkit/renderer/compositor_bindings/web_layer_impl.h (right):

https://codereview.chromium.org/122063002/diff/1/webkit/renderer/compositor_b...
webkit/renderer/compositor_bindings/web_layer_impl.h:55: static bool
usingPictureLayer();
nit: since this isn't overriding a Blink method, it should be
UsingPictureLayer().

[reveman, 2014-01-06 18:32:28]

https://codereview.chromium.org/122063002/diff/1/webkit/renderer/compositor_b...
File webkit/renderer/compositor_bindings/web_layer_impl.h (right):

https://codereview.chromium.org/122063002/diff/1/webkit/renderer/compositor_b...
webkit/renderer/compositor_bindings/web_layer_impl.h:55: static bool
usingPictureLayer();
On 2014/01/06 18:13:25, danakj wrote:
> nit: since this isn't overriding a Blink method, it should be
> UsingPictureLayer().

Done.

[commit-bot: I haz the power, 2014-01-06 23:27:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/122063002/800001

[commit-bot: I haz the power, 2014-01-07 01:01:41]

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

[commit-bot: I haz the power, 2014-01-07 21:10:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/122063002/1080001

[commit-bot: I haz the power, 2014-01-07 23:34:52]

Change committed as 243432