Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2570893003:
Clean up LatencyInfo and RWHLatencyTracker. (Closed)
|
Created:
4 years ago by tdresser Modified:
3 years, 11 months ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, dtapuska+chromiumwatch_chromium.org, jam, qsr+mojo_chromium.org, tdresser+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up LatencyInfo and RWHLatencyTracker. May break trace processing?
In preparation for landing keyboard event latency code, do some
cleanup of the existing input latency logic.
In particular, get rid of modality specific TERMINATED components.
BUG=673731
Review-Url: https://codereview.chromium.org/2570893003
Cr-Commit-Position: refs/heads/master@{#445532}
Committed: https://chromium.googlesource.com/chromium/src/+/fcda569a9fade182b0771194e91be0ab46fb7213
Patch Set 1 #Patch Set 2 : Rebase & polish. #Patch Set 3 : Fix bug & test. #Patch Set 4 : Fix bug with coalesced events. #
Total comments: 5
Patch Set 5 : Remove functional change, nits. #Patch Set 6 : Add comment about coalescing. #
Total comments: 3
Patch Set 7 : No more macros. #Patch Set 8 : Example of STATIC_HISTOGRAM_POINTER_GROUP. #Patch Set 9 : Splitting up patch - not changing histogram API. #
Total comments: 2
Patch Set 10 : Avoid introducing new bare calls to FactoryGet. #
Messages
Total messages: 46 (25 generated)
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tdresser@chromium.org changed reviewers: + dtapuska@chromium.org, eakuefner@chromium.org, isherman@chromium.org, kenrb@chromium.org
+kenrb for mojo +dtapuska for general input +isherman for histogram macro usage in render_widget_host_latency_tracker.cc +eakuefner for info on how likely it is that getting rid of type specific TERMINATED components will break some trace processing
I feel this has a massive memory leak. Perhaps I'm wrong in understanding the histograms API; but ownership is clearly documented here: https://cs.chromium.org/chromium/src/base/metrics/histogram.cc?sq=package:chr... https://codereview.chromium.org/2570893003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (left): https://codereview.chromium.org/2570893003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:413: if (latency.coalesced()) Why are we adding back logging of coalesced events? Is it intentional? If so then won't this change all the histograms? https://codereview.chromium.org/2570893003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2570893003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:89: base::Histogram::FactoryGet(name, 1, 1000000, 100, \ How does this not leak memory? https://codereview.chromium.org/2570893003/diff/60001/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/2570893003/diff/60001/ui/events/latency_info.... ui/events/latency_info.h:91: INPUT_EVENT_LATENCY_TERMINATED_COMPONENT, Can we be a little more detailed what the reason behind the termination was? ie; NO_SWAP or something?
Regarding memory leaks - yeah, I think you're right. This appears to be a relatively common pattern though. e.g., https://cs.chromium.org/chromium/src/sql/connection.cc?rcl=0&l=398 Ilya, what's the correct approach here? I see a couple different macros being used to wrap calls to FactoryGet. https://codereview.chromium.org/2570893003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (left): https://codereview.chromium.org/2570893003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:413: if (latency.coalesced()) On 2016/12/15 14:54:49, dtapuska wrote: > Why are we adding back logging of coalesced events? Is it intentional? If so > then won't this change all the histograms? I'll move this to another patch, update histogram names etc, if this is the right thing to do. https://codereview.chromium.org/2570893003/diff/60001/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/2570893003/diff/60001/ui/events/latency_info.... ui/events/latency_info.h:91: INPUT_EVENT_LATENCY_TERMINATED_COMPONENT, On 2016/12/15 14:54:49, dtapuska wrote: > Can we be a little more detailed what the reason behind the termination was? > ie; NO_SWAP or something? Done.
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mojo lgtm
eakuefner@chromium.org changed reviewers: + benjhayden@chromium.org
+benjhayden, who's a more appropriate reviewer to answer the question posed to me.
Sorry, I'm having trouble understanding how this change might affect traces. Nothing in catapult references any of the components affected by this CL by name. The only related component that is referenced by catapult is INPUT_EVENT_LATENCY_TERMINATED_FRAME_SWAP_COMPONENT, which is unaffected by this change as far as I can tell. https://github.com/catapult-project/catapult/search?utf8=%E2%9C%93&q=INPUT_EV... lgtm unless there's something in particular about trace processing that you think might be affected by this change? I assume that input latency async events will continue to end at the same times as they currently do?
Are there any changes to what histogram data is recorded, or just changes to how the code is structured? (It's hard to tell from scanning the diff.) https://codereview.chromium.org/2570893003/diff/100001/content/browser/render... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2570893003/diff/100001/content/browser/render... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:91: ->Add((end.last_event_time - start.first_event_time).InMicroseconds()); What's the reason that you're switching away from histogram macros? And, if you're switching away from histogram macros, what's the reason to keep the outer macros? FWIW, base::Histogram::FactoryGet is less thread-safe than the UMA macros. And, if you do want to skip the macro, I'd recommend using the wrappers provided in histogram_functions.h.
Thanks Ben - I just wanted to make sure that grepping the catapult repo was adequate. I was worried that you might be relying on these terminating components somewhere. This patch has no functional changes. https://codereview.chromium.org/2570893003/diff/100001/content/browser/render... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2570893003/diff/100001/content/browser/render... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:91: ->Add((end.last_event_time - start.first_event_time).InMicroseconds()); On 2016/12/15 23:14:49, Ilya Sherman wrote: > What's the reason that you're switching away from histogram macros? And, if > you're switching away from histogram macros, what's the reason to keep the outer > macros? > > FWIW, base::Histogram::FactoryGet is less thread-safe than the UMA macros. And, > if you do want to skip the macro, I'd recommend using the wrappers provided in > histogram_functions.h. The reason to not use the standard macros is that we're creating the names dynamically, which makes this code _way_ more readable. The reason to not use the histogram_functions.h calls is the performance penalty. I'm trying to find something which isn't much slower than the previous approach, but lets me get rid of the lines and lines of hard to read repetitive histogram macros. We're calling these macros a bunch of times per event. I see a few approaches to dealing with this, for example: https://cs.chromium.org/chromium/src/ui/events/event.cc?rcl=0&l=67
https://codereview.chromium.org/2570893003/diff/100001/content/browser/render... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2570893003/diff/100001/content/browser/render... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:91: ->Add((end.last_event_time - start.first_event_time).InMicroseconds()); On 2016/12/16 13:48:53, tdresser wrote: > On 2016/12/15 23:14:49, Ilya Sherman wrote: > > What's the reason that you're switching away from histogram macros? And, if > > you're switching away from histogram macros, what's the reason to keep the > outer > > macros? > > > > FWIW, base::Histogram::FactoryGet is less thread-safe than the UMA macros. > And, > > if you do want to skip the macro, I'd recommend using the wrappers provided in > > histogram_functions.h. > > The reason to not use the standard macros is that we're creating the names > dynamically, which makes this code _way_ more readable. > > The reason to not use the histogram_functions.h calls is the performance > penalty. I'm trying to find something which isn't much slower than the previous > approach, but lets me get rid of the lines and lines of hard to read repetitive > histogram macros. Is this a measured performance penalty? If so, what was the observed penalty? I'm surprised that you'd see much difference between calling FactoryGet() directly and using histogram_functions.h. > We're calling these macros a bunch of times per event. I see a few approaches to > dealing with this, for example: > https://cs.chromium.org/chromium/src/ui/events/event.cc?rcl=0&l=67 If you really care about performance, then you should probably sacrifice readability in favor of using the most performant macros, which are the ones defined in histogram_macros.h. These macros allocate a static pointer, which is what gives them the performance boost. It's also what makes them incompatible with runtime-variable names.
The performance concerns were just based on the comments in histogram_functions.h. "These functions are slower compared to their macro equivalent because the histogram objects are not cached between calls. So, these shouldn't be used in performance critical code." https://cs.chromium.org/chromium/src/base/metrics/histogram_functions.h?rcl=0... I could probably write something that was significantly more readable than what we had before using macros or templates - it seems like we shouldn't need to pay that high a readability penalty to have something with acceptable performance. Do you think I should just use the methods in histogram_functions.h? What's your opinion on the approach taken in event.cc? (https://cs.chromium.org/chromium/src/ui/events/event.cc?rcl=0&l=67)
On 2016/12/19 14:17:59, tdresser - OOO until Jan 3 wrote: > The performance concerns were just based on the comments in > histogram_functions.h. > > "These functions are slower compared to their macro equivalent because the > histogram objects are not cached between calls. So, these shouldn't be used in > performance critical code." > > https://cs.chromium.org/chromium/src/base/metrics/histogram_functions.h?rcl=0... Yes, that matches what I said: the performance benefit of the histogram macros comes from caching the histogram pointer in a static local variable. The macros that you're defining do not do that, so they do not get that benefit. Sorry, I realize that it's fairly complicated and therefore confusing! > I could probably write something that was significantly more readable than what > we had before using macros or templates - it seems like we shouldn't need to pay > that high a readability penalty to have something with acceptable performance. > > Do you think I should just use the methods in histogram_functions.h? If you don't need the ultimate in blazing fast performance, then yes, the methods in histogram_functions are probably your best bet. OTOH if you're running this in performance-critical code, and it sounds like you are, then it probably makes sense to use the most efficient option. > What's your opinion on the approach taken in event.cc? > (https://cs.chromium.org/chromium/src/ui/events/event.cc?rcl=0&l=67) This approach seems pretty good to me, except that I'm somewhat confused about why the name is passed in twice (and why the docs have two different functions for getting the name). I guess I can't easily come up with a nice way to remove the redundant histogram name use, and it does look like it's only evaluated if DCHECK_IS_ON(), so it's probably a reasonable implementation.
On 2016/12/19 20:56:20, Ilya Sherman (Away De.29-Ja.4) wrote: > On 2016/12/19 14:17:59, tdresser - OOO until Jan 3 wrote: > > The performance concerns were just based on the comments in > > histogram_functions.h. > > > > "These functions are slower compared to their macro equivalent because the > > histogram objects are not cached between calls. So, these shouldn't be used in > > performance critical code." > > > > > https://cs.chromium.org/chromium/src/base/metrics/histogram_functions.h?rcl=0... > > Yes, that matches what I said: the performance benefit of the histogram macros > comes from caching the histogram pointer in a static local variable. The macros > that you're defining do not do that, so they do not get that benefit. Sorry, I > realize that it's fairly complicated and therefore confusing! > > > I could probably write something that was significantly more readable than > what > > we had before using macros or templates - it seems like we shouldn't need to > pay > > that high a readability penalty to have something with acceptable performance. > > > > Do you think I should just use the methods in histogram_functions.h? > > If you don't need the ultimate in blazing fast performance, then yes, the > methods in histogram_functions are probably your best bet. OTOH if you're > running this in performance-critical code, and it sounds like you are, then it > probably makes sense to use the most efficient option. > > > What's your opinion on the approach taken in event.cc? > > (https://cs.chromium.org/chromium/src/ui/events/event.cc?rcl=0&l=67) > > This approach seems pretty good to me, except that I'm somewhat confused about > why the name is passed in twice (and why the docs have two different functions > for getting the name). I guess I can't easily come up with a nice way to remove > the redundant histogram name use, and it does look like it's only evaluated if > DCHECK_IS_ON(), so it's probably a reasonable implementation. Would it be reasonable for me to move that implementation to histogram_macros.h?
On 2017/01/03 14:52:40, tdresser wrote: > On 2016/12/19 20:56:20, Ilya Sherman (Away De.29-Ja.4) wrote: > > On 2016/12/19 14:17:59, tdresser - OOO until Jan 3 wrote: > > > The performance concerns were just based on the comments in > > > histogram_functions.h. > > > > > > "These functions are slower compared to their macro equivalent because the > > > histogram objects are not cached between calls. So, these shouldn't be used > in > > > performance critical code." > > > > > > > > > https://cs.chromium.org/chromium/src/base/metrics/histogram_functions.h?rcl=0... > > > > Yes, that matches what I said: the performance benefit of the histogram macros > > comes from caching the histogram pointer in a static local variable. The > macros > > that you're defining do not do that, so they do not get that benefit. Sorry, > I > > realize that it's fairly complicated and therefore confusing! > > > > > I could probably write something that was significantly more readable than > > what > > > we had before using macros or templates - it seems like we shouldn't need to > > pay > > > that high a readability penalty to have something with acceptable > performance. > > > > > > Do you think I should just use the methods in histogram_functions.h? > > > > If you don't need the ultimate in blazing fast performance, then yes, the > > methods in histogram_functions are probably your best bet. OTOH if you're > > running this in performance-critical code, and it sounds like you are, then it > > probably makes sense to use the most efficient option. > > > > > What's your opinion on the approach taken in event.cc? > > > (https://cs.chromium.org/chromium/src/ui/events/event.cc?rcl=0&l=67) > > > > This approach seems pretty good to me, except that I'm somewhat confused about > > why the name is passed in twice (and why the docs have two different functions > > for getting the name). I guess I can't easily come up with a nice way to > remove > > the redundant histogram name use, and it does look like it's only evaluated if > > DCHECK_IS_ON(), so it's probably a reasonable implementation. > > Would it be reasonable for me to move that implementation to histogram_macros.h? Yeah, I think that sounds reasonable. Thanks!
tdresser@chromium.org changed reviewers: - eakuefner@chromium.org
In this patch, I've migrated everything over to histogram_functions instead of macros, and cleaned up the naming a bit more. This fixes the memory leak Dave pointed out, and keeps things readable, but does have some performance overhead. Using STATIC_HISTOGRAM_POINTER_GROUP is going to be a bit uglier than I anticipated (though better than just using raw histogram macros). Do we have a sense of what the performance impact of a change like this would be? Should I port everything over to the STATIC_HISTOGRAM_POINTER_GROUP?
On 2017/01/19 21:38:29, tdresser wrote: > In this patch, I've migrated everything over to histogram_functions instead of > macros, and cleaned up the naming a bit more. > This fixes the memory leak Dave pointed out, and keeps things readable, but does > have some performance overhead. > Using STATIC_HISTOGRAM_POINTER_GROUP is going to be a bit uglier than I > anticipated (though better than just using raw histogram macros). > Do we have a sense of what the performance impact of a change like this would > be? Histograms are intentionally leaked. Another way to phrase that is, the lifetime of a histogram is equal to the lifetime of the browser process. (Sorry for missing this question/discussion earlier!) > Should I port everything over to the STATIC_HISTOGRAM_POINTER_GROUP? Fine by me either way. I'll defer to you and others most familiar with this code on whether extra readability or extra performance is more valuable here.
I think I am going to migrate this over to STATIC_HISTOGRAM_POINTER group, but I might as well land this separately. Does this look reasonable?
https://codereview.chromium.org/2570893003/diff/160001/content/browser/render... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2570893003/diff/160001/content/browser/render... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:111: ->Add((end.last_event_time - start.first_event_time).InMilliseconds()); As I mentioned before, it's preferable to use functions from histogram_functions.h rather than calling base::Histogram::FactoryGet() directly.
https://codereview.chromium.org/2570893003/diff/160001/content/browser/render... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2570893003/diff/160001/content/browser/render... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:111: ->Add((end.last_event_time - start.first_event_time).InMilliseconds()); On 2017/01/23 20:02:33, Ilya Sherman wrote: > As I mentioned before, it's preferable to use functions from > histogram_functions.h rather than calling base::Histogram::FactoryGet() > directly. Oops, done. I'll be migrating everything away from the silly local macro definitions in a followup patch.
histogram changes lgtm, thanks
The CQ bit was checked by tdresser@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/2570893003/#ps180001 (title: "Avoid introducing new bare calls to FactoryGet.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485208166988720,
"parent_rev": "d1a069569c574fb053864890f1f181bcf02f8249", "commit_rev":
"fcda569a9fade182b0771194e91be0ab46fb7213"}
Message was sent while issue was closed.
Description was changed from ========== Clean up LatencyInfo and RWHLatencyTracker. May break trace processing? In preparation for landing keyboard event latency code, do some cleanup of the existing input latency logic. In particular, get rid of modality specific TERMINATED components. BUG=673731 ========== to ========== Clean up LatencyInfo and RWHLatencyTracker. May break trace processing? In preparation for landing keyboard event latency code, do some cleanup of the existing input latency logic. In particular, get rid of modality specific TERMINATED components. BUG=673731 Review-Url: https://codereview.chromium.org/2570893003 Cr-Commit-Position: refs/heads/master@{#445532} Committed: https://chromium.googlesource.com/chromium/src/+/fcda569a9fade182b0771194e91b... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/fcda569a9fade182b0771194e91b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
