Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
69123002:
Generic version of caching cc switches to avoid searching of switches on each function call (Closed)
|
Created:
7 years, 1 month ago by sivag Modified:
6 years, 11 months ago CC:
chromium-reviews, cc-bugs_chromium.org, vivekg_samsung Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionGeneric version of caching cc switches to avoid searching of switches on each function call
CC functions like CheckImplSidePaintingStatus will cycle through all the switches
every time the function is called. Initial patch is avoiding it only for IsImplSidePaintingEnabled. So we need a generic way for all the switches, for future use.
This patch provides a one time initialization of all the switches in its first encounter (InitializeSwitchesIfRequired)
and later on will just read the updated data of the corresponding switches, when required.
Declared a seperate structure for holding the switch enable/disable data and saving memory by using bit fields.
BUG=313193
TEST=None; no behavior changes;
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237068
Patch Set 1 #
Total comments: 5
Patch Set 2 : Created a static class to hold the switch initialization! #Patch Set 3 : Adding the default case for MapImageEnabled #
Total comments: 3
Patch Set 4 : Reworked as per comments! #
Total comments: 2
Patch Set 5 : Reworked on old patch to avoid regression caused due to global pod variable. #
Total comments: 3
Patch Set 6 : Modified patch as per comments #Patch Set 7 : Removed logging header file. #Patch Set 8 : Merged with latest code base. #Patch Set 9 : Modified GPU Rasterization switch accordingly. #Messages
Total messages: 28 (0 generated)
PTAL.
Thanks for writing this. It looks like the kind of pattern we want. Is there a reason you didn't also include IsLCDTextEnabled()?
Some style comments. LCD text does look like another good candidate. https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc#newcode168 cc/base/switches.cc:168: // impl side painting The variable is called impl_side_painting, so I don't think this comment adds much value here. https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc#newcode169 cc/base/switches.cc:169: static bool implSidePaintingStatus; this is blink style, you want g_impl_side_painting_status how about g_impl_side_painting_enabled though? https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc#newcode170 cc/base/switches.cc:170: // map-image flag same with this comment https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc#newcode179 cc/base/switches.cc:179: // impl-side painting // Impl-side painting. (capitals and punctuation in comments please) https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc#newcode192 cc/base/switches.cc:192: // map-image flag // Map-image flag.
What is "stl cycle churn"? Is doing init once per flag really a problem?
On 2013/11/11 17:11:08, Tom Hudson wrote: > Thanks for writing this. It looks like the kind of pattern we want. > > Is there a reason you didn't also include IsLCDTextEnabled()? I will add this as well.
On 2013/11/11 19:20:53, jamesr wrote: > What is "stl cycle churn"? Is doing init once per flag really a problem? As described in the bug, multiple calls IsImplSidePaintingEnabled will call internally the stl search. The patch avoids that . InitializeCCSwitchesIfRequired will be processed only on the first call later it will not get called.
On 2013/11/11 19:20:53, jamesr wrote: > What is "stl cycle churn"? Is doing init once per flag really a problem? IsImplSidePaintingEnabled() is called quite frequently, which is why we wrote the patch to cache its value. (http://crbug.com/311248, r231314, https://codereview.chromium.org/43463003) When I did, the reviewers asked for it to be generalized to cache all the values. However, since the other functions are not called nearly so often, we just left it as a P3 bug. Not important, but it seems like a reasonable GoodFirstBug and chance to learn the process and coding conventions.
On 2013/11/11 17:57:53, danakj wrote: > Some style comments. LCD text does look like another good candidate. > > https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc > File cc/base/switches.cc (right): > > https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc#newcode168 > cc/base/switches.cc:168: // impl side painting > The variable is called impl_side_painting, so I don't think this comment adds > much value here. > > https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc#newcode169 > cc/base/switches.cc:169: static bool implSidePaintingStatus; > this is blink style, you want g_impl_side_painting_status > > how about g_impl_side_painting_enabled though? > > https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc#newcode170 > cc/base/switches.cc:170: // map-image flag > same with this comment > > https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc#newcode179 > cc/base/switches.cc:179: // impl-side painting > // Impl-side painting. > > (capitals and punctuation in comments please) > > https://codereview.chromium.org/69123002/diff/1/cc/base/switches.cc#newcode192 > cc/base/switches.cc:192: // map-image flag > // Map-image flag. Thank you @danakj. I have incorporated your review comments in the latest patch. I have tried to remove the usage of global variables with the introduction of static class CCSwitchHandler. PTAL. I am wondering if the entire CCSwitchHandler class can be auto-generated from the list of CC specific switches. Thoughts?
https://codereview.chromium.org/69123002/diff/130001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/69123002/diff/130001/cc/base/switches.cc#newc... cc/base/switches.cc:154: class CCSwitchHandler { you're already in the cc:: namespace. How about just "SwitchesHandler" I don't really see the point of the class here TBH, I think the static file-local variables are the same as some class-static variables, and this just adds more code. I'd move all these static class methods back into the public methods that now do nothing but call them. My last objection was just that the global variables needed a g_ prefix to their names. https://codereview.chromium.org/69123002/diff/130001/cc/base/switches.cc#newc... cc/base/switches.cc:156: static bool isImplSidePaintingEnabled(); IsImplSidePaintingEnabled <- capitalized for all these methods https://codereview.chromium.org/69123002/diff/130001/cc/base/switches.cc#newc... cc/base/switches.cc:164: static bool s_has_initialized_switches_; no trailing _ on static variables. they are not instance/member vars.
On 2013/11/13 19:37:40, danakj wrote: > I don't really see the point of the class here TBH, I think the static > file-local variables are the same as some class-static variables, and this just > adds more code. I'd move all these static class methods back into the public > methods that now do nothing but call them. > I thought of using bitfields for each individual switch variable. Modified the patch to use the structure and associated bitfields. PTAL.
LGTM
Can you update the description of the CL as "for avoiding stl cycle." is not clear what's happening here. Just stating we're avoiding looking through the CommandLine every time we query for the switch sounds right?
https://codereview.chromium.org/69123002/diff/210001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/69123002/diff/210001/cc/base/switches.cc#newc... cc/base/switches.cc:167: static Switches g_switches; How about moving this global variable as local static inside the InitializeSwitchesIfRequired() as- void InitializeSwitchesIfRequired() { static Switches switches; if (switches.has_initialized_switches_) return; ... }
https://codereview.chromium.org/69123002/diff/210001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/69123002/diff/210001/cc/base/switches.cc#newc... cc/base/switches.cc:167: static Switches g_switches; On 2013/11/20 01:50:15, vivekg__ wrote: > How about moving this global variable as local static inside the > InitializeSwitchesIfRequired() as- > > void InitializeSwitchesIfRequired() { > static Switches switches; > > if (switches.has_initialized_switches_) > return; > ... > } Then it would also have to return a const Switches&. I'm not sure what this buys though?
On 2013/11/20 01:55:21, danakj wrote: > https://codereview.chromium.org/69123002/diff/210001/cc/base/switches.cc > File cc/base/switches.cc (right): > > https://codereview.chromium.org/69123002/diff/210001/cc/base/switches.cc#newc... > cc/base/switches.cc:167: static Switches g_switches; > On 2013/11/20 01:50:15, vivekg__ wrote: > > How about moving this global variable as local static inside the > > InitializeSwitchesIfRequired() as- > > > > void InitializeSwitchesIfRequired() { > > static Switches switches; > > > > if (switches.has_initialized_switches_) > > return; > > ... > > } > > Then it would also have to return a const Switches&. I'm not sure what this buys > though? Yes that's right it needs to return the reference. This would have the lazy initialization of switches instead of global one. Just a thought. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/69123002/210001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/69123002/210001
Message was sent while issue was closed.
Change committed as 237068
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/85753003/ by rogerta@chromium.org. The reason for reverting is: This CL caused a regression in sizes bots, see https://chromeperf.appspot.com/report?masters=Chromium&bots=chromium-rel-linu....
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/85903002/ by rogerta@chromium.org. The reason for reverting is: This CL caused a regression in sizes bots, see https://chromeperf.appspot.com/report?masters=Chromium&bots=chromium-rel-linu....
On 2013/11/25 15:01:03, Roger Tawa wrote: > A revert of this CL has been created in > https://codereview.chromium.org/85903002/ by mailto:rogerta@chromium.org. > > The reason for reverting is: This CL caused a regression in sizes bots, see > https://chromeperf.appspot.com/report?masters=Chromium&bots=chromium-rel-linu.... Reworked on old patch to avoid regression caused due to global pod variables.
https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc#newc... cc/base/switches.cc:157: typedef enum { This is a lot of work. Can we just have 4 static bools without a struct?
https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc#newc... cc/base/switches.cc:157: typedef enum { On 2013/11/26 15:55:50, danakj wrote: > This is a lot of work. Can we just have 4 static bools without a struct? As you know we cannot maintain the static bool global variables, so the only way is to maintain the static bool local variables, for this we have to write two functions for getting every switch value , like CheckImplSidePaintingStatus and IsImplSidePaintingEnabled. Instead of this we are trying to getswitch value from a single function just by adding a extra enum.
https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc#newc... cc/base/switches.cc:157: typedef enum { On 2013/11/26 17:28:49, Sikugu wrote: > On 2013/11/26 15:55:50, danakj wrote: > > This is a lot of work. Can we just have 4 static bools without a struct? > As you know we cannot maintain the static bool global variables, so the only way > is to maintain the static bool local variables, for this we have to write two > functions for getting every switch value , like CheckImplSidePaintingStatus > and IsImplSidePaintingEnabled. > Instead of this we are trying to getswitch value from a single function just by > adding a extra enum. > > > Why can't we have global static bools? We do this all the time with primitive types such as bool or pointers. The issue was a static class.
On 2013/11/26 15:55:50, danakj wrote: > https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc > File cc/base/switches.cc (right): > > https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc#newc... > cc/base/switches.cc:157: typedef enum { > This is a lot of work. Can we just have 4 static bools without a struct? The reason we maintained a struct is, if we are going to implement this for all switches the memory requirement and no of variables would me more, so we thought better to wrap in a structure and add bit fields. If you feel this adds complexity we can remove this.
On 2013/11/26 17:35:16, danakj wrote: > https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc > File cc/base/switches.cc (right): > > https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc#newc... > cc/base/switches.cc:157: typedef enum { > On 2013/11/26 17:28:49, Sikugu wrote: > > On 2013/11/26 15:55:50, danakj wrote: > > > This is a lot of work. Can we just have 4 static bools without a struct? > > As you know we cannot maintain the static bool global variables, so the only > way > > is to maintain the static bool local variables, for this we have to write two > > functions for getting every switch value , like CheckImplSidePaintingStatus > > and IsImplSidePaintingEnabled. > > Instead of this we are trying to getswitch value from a single function just > by > > adding a extra enum. > > > > > > > > Why can't we have global static bools? We do this all the time with primitive > types such as bool or pointers. The issue was a static class. Ok got it.
Whoa whoa, this is a lot of complication for some functions that appear to only be called 1-3 times per compositor.
On 2013/11/26 17:35:16, danakj wrote: > https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc > File cc/base/switches.cc (right): > > https://codereview.chromium.org/69123002/diff/590001/cc/base/switches.cc#newc... > cc/base/switches.cc:157: typedef enum { > On 2013/11/26 17:28:49, Sikugu wrote: > > On 2013/11/26 15:55:50, danakj wrote: > > > This is a lot of work. Can we just have 4 static bools without a struct? > > As you know we cannot maintain the static bool global variables, so the only > way > > is to maintain the static bool local variables, for this we have to write two > > functions for getting every switch value , like CheckImplSidePaintingStatus > > and IsImplSidePaintingEnabled. > > Instead of this we are trying to getswitch value from a single function just > by > > adding a extra enum. > > > > > > > > Why can't we have global static bools? We do this all the time with primitive > types such as bool or pointers. The issue was a static class. #Siva:: Hi Dana, Please review this i added new code as per the comments. |
