New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 597011 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Find Repeated Lookups of Histograms by Name

Project Member Reported by bcwh...@chromium.org, Mar 22 2016

Issue description

https://uma.googleplex.com/p/chrome/histograms/?dayCount=1&histograms=UMA.Histograms.Activity&fixupData=true&showMax=true&filters=channel%2Ceq%2C1%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial

The above indicates that are around 150x as many histogram-lookups as there are histograms created.  That likely means that some histograms are being looked-up a great many times rather than caching pointers to them (as the macros do).

Investigate.

 
Cc: tdres...@chromium.org
Should we have some automated testing that ensures that we don't regress on the ratio between histogram-lookups and histograms created?
That's a good idea.  How do I do that?

Cc: rkaplow@chromium.org sullivan@chromium.org motek@chromium.org
It's probably impossible to do in general, but we could do it for histograms touched by a specific set of user actions.

A few thoughts:
- We could make a telemetry test suite which reported the ratio
- We could have a browser test which EXPECT'ed the ratio was below some threshold.

Both of those sound a bit painful.

+some folks who might have better ideas.
Chirp might be the best option, but it would be nice to be able to bisect on regressions.
I don't think it's anything you're likely to catch in tests.  For example, the big culprit so far is the event class doing FactoryGet() for every event, including mouse-move events.  There are many of those in practice but probably not too many in tests.

Chirp is probably the best place to start.  At the very least, it'll give insight to how it could be better guarded.

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f96a2fc9c9a3676d457ad4c95631031613cb19f

commit 2f96a2fc9c9a3676d457ad4c95631031613cb19f
Author: bcwhite <bcwhite@chromium.org>
Date: Fri Apr 08 15:35:18 2016

Cache pointers to histograms for recording event times.

Doing a FactoryGet for every UI event (including every
mouse-move event) is expensive and unnecessary.  This
will reduce the time spent updating the histogram from
about 5us to about 0.8us (measured on Z620).

A reusable macro is added to allow any enumerated block
of histograms to be cached.

BUG=597011

Review URL: https://codereview.chromium.org/1829973002

Cr-Commit-Position: refs/heads/master@{#386086}

[modify] https://crrev.com/2f96a2fc9c9a3676d457ad4c95631031613cb19f/ui/events/event.cc

Sign in to add a comment