Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27058002:
Cache results from respondsToSelector. (Closed)
|
Created:
7 years, 2 months ago by ccameron Modified:
7 years, 2 months ago CC:
blink-reviews, nduca, viettrungluu Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionUse an observer for preferredScrollerStyle instead of querying it.
This came up as a hotspot in layout.
BUG=303205
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159812
Patch Set 1 #Patch Set 2 : Make similar #
Total comments: 2
Patch Set 3 : Use static #Patch Set 4 : Use static #Patch Set 5 : Use static #Patch Set 6 : Add observer #Patch Set 7 : Fix whitespace #Patch Set 8 : Fix whitespace again #
Total comments: 8
Patch Set 9 : Incorporate review feedback #Patch Set 10 : Incorporate review feedback #
Total comments: 1
Patch Set 11 : Add hot warning #Messages
Total messages: 27 (0 generated)
On 2013/10/11 18:52:01, ccameron1 wrote: This mirrors what we do a few lines before in isScrollbarOverlayAPIAvailable. If the (many) other calls to respondsToSelector are problematic as well then we may want to look into a macro to do this sort of caching. https://code.google.com/p/chromium/codesearch#search/&q=respondsToSelector&sq...
I'd guess most of these calls aren't hot (this is the first time I've seen this), so just fixing this instance is probably enough for now. https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... File Source/core/platform/mac/NSScrollerImpDetails.mm (left): https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... Source/core/platform/mac/NSScrollerImpDetails.mm:51: if ([NSScroller respondsToSelector:@selector(preferredScrollerStyle)]) If this is called from a single thread, just static bool respondsToPreferredScrollerStyle = [NSScroller respondsToSelector:@selector(preferredScrollerStyle)]; // static for speed would be enough and a tad shorter. https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... File Source/core/platform/mac/NSScrollerImpDetails.mm (right): https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... Source/core/platform/mac/NSScrollerImpDetails.mm:52: bool shouldInitialize = true; …wait, how does this version work? Isn't this still called every frame? Did you check in a profiler that the hotspot actually goes away?
> On Oct 11, 2013, at 14:48, thakis@chromium.org wrote: > > I'd guess most of these calls aren't hot (this is the first time I've seen > this), so just fixing this instance is probably enough for now. > > > https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... > File Source/core/platform/mac/NSScrollerImpDetails.mm (left): > > https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... > Source/core/platform/mac/NSScrollerImpDetails.mm:51: if ([NSScroller > respondsToSelector:@selector(preferredScrollerStyle)]) > If this is called from a single thread, just > > static bool respondsToPreferredScrollerStyle = [NSScroller > respondsToSelector:@selector(preferredScrollerStyle)]; // static for > speed > > would be enough and a tad shorter. > > https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... > File Source/core/platform/mac/NSScrollerImpDetails.mm (right): > > https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... > Source/core/platform/mac/NSScrollerImpDetails.mm:52: bool > shouldInitialize = true; > …wait, how does this version work? Isn't this still called every frame? > Did you check in a profiler that the hotspot actually goes away? > > https://codereview.chromium.org/27058002/ To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Thanks! On 2013/10/11 19:48:47, Nico wrote: > I'd guess most of these calls aren't hot (this is the first time I've seen > this), so just fixing this instance is probably enough for now. > > https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... > File Source/core/platform/mac/NSScrollerImpDetails.mm (left): > > https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... > Source/core/platform/mac/NSScrollerImpDetails.mm:51: if ([NSScroller > respondsToSelector:@selector(preferredScrollerStyle)]) > If this is called from a single thread, just > > static bool respondsToPreferredScrollerStyle = [NSScroller > respondsToSelector:@selector(preferredScrollerStyle)]; // static for speed > > would be enough and a tad shorter. I assumed that the convention in isScrollbarOverlayAPIAvailable was just convention. IIUC it doesn't provide any thread safety (and even if the ordering of shouldInitialize being un-set it still wouldn't, as the ordering of stores isn't guaranteed)... is that not the case? > https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... > File Source/core/platform/mac/NSScrollerImpDetails.mm (right): > > https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/N... > Source/core/platform/mac/NSScrollerImpDetails.mm:52: bool shouldInitialize = > true; > …wait, how does this version work? Isn't this still called every frame? Did you > check in a profiler that the hotspot actually goes away? Urgh, my mistake. The bug provided a pretty good case for this patch working, but I can't repro the usage locally. I'll poke at it some more.
On Fri, Oct 11, 2013 at 2:24 PM, <ccameron@chromium.org> wrote: > Thanks! > > > On 2013/10/11 19:48:47, Nico wrote: > >> I'd guess most of these calls aren't hot (this is the first time I've seen >> this), so just fixing this instance is probably enough for now. >> > > > https://codereview.chromium.**org/27058002/diff/3001/Source/** > core/platform/mac/**NSScrollerImpDetails.mm<https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/NSScrollerImpDetails.mm> > >> File Source/core/platform/mac/**NSScrollerImpDetails.mm (left): >> > > > https://codereview.chromium.**org/27058002/diff/3001/Source/** > core/platform/mac/**NSScrollerImpDetails.mm#**oldcode51<https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/NSScrollerImpDetails.mm#oldcode51> > >> Source/core/platform/mac/**NSScrollerImpDetails.mm:51: if ([NSScroller >> respondsToSelector:@selector(**preferredScrollerStyle)]) >> If this is called from a single thread, just >> > > static bool respondsToPreferredScrollerSty**le = [NSScroller >> respondsToSelector:@selector(**preferredScrollerStyle)]; // static for >> speed >> > > would be enough and a tad shorter. >> > > I assumed that the convention in isScrollbarOverlayAPIAvailable was just > convention. IIUC it doesn't provide any thread safety (and even if the > ordering > of shouldInitialize being un-set it still wouldn't, as the ordering of > stores > isn't guaranteed)... is that not the case? No, isScrollbarOverlayAPIAvailable isn't any better than what I propose (and it's longer, so it's a bit worse). I didn't see it, even though it's just a few lines further up. We don't build with thread-safe statics. > > > > https://codereview.chromium.**org/27058002/diff/3001/Source/** > core/platform/mac/**NSScrollerImpDetails.mm<https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/NSScrollerImpDetails.mm> > >> File Source/core/platform/mac/**NSScrollerImpDetails.mm (right): >> > > > https://codereview.chromium.**org/27058002/diff/3001/Source/** > core/platform/mac/**NSScrollerImpDetails.mm#**newcode52<https://codereview.chromium.org/27058002/diff/3001/Source/core/platform/mac/NSScrollerImpDetails.mm#newcode52> > >> Source/core/platform/mac/**NSScrollerImpDetails.mm:52: bool >> shouldInitialize = >> true; >> …wait, how does this version work? Isn't this still called every frame? >> Did >> > you > >> check in a profiler that the hotspot actually goes away? >> > > Urgh, my mistake. > > The bug provided a pretty good case for this patch working, but I can't > repro > the usage locally. I'll poke at it some more. > If you can't repro, this patch is fine too if you make your bools static. As is, the patch does nothing :-) > > https://codereview.chromium.**org/27058002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Okay, updated to use static now. I never saw the 8% utilization, but this patch brings it down from 2.1 to 1.
On 2013/10/15 04:09:07, ccameron1 wrote: > Okay, updated to use static now. I never saw the 8% utilization, but this patch > brings it down from 2.1 to 1. I'll see if I can run the profile again tomorrow. I just loaded up that page and then scrolled for several seconds downward as fast as I could. Note that this one function is only 2% in my profile too, it's the rest that adds up to 8%: 224.0ms 8.6% 5.0 WebCore::ScrollbarThemeMacOverlayAPI::usesOverlayScrollbars() const 204.0ms 7.8% 14.0 WebCore::recommendedScrollerStyle() 67.0ms 2.5% 6.0 +[NSObject respondsToSelector:] 66.0ms 2.5% 12.0 _NSRecommendedScrollerStyle 46.0ms 1.7% 46.0 objc_msgSend There's 1.7% in obj_msgSend overhead and 2.5% in _NSRecommendedScrollerStyle (where ever that comes from).
I added the trace and steps to reproduce: https://code.google.com/p/chromium/issues/detail?id=303205#c6 respondsToSelector is only a small portion of this.
Slightly less cryptic: Is there any advantage of your patch over just static bool respondsToSelectorPreferredScrollerStyle = [NSScroller respondsToSelector:@selector(preferredScrollerStyle)]; ? That's a bit shorter. (If you do change this, change it in the other function in this file too I guess.)
It turns out that the big hotspot (~8-15%) only happens with an external mouse (and maybe monitor). It turns out that just using statics won't do enough there, so I added an observer for changes in the property. Of note is that we leak this (single) instance of the notifier -- is that a reasonable policy?
lgtm Wow. I suspect this basically never changes. I'm surprised we can get this kind of information inside the Mac sandbox (and that we wouldn't have to boot renderers with this). I assume you tested by running chrome and changing your appearance settings?
I should note: although I feel confident reviewing Obj-C changes, and core/ changes, you probably want someone who's more familiar with the quirks of our sandbox and chromium-mac-code style before landing.
On 2013/10/15 21:51:02, eseidel wrote: > lgtm > > Wow. > > I suspect this basically never changes. I'm surprised we can get this kind of > information inside the Mac sandbox (and that we wouldn't have to boot renderers > with this). > > I assume you tested by running chrome and changing your appearance settings? Yup, I can trigger the observer by changing settings in control panel. Also of note is that this makes recommendedScrollerStyle completely vanish from profiles, but isOverlayScrollbar() is still there (around <1%, but still there) -- maybe we can figure out how to elide all of these calls?
https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... File Source/core/platform/mac/NSScrollerImpDetails.mm (right): https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... Source/core/platform/mac/NSScrollerImpDetails.mm:54: if (self) { I might have written if (!self)\n return nil; Also, why 2-space indent? https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... Source/core/platform/mac/NSScrollerImpDetails.mm:89: static ScrollerStyleObserver* scrollerStyleObserver = [[ScrollerStyleObserver alloc] init]; new] would have done the same as alloc] init];
I'm happy to make incremental progress. We can make isOverlayScrollbar a global if we need to. But I suspect we can just avoid calling it so much. Also, when this changes, don't we need to invalidate the whole document?
https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... File Source/core/platform/mac/NSScrollerImpDetails.mm (right): https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... Source/core/platform/mac/NSScrollerImpDetails.mm:69: g_scrollerStyle = [NSScroller preferredScrollerStyle]; Do we need to tell somebody that this changed? Send some global notification of our own, or are we content with the fact that the user will start to see the srollbars once they refresh. I'm curious what Apple's code looks like for the same in WebKit2.
lgtm Wow, surprising. viettrungluu is thinking about removing the ui runloop from the renderer ( https://code.google.com/p/chromium/issues/detail?id=306348 ), he should check this notification still works when he does that. If not, he needs to pipe this through from the browser side. https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... File Source/core/platform/mac/NSScrollerImpDetails.mm (right): https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... Source/core/platform/mac/NSScrollerImpDetails.mm:54: if (self) { On 2013/10/15 21:56:02, eseidel wrote: > I might have written if (!self)\n return nil; I think the usual stanza in chromium is if ((self = [super init])) I like the early return better too, but oh well. https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... Source/core/platform/mac/NSScrollerImpDetails.mm:89: static ScrollerStyleObserver* scrollerStyleObserver = [[ScrollerStyleObserver alloc] init]; On 2013/10/15 21:56:02, eseidel wrote: > new] would have done the same as alloc] init]; Yeah, but alloc init is more common for some reason.
Thanks!! https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... File Source/core/platform/mac/NSScrollerImpDetails.mm (right): https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... Source/core/platform/mac/NSScrollerImpDetails.mm:54: if (self) { On 2013/10/15 22:00:14, Nico wrote: > On 2013/10/15 21:56:02, eseidel wrote: > > I might have written if (!self)\n return nil; > > I think the usual stanza in chromium is > > if ((self = [super init])) > > I like the early return better too, but oh well. Done. Thought I'd caught all of the 2-spaces... https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... Source/core/platform/mac/NSScrollerImpDetails.mm:69: g_scrollerStyle = [NSScroller preferredScrollerStyle]; On 2013/10/15 21:57:53, eseidel wrote: > Do we need to tell somebody that this changed? Send some global notification of > our own, or are we content with the fact that the user will start to see the > srollbars once they refresh. > > I'm curious what Apple's code looks like for the same in WebKit2. Something else /does/ trigger a re-layout when the properties are changed, but I'm not sure what the mechanism is offhand (it may be in RenderThemeChromiumMac ...). https://codereview.chromium.org/27058002/diff/31001/Source/core/platform/mac/... Source/core/platform/mac/NSScrollerImpDetails.mm:89: static ScrollerStyleObserver* scrollerStyleObserver = [[ScrollerStyleObserver alloc] init]; On 2013/10/15 22:00:14, Nico wrote: > On 2013/10/15 21:56:02, eseidel wrote: > > new] would have done the same as alloc] init]; > > Yeah, but alloc init is more common for some reason. Left this one as-is.
https://codereview.chromium.org/27058002/diff/45001/Source/core/platform/mac/... File Source/core/platform/mac/NSScrollerImpDetails.mm (right): https://codereview.chromium.org/27058002/diff/45001/Source/core/platform/mac/... Source/core/platform/mac/NSScrollerImpDetails.mm:87: // The ScrollerStyleObserver will update g_scrollerStyle at init and when needed. (might want to add a "this function is hot" comment the next time you're here)
Added the hot warning & putting in the CQ (I see some layout failures, but I see them in previous non-me runs on those bots).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/27058002/52001
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/27058002/52001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/27058002/52001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/27058002/52001
Message was sent while issue was closed.
Change committed as 159812 |
