Migrate scoped thread timers to base/metrics. [chromium/src : main]
Alex Turner (Gerrit)
Alex Turner added 5 comments![]( "Open in Gerrit")
Tanushree BishtWe already have some scoped timers in base/metrics, e.g.:
https://source.chromium.org/chromium/chromium/src/+/main:base/metrics/histogram_functions.h;drc=ce31a6f9e56b1ff3127b285f63022e095da04103;l=287
(And the corresponding macro).I didn't see a discussion of how this differs? I don't think we should have both, so if we think these other ones are better (in what way?), we should replace the use of that one with these. And they shouldn't be scattered across different files...
Alex TurnerThese ones are thread timers whereas those are wall timers that will replace macro usage. See context: https://issues.chromium.org/issues/336333963
And you bring up a good point actually in unifying these to the same files. Originally this change was made just to move everything to base/metrics so it could be used outside of the subresource filter component. Made the necessary adjustments by moving things to the histogram macros/functions files accordingly (in particular splitting up time_measurements.h/cc).
Yeah, I wonder if we should instead be expanding the existing base implementations to also allow for thread timers rather than copying over the subresource filter implementations.
ScopedUmaHistogramThreadTimer::ScopedUmaHistogramThreadTimer(Should we be trying to align this with how ScopedUmaHistogramTimer works other than the timing source being different?
#define IMPL_SCOPED_UMA_HISTOGRAM_TIMER_EXPANDER( \nit: maybe convert IMPL -> INTERNAL to match
#define IMPL_SCOPED_UMA_HISTOGRAM_TIMER_EXPANDER( \Hmm, this exactly matches the name of an above helper (other than IMPL vs INTERNAL). I think we should probably be either consolidating or changing naming to account for any differences.
namespace impl {Should we be keeping internal?
Related details
- Alexei Svitkine
- Mark Mentovai
- Robert Kaplow
- Tanushree Bisht
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tanushree Bisht (Gerrit)
Tanushree Bisht added 1 comment![]( "Open in Gerrit")
Tanushree BishtWe already have some scoped timers in base/metrics, e.g.:
https://source.chromium.org/chromium/chromium/src/+/main:base/metrics/histogram_functions.h;drc=ce31a6f9e56b1ff3127b285f63022e095da04103;l=287
(And the corresponding macro).I didn't see a discussion of how this differs? I don't think we should have both, so if we think these other ones are better (in what way?), we should replace the use of that one with these. And they shouldn't be scattered across different files...
Alex TurnerThese ones are thread timers whereas those are wall timers that will replace macro usage. See context: https://issues.chromium.org/issues/336333963
And you bring up a good point actually in unifying these to the same files. Originally this change was made just to move everything to base/metrics so it could be used outside of the subresource filter component. Made the necessary adjustments by moving things to the histogram macros/functions files accordingly (in particular splitting up time_measurements.h/cc).
Yeah, I wonder if we should instead be expanding the existing base implementations to also allow for thread timers rather than copying over the subresource filter implementations.
That sounds like a good idea. WDYT about having that change be a follow-up for later? If it sounds good, I can file a bug for it.
Currently, we want to prioritize changing the anti-fingerprinting call-sites to use these to obtain metrics. Once we have those, we can migrate to the consolidated implementation whenever it's ready?
Related details
- Alex Turner
- Alexei Svitkine
- Mark Mentovai
- Robert Kaplow
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Turner (Gerrit)
Alex Turner added 1 comment![]( "Open in Gerrit")
Tanushree BishtWe already have some scoped timers in base/metrics, e.g.:
https://source.chromium.org/chromium/chromium/src/+/main:base/metrics/histogram_functions.h;drc=ce31a6f9e56b1ff3127b285f63022e095da04103;l=287
(And the corresponding macro).I didn't see a discussion of how this differs? I don't think we should have both, so if we think these other ones are better (in what way?), we should replace the use of that one with these. And they shouldn't be scattered across different files...
Alex TurnerThese ones are thread timers whereas those are wall timers that will replace macro usage. See context: https://issues.chromium.org/issues/336333963
And you bring up a good point actually in unifying these to the same files. Originally this change was made just to move everything to base/metrics so it could be used outside of the subresource filter component. Made the necessary adjustments by moving things to the histogram macros/functions files accordingly (in particular splitting up time_measurements.h/cc).
Tanushree BishtYeah, I wonder if we should instead be expanding the existing base implementations to also allow for thread timers rather than copying over the subresource filter implementations.
That sounds like a good idea. WDYT about having that change be a follow-up for later? If it sounds good, I can file a bug for it.
Currently, we want to prioritize changing the anti-fingerprinting call-sites to use these to obtain metrics. Once we have those, we can migrate to the consolidated implementation whenever it's ready?
I think the fingerprinting protection filter should already be able to use these as they're in //components/subresource_filter/core. If so, I'm not sure we need to do this in two cls.
Related details
- Alexei Svitkine
- Mark Mentovai
- Robert Kaplow
- Tanushree Bisht
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tanushree Bisht (Gerrit)
Tanushree Bisht added 1 comment![]( "Open in Gerrit")
Tanushree BishtWe already have some scoped timers in base/metrics, e.g.:
https://source.chromium.org/chromium/chromium/src/+/main:base/metrics/histogram_functions.h;drc=ce31a6f9e56b1ff3127b285f63022e095da04103;l=287
(And the corresponding macro).I didn't see a discussion of how this differs? I don't think we should have both, so if we think these other ones are better (in what way?), we should replace the use of that one with these. And they shouldn't be scattered across different files...
Alex TurnerThese ones are thread timers whereas those are wall timers that will replace macro usage. See context: https://issues.chromium.org/issues/336333963
And you bring up a good point actually in unifying these to the same files. Originally this change was made just to move everything to base/metrics so it could be used outside of the subresource filter component. Made the necessary adjustments by moving things to the histogram macros/functions files accordingly (in particular splitting up time_measurements.h/cc).
Tanushree BishtYeah, I wonder if we should instead be expanding the existing base implementations to also allow for thread timers rather than copying over the subresource filter implementations.
Alex TurnerThat sounds like a good idea. WDYT about having that change be a follow-up for later? If it sounds good, I can file a bug for it.
Currently, we want to prioritize changing the anti-fingerprinting call-sites to use these to obtain metrics. Once we have those, we can migrate to the consolidated implementation whenever it's ready?
I think the fingerprinting protection filter should already be able to use these as they're in //components/subresource_filter/core. If so, I'm not sure we need to do this in two cls.
That's true. This CL was originally started with the goal of moving the timers to base/metrics in order to fulfill an old TODO.
Wanted to clarify some things:
As for your earlier comment in this thread, can you clarify whether "expanding the existing base implementations to also allow for thread timers rather than copying over the subresource filter implementations" maps to the other comment: "Should we be trying to align this with how ScopedUmaHistogramTimer works other than the timing source being different?"?
Or did you mean something else, like expanding the base implementations that existed prior to ScopedUmaHistogramTimer was introduced in https://chromium-review.googlesource.com/c/chromium/src/+/5552873 ?
Related details
- Alex Turner
- Alexei Svitkine
- Mark Mentovai
- Robert Kaplow
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Turner (Gerrit)
Alex Turner added 1 comment![]( "Open in Gerrit")
Tanushree BishtWe already have some scoped timers in base/metrics, e.g.:
https://source.chromium.org/chromium/chromium/src/+/main:base/metrics/histogram_functions.h;drc=ce31a6f9e56b1ff3127b285f63022e095da04103;l=287
(And the corresponding macro).I didn't see a discussion of how this differs? I don't think we should have both, so if we think these other ones are better (in what way?), we should replace the use of that one with these. And they shouldn't be scattered across different files...
Alex TurnerThese ones are thread timers whereas those are wall timers that will replace macro usage. See context: https://issues.chromium.org/issues/336333963
And you bring up a good point actually in unifying these to the same files. Originally this change was made just to move everything to base/metrics so it could be used outside of the subresource filter component. Made the necessary adjustments by moving things to the histogram macros/functions files accordingly (in particular splitting up time_measurements.h/cc).
Tanushree BishtYeah, I wonder if we should instead be expanding the existing base implementations to also allow for thread timers rather than copying over the subresource filter implementations.
Alex TurnerThat sounds like a good idea. WDYT about having that change be a follow-up for later? If it sounds good, I can file a bug for it.
Currently, we want to prioritize changing the anti-fingerprinting call-sites to use these to obtain metrics. Once we have those, we can migrate to the consolidated implementation whenever it's ready?
Tanushree BishtI think the fingerprinting protection filter should already be able to use these as they're in //components/subresource_filter/core. If so, I'm not sure we need to do this in two cls.
That's true. This CL was originally started with the goal of moving the timers to base/metrics in order to fulfill an old TODO.
Wanted to clarify some things:
As for your earlier comment in this thread, can you clarify whether "expanding the existing base implementations to also allow for thread timers rather than copying over the subresource filter implementations" maps to the other comment: "Should we be trying to align this with how ScopedUmaHistogramTimer works other than the timing source being different?"?
Or did you mean something else, like expanding the base implementations that existed prior to ScopedUmaHistogramTimer was introduced in https://chromium-review.googlesource.com/c/chromium/src/+/5552873 ?
That's true. This CL was originally started with the goal of moving the timers to base/metrics in order to fulfill an old TODO.
Wanted to clarify some things:
As for your earlier comment in this thread, can you clarify whether "expanding the existing base implementations to also allow for thread timers rather than copying over the subresource filter implementations" maps to the other comment: "Should we be trying to align this with how ScopedUmaHistogramTimer works other than the timing source being different?"?
Yep, exactly
Or did you mean something else, like expanding the base implementations that existed prior to ScopedUmaHistogramTimer was introduced in https://chromium-review.googlesource.com/c/chromium/src/+/5552873 ?
- Alexei Svitkine
- Mark Mentovai
- Robert Kaplow
- Tanushree Bisht
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tanushree Bisht (Gerrit)
Tanushree Bisht added 1 comment![]( "Open in Gerrit")
Pausing on this for now to focus on implementing other metrics as this is not top priority
Related details
- Alexei Svitkine
- Mark Mentovai
- Robert Kaplow
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Kaplow (Gerrit)
Robert Kaplow added 1 comment![]( "Open in Gerrit")
if this is paused and Alexei has been following, i'll let him review
Related details
- Alexei Svitkine
- Mark Mentovai
- Tanushree Bisht