[discard] Add latency metric for Tab discarding [chromium/src : main]
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz voted and added 4 comments![]( "Open in Gerrit")
Votes added by Tom Lukaszewicz
| Commit-Queue | +1 |
4 comments
alexmos@ ptal //content
pmonette@ ptal //c/b/resource_coordinator
This CL adds Discarding.RetainedWebContents.DiscardLatency toTom Lukaszewicznit: `Discarding.TabLifecycleUnit.DiscardLatency UMA metrics`
Done
This metric is specific to the WebContentsDiscard feature andTom LukaszewiczCan we update this correspondingly?
Done
return false;Tom LukaszewiczIn this case, on_discarded_cb is never triggered.
I think we should either:
- make on_discarded_cb to accept bool. false if Discard is cancelled, true when the discard is completed.
- Add code comment to WebContents::Discard that on_discarded_cb is not guaranteed to be triggered.
Added the comment to WebContents::Discard() for the time being. Based on metrics this has a very small effect size (occurs 0.01% of the time).
Once WebContentsDiscard ships and the old codepath is removed it will be easier to return a bool to handle the exceptional circumstance.
Related details
- Alex Moshchuk
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz added 1 comment![]( "Open in Gerrit")
dcheng@ ptal //third_party/blink
dljames@ ptal histograms.xml
Related details
- Alex Moshchuk
- Daniel Cheng
- Darryl James
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
<histogram name="Discarding.TabLifecycleUnit.DiscardLatency" units="ms"This was probably discussed already but the description is split into 2 completely different types of metrics:
1) When the discarded tab has its WebContents replaced
2) When the discarded tab has its WebContents retained
Does it make sense to split this into 2 different metrics? Is there a reason we want to keep a single metric for now?
Related details
- Alex Moshchuk
- Daniel Cheng
- Patrick Monette
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted Code-Review+1![]( "Open in Gerrit")
- Alex Moshchuk
- Patrick Monette
- Tom Lukaszewicz
Patrick Monette (Gerrit)
Patrick Monette voted and added 1 comment![]( "Open in Gerrit")
Votes added by Patrick Monette
| Code-Review | +1 |
1 comment
chrome/browser/resource_coordinator lgtm
Related details
- Alex Moshchuk
- Tom Lukaszewicz
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz added 2 comments![]( "Open in Gerrit")
Avi could you take a look at //content/browser (alexmos@ to cc for OOO)
<histogram name="Discarding.TabLifecycleUnit.DiscardLatency" units="ms"This was probably discussed already but the description is split into 2 completely different types of metrics:
1) When the discarded tab has its WebContents replaced
2) When the discarded tab has its WebContents retainedDoes it make sense to split this into 2 different metrics? Is there a reason we want to keep a single metric for now?
I originally had this as two separate metrics (discussion above [here](https://chromium-review.googlesource.com/c/chromium/src/+/6791366/comment/f88c449f_bb02d892/)).
It aims to measure the same thing across both implementations - though the way the metric is implemented in both is different as you mentioned. Sharing the metric for both will make it easier to compare results during rollout (feature-gated, only one can be active at a given time).
Related details
- Alex Moshchuk
- Avi Drissman
- Darryl James
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 2 comments![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
2 comments
histograms.xml lgtm!
<histogram name="Discarding.TabLifecycleUnit.DiscardLatency" units="ms"Tom LukaszewiczThis was probably discussed already but the description is split into 2 completely different types of metrics:
1) When the discarded tab has its WebContents replaced
2) When the discarded tab has its WebContents retainedDoes it make sense to split this into 2 different metrics? Is there a reason we want to keep a single metric for now?
I originally had this as two separate metrics (discussion above [here](https://chromium-review.googlesource.com/c/chromium/src/+/6791366/comment/f88c449f_bb02d892/)).
It aims to measure the same thing across both implementations - though the way the metric is implemented in both is different as you mentioned. Sharing the metric for both will make it easier to compare results during rollout (feature-gated, only one can be active at a given time).
Ahhh got it - okay can we call that out in the metric, specifically that one or the other will be recorded based on a feature flag / experiment?
Related details
- Avi Drissman
- Tom Lukaszewicz
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz added 1 comment![]( "Open in Gerrit")
<histogram name="Discarding.TabLifecycleUnit.DiscardLatency" units="ms"Tom LukaszewiczThis was probably discussed already but the description is split into 2 completely different types of metrics:
1) When the discarded tab has its WebContents replaced
2) When the discarded tab has its WebContents retainedDoes it make sense to split this into 2 different metrics? Is there a reason we want to keep a single metric for now?
Darryl JamesI originally had this as two separate metrics (discussion above [here](https://chromium-review.googlesource.com/c/chromium/src/+/6791366/comment/f88c449f_bb02d892/)).
It aims to measure the same thing across both implementations - though the way the metric is implemented in both is different as you mentioned. Sharing the metric for both will make it easier to compare results during rollout (feature-gated, only one can be active at a given time).
Ahhh got it - okay can we call that out in the metric, specifically that one or the other will be recorded based on a feature flag / experiment?
Sure, done!
Related details
- Avi Drissman
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
1 comment
<histogram name="Discarding.TabLifecycleUnit.DiscardLatency" units="ms"Tom LukaszewiczThis was probably discussed already but the description is split into 2 completely different types of metrics:
1) When the discarded tab has its WebContents replaced
2) When the discarded tab has its WebContents retainedDoes it make sense to split this into 2 different metrics? Is there a reason we want to keep a single metric for now?
Darryl JamesI originally had this as two separate metrics (discussion above [here](https://chromium-review.googlesource.com/c/chromium/src/+/6791366/comment/f88c449f_bb02d892/)).
It aims to measure the same thing across both implementations - though the way the metric is implemented in both is different as you mentioned. Sharing the metric for both will make it easier to compare results during rollout (feature-gated, only one can be active at a given time).
Tom LukaszewiczAhhh got it - okay can we call that out in the metric, specifically that one or the other will be recorded based on a feature flag / experiment?
Sure, done!
- Avi Drissman
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted Code-Review+1![]( "Open in Gerrit")
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[discard] Add latency metric for Tab discarding
This CL adds Discarding.TabLifecycleUnit.DiscardLatency to
TabLifecycleUnit.
This metric measures discard latency for both the retained and
unretained WebContents discarding implementations.
- M chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
- M chrome/browser/resource_coordinator/tab_lifecycle_unit.h
- M chrome/browser/resource_coordinator/tab_manager_browsertest.cc
- M content/browser/renderer_host/frame_tree.cc
- M content/browser/renderer_host/frame_tree.h
- M content/browser/renderer_host/render_frame_host_impl.cc
- M content/browser/renderer_host/render_frame_host_impl.h
- M content/browser/web_contents/web_contents_impl.cc
- M content/browser/web_contents/web_contents_impl.h
- M content/browser/web_contents/web_contents_impl_browsertest.cc
- M content/public/browser/web_contents.h
- M third_party/blink/public/mojom/frame/frame.mojom
- M third_party/blink/renderer/core/frame/local_frame_mojo_handler.cc
- M third_party/blink/renderer/core/frame/local_frame_mojo_handler.h
- M tools/metrics/histograms/metadata/tab/histograms.xml
Code-Review: +1 by Daniel Cheng, +1 by Patrick Monette, +1 by Darryl James, +1 by Avi Drissman, +1 by Shin Kawamura
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |