[event-timing] Update INP calculator to support multiple event timings. [chromium/src : main]
Michal Mocny (Gerrit)
Michal Mocny added 3 comments![]( "Open in Gerrit")
Hey folks-- this is part 2 of the INP refactoring.
This should enable us to report multiple Event Timings, but the renderer still only sends one. The fact that the renderer hasn't changed de-risks this change in my oppinion and means we can make the change without a feature flag and delay deciding if we want to slow roll until the renderer-side change. (I think it will be easier to do it there, too).
But I'm open to suggestions otherwise, or how to simplify / test this to de-risk.
uint64_t& max_id = max_interaction_id_per_source_[source_token];Michal MocnyRight now this defaults to 0 for the first interaction with a new RenderFrameHost.
For this initial navigation on a page, this is the right assumption, but for soft-navs, and especially for soft-navs with iframes, the first id can be arbitrary.
I need to update to support this, perhaps by maintaining both a min and a max id, per renderer?
Done
// It might be that MergeAndClear can only ever "merge backwards", in whichMichal MocnyIf we knew 100% that `other_interactions` was strictly "newer" than we are, as would be the case if you "fork" and speculatively start to report a new soft-nav and then decide to abort instead.... we might be able to just move the data structures and overwrite our own.
But if that's the case, we could just overwrite the whole INP calculator.
So I tried to do a semi-decent merge for now.
Done-- but I think it only makes sense to merge two "adjacent" soft-navs from the same perf timeline together. You cannot merge two arbitrary lists and expect it to work.
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 2 comments![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_P(SoftNavigationTest, MAYBE_INP_ClickWithPresentation) {Michal MocnyThis change isn't strictly part of the refactoring, but I was given a presubmit check fail about it because I touched the file.
I think that we might want to try to re-enabled these tests, or just delete them ive we have better coverage elsewhere. I didn't audit the quality of the test itself, but it ran locally.
Ah I read the message more carefully and it just says that we should use the macro or delete the macro (above).
I think its safer to leave this test disabled, and I can skip the presubmit check to keep the patch clean -- or just delete the test(s).
@joha...@chromium.org I think you have the best grasp of the state of the tests and maybe can advice if these are uniquely useful or not?
event_timings_.erase(event_timings_.begin() + 10, event_timings_.end());Michal MocnyI dont recall if `it+10` is good style or should use `std::next(it, 10)`. Both are fine for vector....
Done
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
dibyapal: Just for OWNERS review of a trivial `browser/web_applications/` change (I removed the include from a header file that you were depending on somehow)
Related details
- Dibyajyoti Pal
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_P(SoftNavigationTest, MAYBE_INP_ClickWithPresentation) {Michal MocnyThis change isn't strictly part of the refactoring, but I was given a presubmit check fail about it because I touched the file.
I think that we might want to try to re-enabled these tests, or just delete them ive we have better coverage elsewhere. I didn't audit the quality of the test itself, but it ran locally.
Ah I read the message more carefully and it just says that we should use the macro or delete the macro (above).
I think its safer to leave this test disabled, and I can skip the presubmit check to keep the patch clean -- or just delete the test(s).
@joha...@chromium.org I think you have the best grasp of the state of the tests and maybe can advice if these are uniquely useful or not?
Yeah I wouldn't re-enable it in this patch since that could cause it to get reverted if it starts to flake.
Related details
- Dibyajyoti Pal
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_P(SoftNavigationTest, MAYBE_INP_ClickWithPresentation) {Michal MocnyThis change isn't strictly part of the refactoring, but I was given a presubmit check fail about it because I touched the file.
I think that we might want to try to re-enabled these tests, or just delete them ive we have better coverage elsewhere. I didn't audit the quality of the test itself, but it ran locally.
Scott HaseleyAh I read the message more carefully and it just says that we should use the macro or delete the macro (above).
I think its safer to leave this test disabled, and I can skip the presubmit check to keep the patch clean -- or just delete the test(s).
@joha...@chromium.org I think you have the best grasp of the state of the tests and maybe can advice if these are uniquely useful or not?
Yeah I wouldn't re-enable it in this patch since that could cause it to get reverted if it starts to flake.
Will just go back to what was there and skip checks, then we can clean up afterwards.
Related details
- Dibyajyoti Pal
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dibyajyoti Pal
| Code-Review | +1 |
1 comment
`c/b/web_applications/` LGTM, thanks for fixing iwyu.
Related details
- Johannes Henkel
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 6 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
6 comments
LGTM % a few comments/suggestions.
uint64_t min = std::numeric_limits<uint64_t>::max();It might be worth commenting at a high level how the interaction count is computing this, and the assumptions (I think just that interaction IDs are consecutive).
const content::RenderFrameHost* source,style nit: consider passing by const ref since `source` can't be null and it isn't stored past the life of the method.
mojom::EventTiming max_event;Q: Is the thought that this is small enough -- and will probably stay small/simple enough -- that copying makes sense? This version is probably better for memory locality when computing, so that's nice. Was just wondering thoughts behind changing from storing the pointer to copying.
struct InteractionEvents {Can this be private?
if (event.duration > interaction.max_event.duration) {Is it worth commenting why this happens? It's not clear/obvious to me why the duration for an existing event would change.
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +1 |
1 comment
Kicking off dry-run to check if this breaks anything.
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 6 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Code-Review | +1 |
6 comments
interaction_id_ranges_per_source_;Whereas the event_timings_ vector gets trimmed to at most 10, this map is only limited by the fact that there may not be so render frame hosts, is that right? Probably ok, but figure I should explicitly ask here. :-)
std::optional<mojom::EventTiming> ApproximateHighPercentile() const;
std::optional<mojom::EventTiming> worst_latency() const;I notice how both of these methods return mojom::EventTiming, which doesn't have the render frame host information. And, I'm wondering whether the other fields that are being returned here are useful: start_time and interaction id.
If only the duration is useful, then perhaps we could just return that?
If so, then the struct InteractionEvents could be made easier to read. It could have the stuff that's the identifying key (source_token and id), and the duration, and not carry the other stuff? Perhaps for a later refactoring? :-)
uint64_t index =Maybe the same in practice, though a compiler nitpicky person would use size_t for the [] in l. 26?
CompressEventTimings();nitpicky also: Maybe TrimEventTimings would be an even better name?
#include "third_party/blink/public/common/features.h"This added include feels a bit mysterious to me.
#include "third_party/blink/public/common/features.h"a second mention of features.h! :-)
Related details
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 9 comments![]( "Open in Gerrit")
Thanks for comments-- will address and upload shortly. Looking at the failing test in parallel
interaction_id_ranges_per_source_;Whereas the event_timings_ vector gets trimmed to at most 10, this map is only limited by the fact that there may not be so render frame hosts, is that right? Probably ok, but figure I should explicitly ask here. :-)
True, but we dont want to trim RenderFrameHost (i.e. "forget" about some subset of frames). And we already have a bunch of throttling for number of renderers, and a lot of data structures for them.
The reason we bound the number of interactions is just because its theoretically infinite. We probably should allow for more than 10 :P but that was borrowed from the JS suggestion.
(We could experiment with adding a histogram to see how often we exceed 10 and consider bumping that up. Alternatively, with soft-navs, we could consider removing the need for p98...)
std::optional<mojom::EventTiming> ApproximateHighPercentile() const;
std::optional<mojom::EventTiming> worst_latency() const;I notice how both of these methods return mojom::EventTiming, which doesn't have the render frame host information. And, I'm wondering whether the other fields that are being returned here are useful: start_time and interaction id.
If only the duration is useful, then perhaps we could just return that?
If so, then the struct InteractionEvents could be made easier to read. It could have the stuff that's the identifying key (source_token and id), and the duration, and not carry the other stuff? Perhaps for a later refactoring? :-)
I dont' remember if startTime is reporter, but InteractionId is used to report the InteractionOffset to UKM.
This ends up being a subtle bug when you have iframes: the INP interaction might be the first in an iframe but not the first with the page overall... but this is existing behaviour and worth fixing later.
---
The return value is the same it was before this patch, so I'm keen to leave it, but I think that we could return a Duration+Offset pair, or something. But right now we don't keep account of offset so it would take more work to add to this patch.
---
If so, then the struct InteractionEvents could be made easier to read.
I think this will always need to have RenderFrameHost id, Interaction Id, and Duration. So the only thing we can potentially get rid of is Interaction StartTime-- but I think its cleaner to just store the whole EventTiming.
const content::RenderFrameHost* source,style nit: consider passing by const ref since `source` can't be null and it isn't stored past the life of the method.
I was following existing style for page load metrics observers (e.g.: https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/page_load_metrics_observer.h;l=154-167;drc=ba8c2b489614ce6ee0f4d9109dbb19743b9c7c32)
This calculator of course could accept a dereferenced value but it was just consistency.
Not sure!
mojom::EventTiming max_event;Q: Is the thought that this is small enough -- and will probably stay small/simple enough -- that copying makes sense? This version is probably better for memory locality when computing, so that's nice. Was just wondering thoughts behind changing from storing the pointer to copying.
If I switch to Ptr, then the struct is no longer POD and needs its own constructor/destructor. Trivial enough to add. Then you also need to use a bunch of .Clone() calls in various places. Trivial enough to add.
But the values inside this EventTiming message are 3 numbers, so I just thought it was cleaner this way.
uint64_t index =Maybe the same in practice, though a compiler nitpicky person would use size_t for the [] in l. 26?
The type of index comes from the type of `num_user_interactions_`. Either way, there is a conversion going on here.
We could change the num_user_interactions_ type to `size_t`, but this is existing code and so I'll keep the patch focused on the goal!
if (event.duration > interaction.max_event.duration) {Is it worth commenting why this happens? It's not clear/obvious to me why the duration for an existing event would change.
I will add a comment-- but this isn't "an existing event changing" its a new event for the same interactionId.
So like, pointerdown with 100ms followed by pointerup with 200ms.
#include "third_party/blink/public/common/features.h"This added include feels a bit mysterious to me.
I mentioned it in earlier reviewer comments, but, the INP Calculator previously included this header even though it didn't need it. Over time, other cc files came to depend on it as an include, so when I removed it from the header these started to not compile.
These files should have "include what you use (IWYU)" so this is just a necessary include being added.
#include "third_party/blink/public/common/features.h"a second mention of features.h! :-)
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
CHECK(range.min != 0);This is the failing check in the bots tests. The test runs fine for me, locally, so its somehow flaky, I guess.
But the min value comes from the interactionid values that are sent to us. Some of the tests I already fixed where accidentally sending interaction id == 0 which is not a valid value.
I can move some enforcement to the AddEventTimings() function to ingore EventTimings with interaction id == 0 (which might be prudent in case renderer sends bad data)-- but I still don't see how it could *sometimes* happen in tests.
Investigating further.
Michal Mocny (Gerrit)
Michal Mocny added 6 comments![]( "Open in Gerrit")
uint64_t min = std::numeric_limits<uint64_t>::max();It might be worth commenting at a high level how the interaction count is computing this, and the assumptions (I think just that interaction IDs are consecutive).
Done
const content::RenderFrameHost* source,Michal Mocnystyle nit: consider passing by const ref since `source` can't be null and it isn't stored past the life of the method.
I was following existing style for page load metrics observers (e.g.: https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/page_load_metrics_observer.h;l=154-167;drc=ba8c2b489614ce6ee0f4d9109dbb19743b9c7c32)
This calculator of course could accept a dereferenced value but it was just consistency.
Not sure!
Done
struct InteractionEvents {Michal MocnyCan this be private?
Done
CompressEventTimings();nitpicky also: Maybe TrimEventTimings would be an even better name?
Done
if (event.duration > interaction.max_event.duration) {Michal MocnyIs it worth commenting why this happens? It's not clear/obvious to me why the duration for an existing event would change.
I will add a comment-- but this isn't "an existing event changing" its a new event for the same interactionId.
So like, pointerdown with 100ms followed by pointerup with 200ms.
Done
CHECK(range.min != 0);This is the failing check in the bots tests. The test runs fine for me, locally, so its somehow flaky, I guess.
But the min value comes from the interactionid values that are sent to us. Some of the tests I already fixed where accidentally sending interaction id == 0 which is not a valid value.
I can move some enforcement to the AddEventTimings() function to ingore EventTimings with interaction id == 0 (which might be prudent in case renderer sends bad data)-- but I still don't see how it could *sometimes* happen in tests.
Investigating further.
Okay-- I found that this test was indeed setting 0.
I dont know how it ran for me locally-- maybe CHECKS werent enforced or I wasn't running the test right...
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 2 comments![]( "Open in Gerrit")
if (event_timing->interaction_id == 0) {I won't add it to this patch, but this could be a good candidate for a UseCounter or adding to the existing PLMO error states histogram...
client_->OnPageEventTimingChanged(event_timings.size());Reviewers: This is one more new fix that I snuck in, the rest of the changes are mostly trivial updates after initial review.
This change here is because the `PageLoadMetricsUpdateDispatcher` was doing part of the job of the INP calculator and making assumptions about num interactions based on event timing vector size.
I think it would be even better to remove the num_interactions from this `OnPageEventTimingChanged` and force clients to get this value from the right INP calculator-- after all, they already get the INP value from this.
But I think this patch has sprawled enough and I wanted to stop somewhere !
Related details
- Dibyajyoti Pal
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |