Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1252133002:
Optimizing the time taken to build and show the WrenchMenu. (Closed)
DescriptionOptimizing the time taken to build and show the WrenchMenu.
While adding the ink drop ripple animation to the WrenchToolbarButton I found that the ToolbarView::ShowAppMenu(...) was taking a noticeable amount of time to build and show the menu.
Patch set 1 has some profiling logging added and contains a log_output.txt showing that the majority of time is being spent calculating the string length for the ZoomView.
Patch set 2 includes a fix to cache the max length and prevent it from being calculated more often than it needs to be. It also contains log_output.txt to show the time improvements.
tldr for log_output.txt: With no functional changes patch set 1 spent 0.090804s creating the WrenchMenu and 0.248378s total time in Canvas::SizeStringFloat(...) over 64 invocations whereas Patch set 2 spent 0.003858s creating the WrenchMenu and 0.202926s total time in Canvas::SizeStringFloat(...) over 42 invocations.
Note: The timings were harvested when running the ChromeOS build on a linux box so the values may be somewhat inflated however the percentage increase is still very dramatic.
BUG=505587
Committed: https://crrev.com/bc8f9df013152ffc9708eb760fd4a0903cbad63d
Cr-Commit-Position: refs/heads/master@{#340691}
Patch Set 1 : Profiling logging added. #Patch Set 2 : Added lazy computation for the WrenchMenu::ZoomView::zoom_label_width_ and updated log_output.txt. #Patch Set 3 : Removed experimental work, logging and output_log.txt. #
Total comments: 14
Patch Set 4 : Addressed pkasting@'s comments from patch set 3. #Messages
Total messages: 19 (3 generated)
bruthig@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@, can you PTAL?
Cool. 0.2 sec in SizeStringFloat() still seems like a long time. Can we improve that further? (Doesn't have to be in this change.) https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/wrench_menu.cc (right): https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:374: WrenchMenu* menu() const { return menu_; } Const methods cannot return non-const pointers. You can return a const WrenchMenu* if need be. Otherwise we need to come up with a different solution to whatever problem you're having. https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:661: int MaxWidthForZoomLabel() const { Nit: ZoomLabelMaxWidth()? MaxZoomLabelWidth()? And then change both the member variable names to match https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:663: zoom_label_width_ = CalculateMaxWidthForZoomLabel(); Nit: Since this is called once, just inline its code into here. https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:716: // 100%. Nit: Consider adding a short note about why we bother to cache this. https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:719: // Flag that is true if |zoom_label_width_| is a valid value or false if it Nit: How about "Flag tracking whether calls to MaxWidthForZoomLabel() need to recalculate the label width, because the cached value may no longer be correct." https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:721: mutable bool zoom_label_width_value_is_valid_; Nit: |zoom_label_width_valid_| is just as clear and less verbose.
jonross@chromium.org changed reviewers: + jonross@chromium.org
A future optimization I believe can be gained when we change the layout of the wrench menu. Having the regions for the zoom menu item be defined as a proportion of the width of the wrench menu would reduce the number of string measurements. We could only measure the string we plan to display. Rather than measuring all possible zoom level strings to see what is the widest we could be. https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/wrench_menu.cc (right): https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:663: zoom_label_width_ = CalculateMaxWidthForZoomLabel(); On 2015/07/23 18:46:32, Peter Kasting wrote: > Nit: Since this is called once, just inline its code into here. It is called in both GetPreferredSize and Layout.
https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/wrench_menu.cc (right): https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:663: zoom_label_width_ = CalculateMaxWidthForZoomLabel(); On 2015/07/23 19:55:51, jonross wrote: > On 2015/07/23 18:46:32, Peter Kasting wrote: > > Nit: Since this is called once, just inline its code into here. > > It is called in both GetPreferredSize and Layout. Nevermind, misread. Please ignore.
Also, I don't think patch set 3 is complete; I think there should be some .h changes as well.
pkasting@, can you PTAL? Also it's not immediately obvious to me what changes are required in the .h file. Can you please elaborate? https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/wrench_menu.cc (right): https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:374: WrenchMenu* menu() const { return menu_; } On 2015/07/23 18:46:32, Peter Kasting wrote: > Const methods cannot return non-const pointers. You can return a const > WrenchMenu* if need be. Otherwise we need to come up with a different solution > to whatever problem you're having. Done. https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:661: int MaxWidthForZoomLabel() const { On 2015/07/23 18:46:32, Peter Kasting wrote: > Nit: ZoomLabelMaxWidth()? MaxZoomLabelWidth()? And then change both the member > variable names to match Done. https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:663: zoom_label_width_ = CalculateMaxWidthForZoomLabel(); On 2015/07/23 18:46:32, Peter Kasting wrote: > Nit: Since this is called once, just inline its code into here. Done. But I am curious why it is you prefer to inline this long and somewhat complex method? From a readability standpoint I find having the CalculateZoomLabelMaxWidth() much easier to read and understand. It offers a nice level of abstraction from the perspective of the ZoomLabelMaxWidth() method. i.e. I don't have to sift through the details of the calculation code when trying to understand how ZoomLabelMaxWidth() to fully understand how ZoomLabelMaxWidth() is performing the caching I can simply trust that CalculateZoomLabelMaxWidth() is doing something. https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:716: // 100%. On 2015/07/23 18:46:32, Peter Kasting wrote: > Nit: Consider adding a short note about why we bother to cache this. Done. https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:719: // Flag that is true if |zoom_label_width_| is a valid value or false if it On 2015/07/23 18:46:32, Peter Kasting wrote: > Nit: How about "Flag tracking whether calls to MaxWidthForZoomLabel() need to > recalculate the label width, because the cached value may no longer be correct." Done. I like :) https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/wrench_menu.cc:721: mutable bool zoom_label_width_value_is_valid_; On 2015/07/23 18:46:32, Peter Kasting wrote: > Nit: |zoom_label_width_valid_| is just as clear and less verbose. Done.
danakj@, do you have any idea if Canvas::SizeStringFloat(...) has been scrutinized for efficiency?
On 2015/07/23 21:19:14, bruthig wrote: > danakj@, do you have any idea if Canvas::SizeStringFloat(...) has been > scrutinized for efficiency? I have no data. Given it's doing a malloc every call, it looks unlikely.
LGTM Ignore the .h comment, I was confused. Regarding inlining: separating code into different functions can definitely improve readability in the form of putting a single functional name to a long block of code, so caller code doesn't have a long chunk of stuff obscuring the surrounding meaning. However, it also has two small costs in a case where the code was only used once: the overall code may be longer and have more function declaration boilerplate; and it's not obvious either from the caller or callee site that the functionality is only used once, rather than being necessary for multiple places. This can lead to orphaned code if the caller is removed or changed but the callee not removed, and it can lead to obscuring the data model if a reader is trying to figure out when various members the callee touches are modified. In this case, the benefit is small, because the caller did almost nothing other than call the callee, so the costs potentially outweigh it. Regarding SizeStringInt(): I'm pretty sure we've never tried to improve the perf of this function. It's called enough in the UI code that looking at its perf would make sense, though the solution is likely a combination of speeding up that method and caching calls to it.
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1252133002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1252133002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bc8f9df013152ffc9708eb760fd4a0903cbad63d Cr-Commit-Position: refs/heads/master@{#340691}
Message was sent while issue was closed.
On 2015/07/23 19:55:51, jonross wrote: > A future optimization I believe can be gained when we change the layout of the > wrench menu. > > Having the regions for the zoom menu item be defined as a proportion of the > width of the wrench menu would reduce the number of string measurements. > > We could only measure the string we plan to display. Rather than measuring all > possible zoom level strings to see what is the widest we could be. I don't think this is optimal because the string you display can change while the menu is showing. But it seems like we should be able to just test what we assume to be the widest. 500%? Is the widest number going to depend on the locale? I might start with setting the width based on 500% and test on a number of locales to see if anything breaks. If so you could do the old trick of creating a translateable string which is just a number of characters, and defines the element's width, so translators can set a custom width for every locale. > > https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/toolbar/wrench_menu.cc (right): > > https://codereview.chromium.org/1252133002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/wrench_menu.cc:663: zoom_label_width_ = > CalculateMaxWidthForZoomLabel(); > On 2015/07/23 18:46:32, Peter Kasting wrote: > > Nit: Since this is called once, just inline its code into here. > > It is called in both GetPreferredSize and Layout.
Message was sent while issue was closed.
> I don't think this is optimal because the string you display can change while > the menu is showing. But it seems like we should be able to just test what we > assume to be the widest. 500%? Is the widest number going to depend on the > locale? I might start with setting the width based on 500% and test on a number > of locales to see if anything breaks. If so you could do the old trick of > creating a translateable string which is just a number of characters, and > defines the element's width, so translators can set a custom width for every > locale. Sounds like a good idea to me, if we really wanted to be safe we could add a unit test that verifies that the string length of the width string is actually larger than the string length of all the possible values. Essentially we would be paying the runtime expense during our tests instead of the end users.
Message was sent while issue was closed.
On 2015/09/18 19:34:43, bruthig wrote: > > I don't think this is optimal because the string you display can change while > > the menu is showing. But it seems like we should be able to just test what we > > assume to be the widest. 500%? Is the widest number going to depend on the > > locale? I might start with setting the width based on 500% and test on a > number > > of locales to see if anything breaks. If so you could do the old trick of > > creating a translateable string which is just a number of characters, and > > defines the element's width, so translators can set a custom width for every > > locale. > > Sounds like a good idea to me, if we really wanted to be safe we could add a > unit test that verifies that the string length of the width string is actually > larger than the string length of all the possible values. Essentially we would > be paying the runtime expense during our tests instead of the end users. Unittests aren't run in other locales, so that wouldn't save you from problems there. |
