Bug 88384 – [chromium] Certain settings in CCSettings could be global

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

Certain settings (e.g., enabling per-tile painting) could be made global. To accomplish this, CCSettings should be moved to its own file, the current CCSettings should be renamed CCLayerTreeSettings and retain only those settings that may vary from tab to tab. This will depend on a chromium patch to prepare for the API breakage.

[Nat Duca]

+1

Created attachment 145911 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Comment on attachment 145911 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145911&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:53
> +static const IntSize defaultTileSizeDefault = IntSize(256, 256);
> +static IntSize s_defaultTileSize = defaultTileSizeDefault;

This smells like a static initializer - is it?  If so that's a no-no.  Perhaps we could use a lazy singleton pattern instead?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3483
> +        CCSettings::setAcceleratedPaintingEnabled(page()->settings()->acceleratedDrawingEnabled());

I think this is (slightly) wrong - we shouldn't have every view clobbering the global settings.  Since this is exposed through WebKit API the embedder (chromium) should be making these decisions based on command line flags or whatnot before we even get to here.

Created attachment 146086 [details]
Patch

(In reply to comment #4)
> (From update of attachment 145911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145911&action=review
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:53
> > +static const IntSize defaultTileSizeDefault = IntSize(256, 256);
> > +static IntSize s_defaultTileSize = defaultTileSizeDefault;
>
> This smells like a static initializer - is it?  If so that's a no-no.  Perhaps we could use a lazy singleton pattern instead?
*blush* Fixed.
>
> > Source/WebKit/chromium/src/WebViewImpl.cpp:3483
> > +        CCSettings::setAcceleratedPaintingEnabled(page()->settings()->acceleratedDrawingEnabled());
>
> I think this is (slightly) wrong - we shouldn't have every view clobbering the global settings.  Since this is exposed through WebKit API the embedder (chromium) should be making these decisions based on command line flags or whatnot before we even get to here.
Fixed.

[James Robinson]

Comment on attachment 146086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146086&action=review

Getting there!

> Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:113
> +    MutexLocker lock(m_mutex);

this is the suck :(  we query settings all the time, we shouldn't be taking a mutex hit each one!

i would define the interface as saying that the embedder is responsible for initializing these settings before the compositor starts running and they have to take care of the appropriate happens-before relationships.  one easy way to do this is to set everything before calling WebCompositor::initialize().  then you don't need any internal locking

> Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:199
> +    if (s_instance)

looking at it more this seems a bit silly.  to avoid static initializers, we just need to avoid having any non-POD globals. the only settings we have are bools, ints, and IntSizes.  if we stored the IntSize as pairs of ints and converted them to IntSize at call time then they could all live as statics without any issue

> Source/WebKit/chromium/public/WebCompositor.h:55
> +    WEBKIT_EXPORT static bool acceleratedPaintingEnabled();

you need to document the rules for these settings. when can they be set? how long are they valid for? what is their relationship with initialize() / shutdown() ?

why do we have getters on these? does anyone use the WebCompositor interface for anything other than setting?

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:51
> +    CCSettings::initialize();
>      WebCompositorImpl::initialize(implThread);

it's unfortunate to require both

[Nat Duca]

(In reply to comment #6)
> > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:113
> > +    MutexLocker lock(m_mutex);
> i would define the interface as saying that the embedder is responsible for initializing these settings before the compositor starts running and they have to take care of the appropriate happens-before relationships.  one easy way to do this is to set everything before calling WebCompositor::initialize().  then you don't need any internal locking

This is where having the public methods that affect CCSettings being on WebCompositor would be helpful.... that way you can basically say "these can only be set before you call the initialize function."

(In reply to comment #6)
> (From update of attachment 146086 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146086&action=review
> 
> Getting there!
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:113
> > +    MutexLocker lock(m_mutex);
> 
> this is the suck :(  we query settings all the time, we shouldn't be taking a mutex hit each one!
> 
> i would define the interface as saying that the embedder is responsible for initializing these settings before the compositor starts running and they have to take care of the appropriate happens-before relationships.  one easy way to do this is to set everything before calling WebCompositor::initialize().  then you don't need any internal locking
>

Looking at render_thread_impl.cc, we lazily initialize the WebCompositor in several spots, but we only have the preferences we need in order to initialize the global settings in one of them (when we create a view). This approach would require us to guarantee that the first time we try to initialize the WebCompositor is when we create a view which, given the lazy-initialization approach, is not currently not the case. We _can_ guarantee that the settings are in place before the creation of the first view. Would that be ok?


> > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:199
> > +    if (s_instance)
> 
> looking at it more this seems a bit silly.  to avoid static initializers, we just need to avoid having any non-POD globals. the only settings we have are bools, ints, and IntSizes.  if we stored the IntSize as pairs of ints and converted them to IntSize at call time then they could all live as statics without any issue
Sounds good. I will do this.

> 
> > Source/WebKit/chromium/public/WebCompositor.h:55
> > +    WEBKIT_EXPORT static bool acceleratedPaintingEnabled();
> 
> you need to document the rules for these settings. when can they be set? how long are they valid for? what is their relationship with initialize() / shutdown() ?

Initialize and shutdown will be going away. The rules will be -- you can only start using the getters once you're done with the setters. And you can only use the setters on the main thread. Should I adjust the code to enforce this?

> 
> why do we have getters on these? does anyone use the WebCompositor interface for anything other than setting?

Will remove these.

> 
> > Source/WebKit/chromium/src/WebCompositorImpl.cpp:51
> > +    CCSettings::initialize();
> >      WebCompositorImpl::initialize(implThread);
> 
> it's unfortunate to require both

CCSettings::initialize() should be going away.

Created attachment 146191 [details]
Patch

(In reply to comment #6)
> (From update of attachment 146086 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146086&action=review
>
> Getting there!
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:113
> > +    MutexLocker lock(m_mutex);
>
> this is the suck :(  we query settings all the time, we shouldn't be taking a mutex hit each one!
>
> i would define the interface as saying that the embedder is responsible for initializing these settings before the compositor starts running and they have to take care of the appropriate happens-before relationships.  one easy way to do this is to set everything before calling WebCompositor::initialize().  then you don't need any internal locking
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:199
> > +    if (s_instance)
>
> looking at it more this seems a bit silly.  to avoid static initializers, we just need to avoid having any non-POD globals. the only settings we have are bools, ints, and IntSizes.  if we stored the IntSize as pairs of ints and converted them to IntSize at call time then they could all live as statics without any issue
Fixed.
>
> > Source/WebKit/chromium/public/WebCompositor.h:55
> > +    WEBKIT_EXPORT static bool acceleratedPaintingEnabled();
>
> you need to document the rules for these settings. when can they be set? how long are they valid for? what is their relationship with initialize() / shutdown() ?
Done.
>
> why do we have getters on these? does anyone use the WebCompositor interface for anything other than setting?
Removed.
>
> > Source/WebKit/chromium/src/WebCompositorImpl.cpp:51
> > +    CCSettings::initialize();
> >      WebCompositorImpl::initialize(implThread);
>
> it's unfortunate to require both
Removed.

[WebKit Review Bot]

Comment on attachment 146191 [details]
Patch

Attachment 146191 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12919131

[James Robinson]

Comment on attachment 146191 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146191&action=review

Still have one static initializer and this doesn't appear to quite compile, but otherwise I think this is looking good.  R- for now since we need another round but it's close.

> Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:45
> +static size_t s_maxPartialTextureUpdatesDefault = std::numeric_limits<size_t>::max();

hey guess what this is!  sadly, it's a static initializer. If you're on linux here's a way to test things like this:

$ cat test.cc
#include <limits>

int mybar = std::numeric_limits<int>::max();

int main() {
    return 0;
}
jamesr@jamesr:~$ g++ test.cc -o test -O2
jamesr@jamesr:~$ /path/to/chromium/checkout/src/tools/linux/dump-static-initializers.py test
mybar (initializer offset 0x4005d0 size 0xb)
  mybar

Found 1 static initializers in 1 files.


works with clang as well.

I'd suggest just a really big number or perhaps the slightly uglier static_cast<size_t>(-1);

> Source/WebCore/platform/graphics/chromium/cc/CCSettings.h:35
> +    // Must be called on the main thread before WebCompostor::initialize.

Typo: Compostor.  We aren't turning layers into manure here :D

I think having one comment describing this would make more sense - and I'd also group all the setters together at the bottom since most people looking at this 

It's a bit of a (conceptual) layering violation to have this refer to WebCompositor, that's a WebKit client API. I think the comment in WebCompositor.h is sufficient - alternately you could describe the invariants here in terms of other CC concepts

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:639
> +        // For these tests, we will enable threaded animations.
> +        WebCompositor::setAcceleratedAnimationEnabled(true);

For all of these tests?  Is this a change in behavior, or just shuffling logic around?

> Source/WebKit/chromium/tests/CCTestCommon.h:33
> +// If you have a test that modifies or uses global settings, keep an instance
> +// of this class to ensure that you start and end with a clean slate.

Could we instead have WebCompositor::shutdown() reset the settings?  Tests that depend on these things should hopefully be calling ::initialize()/::shutdown() already - right?

Created attachment 146558 [details]
Patch

(In reply to comment #11)
> (From update of attachment 146191 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146191&action=review
>
> Still have one static initializer and this doesn't appear to quite compile, but otherwise I think this is looking good.  R- for now since we need another round but it's close.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:45
> > +static size_t s_maxPartialTextureUpdatesDefault = std::numeric_limits<size_t>::max();
>
> hey guess what this is!  sadly, it's a static initializer. If you're on linux here's a way to test things like this:
>
> $ cat test.cc
> #include <limits>
>
> int mybar = std::numeric_limits<int>::max();
>
> int main() {
>     return 0;
> }
> jamesr@jamesr:~$ g++ test.cc -o test -O2
> jamesr@jamesr:~$ /path/to/chromium/checkout/src/tools/linux/dump-static-initializers.py test
> mybar (initializer offset 0x4005d0 size 0xb)
>   mybar
>
> Found 1 static initializers in 1 files.
>
>
> works with clang as well.
>
> I'd suggest just a really big number or perhaps the slightly uglier static_cast<size_t>(-1);
D'oh! Fixed. (Thanks for the tip, too).
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSettings.h:35
> > +    // Must be called on the main thread before WebCompostor::initialize.
>
> Typo: Compostor.  We aren't turning layers into manure here :D
>
> I think having one comment describing this would make more sense - and I'd also group all the setters together at the bottom since most people looking at this
>
> It's a bit of a (conceptual) layering violation to have this refer to WebCompositor, that's a WebKit client API. I think the comment in WebCompositor.h is sufficient - alternately you could describe the invariants here in terms of other CC concepts
I've updated the comment, but I think I've noticed a problem. in CCLayerTreeHost::initializeLayerRenderer, we update the global settings based on capabilities we get from the layer renderer. This could, I think, happen more than once per process. Is this an issue? If so, do you think it would be better if CCSettings held the 'requested' settings, and CCLayerTreeSettings held the 'effective' settings (based on the real capabilities of the machine)?
>
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:639
> > +        // For these tests, we will enable threaded animations.
> > +        WebCompositor::setAcceleratedAnimationEnabled(true);
>
> For all of these tests?  Is this a change in behavior, or just shuffling logic around?
This is not a behavior change -- I just moved the spot where I applied the setting.
>
> > Source/WebKit/chromium/tests/CCTestCommon.h:33
> > +// If you have a test that modifies or uses global settings, keep an instance
> > +// of this class to ensure that you start and end with a clean slate.
>
> Could we instead have WebCompositor::shutdown() reset the settings?  Tests that depend on these things should hopefully be calling ::initialize()/::shutdown() already - right?

Done. This won't work for all the tests, though. Some tests do use the settings, but don't initialize or shut down the WebCompositor. E.g., LayerRendererChromiumTest2.initializationDoesNotMakeSynchronousCalls.

[James Robinson]

Comment on attachment 146558 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146558&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:147
> +    // Update settings based on capabilities that we got back from the renderer.

Ah yes, I think this is a (potential) problem - these capabilities are specific to the context used by this compositor instance, so I think it's (slightly) wrong to have them change global settings.  I say slightly because it would only matter if different contexts had different capabilities - it's really hard to think of a case where that would happen.

I could imagine these changing over time - for instance, on a dual-GPU system if we switched from one to the other I could imagine the maximum texture size changing.  Today if that happens we lose all contexts on the "old" device and then have to reinitialize them all.

Perhaps some of these really should be CCLayerTreeSettings - such as usingAcceleratedPainting, maxPartialTextureUpdates, maxUntiledLayerSize.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:150
> +    CCSettings::setDefaultTileSize(IntSize(min(CCSettings::defaultTileSize().width(), m_proxy->layerRendererCapabilities().maxTextureSize),

It's a bit strange to set the default value based on a capability - but maybe this setting is just poorly named?  I think we can safely assume that the max texture size will always be >=2k since that's required by DX9, so maybe we don't need this?

Created attachment 146666 [details]
Patch

(In reply to comment #13)
> (From update of attachment 146558 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146558&action=review
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:147
> > +    // Update settings based on capabilities that we got back from the renderer.
>
> Ah yes, I think this is a (potential) problem - these capabilities are specific to the context used by this compositor instance, so I think it's (slightly) wrong to have them change global settings.  I say slightly because it would only matter if different contexts had different capabilities - it's really hard to think of a case where that would happen.
>
> I could imagine these changing over time - for instance, on a dual-GPU system if we switched from one to the other I could imagine the maximum texture size changing.  Today if that happens we lose all contexts on the "old" device and then have to reinitialize them all.
>
> Perhaps some of these really should be CCLayerTreeSettings - such as usingAcceleratedPainting, maxPartialTextureUpdates, maxUntiledLayerSize.
That sounds safe. I've made these CCLayerTreeSettings.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:150
> > +    CCSettings::setDefaultTileSize(IntSize(min(CCSettings::defaultTileSize().width(), m_proxy->layerRendererCapabilities().maxTextureSize),
>
> It's a bit strange to set the default value based on a capability - but maybe this setting is just poorly named?  I think we can safely assume that the max texture size will always be >=2k since that's required by DX9, so maybe we don't need this?
I'm not sure about the name, but at first glance it seems reasonable to bound the default tile size by the maximum possible texture size.

[James Robinson]

Comment on attachment 146666 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146666&action=review

R=me.

> Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:29
> +#include <limits>

I think this #include is unused now

> Source/WebKit/chromium/public/WebCompositor.h:32
> +#include "platform/WebSize.h"

this appears to be unused

> Source/WebKit/chromium/tests/CCTestCommon.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012

Created attachment 146871 [details]
=patch for landing

(In reply to comment #15)
> (From update of attachment 146666 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146666&action=review
>
> R=me.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSettings.cpp:29
> > +#include <limits>
>
> I think this #include is unused now
Removed.
>
> > Source/WebKit/chromium/public/WebCompositor.h:32
> > +#include "platform/WebSize.h"
>
> this appears to be unused
Removed.
>
> > Source/WebKit/chromium/tests/CCTestCommon.h:2
> > + * Copyright (C) 2011 Google Inc. All rights reserved.
>
> 2012
Done.

[Dana Jansens]

Comment on attachment 146871 [details]
=patch for landing

=cq+

[WebKit Review Bot]

Comment on attachment 146871 [details]
=patch for landing

Rejecting attachment 146871 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
med 'setPartialSwapEnabled'
Source/WebKit/chromium/webkit/glue/webpreferences.cc:372: error: 'class WebKit::WebSettings' has no member named 'setPerTilePaintingEnabled'
cc1plus: warnings being treated as errors
At global scope:
cc1plus: error: unrecognized command line option "-Wno-narrowing"
cc1plus: error: unrecognized command line option "-Wno-narrowing"
make: *** [out/Debug/obj.target/glue/Source/WebKit/chromium/webkit/glue/webpreferences.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/12938488

[James Robinson]

(In reply to comment #18)
> (From update of attachment 146871 [details])
> Rejecting attachment 146871 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> med 'setPartialSwapEnabled'
> Source/WebKit/chromium/webkit/glue/webpreferences.cc:372: error: 'class WebKit::WebSettings' has no member named 'setPerTilePaintingEnabled'

Looks like you'll need to land some chromium-side changes first or add more guards.

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 146871 [details] [details])
> > Rejecting attachment 146871 [details] [details] from commit-queue.
> > 
> > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> > 
> > Last 500 characters of output:
> > med 'setPartialSwapEnabled'
> > Source/WebKit/chromium/webkit/glue/webpreferences.cc:372: error: 'class WebKit::WebSettings' has no member named 'setPerTilePaintingEnabled'
> 
> Looks like you'll need to land some chromium-side changes first or add more guards.

Strange. The chromium side has landed and does include guards. Has it maybe not rolled?

[James Robinson]

Check Source/WebKit/chromium/DEPS to see.  If it hasn't, please post a separate patch to roll that rev to the value you want and mark that as blocking this one (separate patch to avoid merge conflict hell).

[WebKit Review Bot]

Comment on attachment 146871 [details]
=patch for landing

Attachment 146871 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12951310

Created attachment 147355 [details]
Patch

Fix DRT.

[WebKit Review Bot]

Comment on attachment 147355 [details]
Patch

Clearing flags on attachment: 147355

Committed r120268: <http://trac.webkit.org/changeset/120268>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 89060

Created attachment 147587 [details]
Patch

Fix DRT again.

Ensure that we enable/disable per tile painting only once before we initialize
the WebCompositor.

[WebKit Review Bot]

Comment on attachment 147587 [details]
Patch

Clearing flags on attachment: 147587

Committed r120360: <http://trac.webkit.org/changeset/120360>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.