Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1829973002:
Cache pointers to histograms for recording event times. (Closed)
DescriptionCache 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
Committed: https://crrev.com/2f96a2fc9c9a3676d457ad4c95631031613cb19f
Cr-Commit-Position: refs/heads/master@{#386086}
Patch Set 1 #
Total comments: 4
Patch Set 2 : rebased #Patch Set 3 : move macro into only .cc file in which it's (currently) used #Patch Set 4 : rebased #
Depends on Patchset: Messages
Total messages: 27 (14 generated)
Description was changed from ========== Cache pointers to histograms for recording event times. Doing a FactoryGet for every UI event (including every mouse-move event) is expensive and unnecessary. A reusable macro is added to allow any enumerated block of histograms to be cached. BUG=597011 ========== to ========== 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. A reusable macro is added to allow any enumerated block of histograms to be cached. BUG=597011 ==========
Description was changed from ========== 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. A reusable macro is added to allow any enumerated block of histograms to be cached. BUG=597011 ========== to ========== 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. A reusable macro is added to allow any enumerated block of histograms to be cached. BUG=597011 ==========
Description was changed from ========== 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. A reusable macro is added to allow any enumerated block of histograms to be cached. BUG=597011 ========== to ========== 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 ==========
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1829973002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/1829973002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:112: // Support a collection of histograms, perhaps one for each entry in an This seems to be an esoteric use case - and while suffixed histograms are fairly common, I don't think it's often the case that the suffixes always correspond to an enum. Unless there's more than one place that needs (which doesn't look like just from this CL - but maybe you've found other cases?), I think it's better to define this macro in the same file that uses it. If we find more cases, then it can be provided more generally.
https://codereview.chromium.org/1829973002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/1829973002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:112: // Support a collection of histograms, perhaps one for each entry in an On 2016/03/30 16:47:38, Alexei Svitkine wrote: > This seems to be an esoteric use case - and while suffixed histograms are fairly > common, I don't think it's often the case that the suffixes always correspond to > an enum. This macro just manages a vector. It knows nothing about the actual histogram names. The factory_get_invocation deals with those details. > Unless there's more than one place that needs (which doesn't look like just from > this CL - but maybe you've found other cases?), I think it's better to define > this macro in the same file that uses it. If we find more cases, then it can be > provided more generally. I think there will be. Another culprit are certain "disk" tracking which index 1,2,3,4,5. I haven't investigated; I just saw them being repeatedly loaded.
https://codereview.chromium.org/1829973002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/1829973002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:112: // Support a collection of histograms, perhaps one for each entry in an On 2016/03/30 17:33:08, bcwhite wrote: > On 2016/03/30 16:47:38, Alexei Svitkine wrote: > > This seems to be an esoteric use case - and while suffixed histograms are > fairly > > common, I don't think it's often the case that the suffixes always correspond > to > > an enum. > > This macro just manages a vector. It knows nothing about the actual histogram > names. The factory_get_invocation deals with those details. Right, but it requires having an index that also corresponds to a custom name. that you're passing to the histogram. (If you pass the same name, it doesn't work right.) Now, I could believe that more than one place would want this, but I'd like to see it (i.e. two places using) before agreeing that we should have it in base histograms code. Can you take a look at the other call site you were planning to look at and see if it can benefit from this? If so, both can be added to the same CL. Also, it would be useful to have a comment with an example use, since it's not super clear from this comment. One other thought - if we do want to have it here, perhaps we could just make STATIC_HISTOGRAM_POINTER_BLOCK() use this new macro with an index of 0? > > > > Unless there's more than one place that needs (which doesn't look like just > from > > this CL - but maybe you've found other cases?), I think it's better to define > > this macro in the same file that uses it. If we find more cases, then it can > be > > provided more generally. > > I think there will be. Another culprit are certain "disk" tracking which index > 1,2,3,4,5. I haven't investigated; I just saw them being repeatedly loaded.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/1829973002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/1829973002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:112: // Support a collection of histograms, perhaps one for each entry in an > > > This seems to be an esoteric use case - and while suffixed histograms are > > fairly > > > common, I don't think it's often the case that the suffixes always > correspond > > to > > > an enum. > > > > This macro just manages a vector. It knows nothing about the actual histogram > > names. The factory_get_invocation deals with those details. > > Right, but it requires having an index that also corresponds to a custom name. > that you're passing to the histogram. (If you pass the same name, it doesn't > work right.) Gotcha. Right. > Now, I could believe that more than one place would want this, but I'd like to > see it (i.e. two places using) before agreeing that we should have it in base > histograms code. Can you take a look at the other call site you were planning to > look at and see if it can benefit from this? If so, both can be added to the > same CL. Okay. I'll move it into event.cc with a comment saying it can be moved to histogram_macros if it proves useful elsewhere. > Also, it would be useful to have a comment with an example use, since it's not > super clear from this comment. Done. > > One other thought - if we do want to have it here, perhaps we could just make > STATIC_HISTOGRAM_POINTER_BLOCK() use this new macro with an index of 0? > > > > > > > > Unless there's more than one place that needs (which doesn't look like just > > from > > > this CL - but maybe you've found other cases?), I think it's better to > define > > > this macro in the same file that uses it. If we find more cases, then it can > > be > > > provided more generally. > > > > I think there will be. Another culprit are certain "disk" tracking which > index > > 1,2,3,4,5. I haven't investigated; I just saw them being repeatedly loaded. >
lgtm
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829973002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829973002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
bcwhite@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@chromium.org: Please review changes in events.cc
stamp lgtm
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1829973002/#ps120001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829973002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829973002/120001
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/2f96a2fc9c9a3676d457ad4c95631031613cb19f Cr-Commit-Position: refs/heads/master@{#386086} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2f96a2fc9c9a3676d457ad4c95631031613cb19f Cr-Commit-Position: refs/heads/master@{#386086} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
