[Scroll Jank] Add a top-level single scroll event. [chromium/src : main]
Omar Elmekkawy (Gerrit)
Omar Elmekkawy added 3 comments![]( "Open in Gerrit")
Is the UKM data aggregated per session or per scroll? is the data we're getting from here comparable to UKM?
TRACE_EVENT_INSTANT(Can this be a normal trace event with the start and end times of the scroll?
final_frame_presentation_timestamp_ = base::TimeTicks::Now();in what cases will this be emitted without having the final presentation timestamp? last scroll?
Related details
- Harkiran Bolaria
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 4 comments![]( "Open in Gerrit")
Thanks for the review!
Is the UKM data aggregated per session or per scroll? is the data we're getting from here comparable to UKM?
UKM metrics are per scroll; we emit them/reset them on each new scroll, with exponential bucketing. This metric is subject to the usual UKM downsampling as well.
TRACE_EVENT_INSTANT(Can this be a normal trace event with the start and end times of the scroll?
Unfortunately there are a few cases where the start/ends of two consecutive scroll events overlap, and tracing doesn't appear to handle this well - e.g. the overlapping start/end are treated as a "mini" slice - see [screenshot](https://screenshot.googleplex.com/7NupmhiwseLDPrK) illustrating how this looks.
With the instant events, we can get the expected view using debug tracks.
final_frame_presentation_timestamp_ = base::TimeTicks::Now();in what cases will this be emitted without having the final presentation timestamp? last scroll?
- Omar Elmekkawy
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar Elmekkawy (Gerrit)
Omar Elmekkawy added 2 comments![]( "Open in Gerrit")
Harkiran BolariaIs the UKM data aggregated per session or per scroll? is the data we're getting from here comparable to UKM?
UKM metrics are per scroll; we emit them/reset them on each new scroll, with exponential bucketing. This metric is subject to the usual UKM downsampling as well.
Thanks.
TRACE_EVENT_INSTANT(Harkiran BolariaCan this be a normal trace event with the start and end times of the scroll?
Unfortunately there are a few cases where the start/ends of two consecutive scroll events overlap, and tracing doesn't appear to handle this well - e.g. the overlapping start/end are treated as a "mini" slice - see [screenshot](https://screenshot.googleplex.com/7NupmhiwseLDPrK) illustrating how this looks.
With the instant events, we can get the expected view using debug tracks.
+ @nus...@chromium.org for tracing expertise, is there any way around this?
Related details
- Harkiran Bolaria
- Stephen Nusko
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar Elmekkawy (Gerrit)
Omar Elmekkawy voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Harkiran Bolaria
- Stephen Nusko
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 1 comment![]( "Open in Gerrit")
TRACE_EVENT_INSTANT(Harkiran BolariaCan this be a normal trace event with the start and end times of the scroll?
Omar ElmekkawyUnfortunately there are a few cases where the start/ends of two consecutive scroll events overlap, and tracing doesn't appear to handle this well - e.g. the overlapping start/end are treated as a "mini" slice - see [screenshot](https://screenshot.googleplex.com/7NupmhiwseLDPrK) illustrating how this looks.
With the instant events, we can get the expected view using debug tracks.
+ @nus...@chromium.org for tracing expertise, is there any way around this?
Just spoke with Stephen offline, we can achieve this by doing what EventLatency does, rather than separate instant events. I will re-upload once I've tested/implemented this way.
Related details
- Harkiran Bolaria
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria voted and added 1 comment![]( "Open in Gerrit")
Votes added by Harkiran Bolaria
| Commit-Queue | +1 |
1 comment
Harkiran BolariaCan this be a normal trace event with the start and end times of the scroll?
Omar ElmekkawyUnfortunately there are a few cases where the start/ends of two consecutive scroll events overlap, and tracing doesn't appear to handle this well - e.g. the overlapping start/end are treated as a "mini" slice - see [screenshot](https://screenshot.googleplex.com/7NupmhiwseLDPrK) illustrating how this looks.
With the instant events, we can get the expected view using debug tracks.
Harkiran Bolaria+ @nus...@chromium.org for tracing expertise, is there any way around this?
Just spoke with Stephen offline, we can achieve this by doing what EventLatency does, rather than separate instant events. I will re-upload once I've tested/implemented this way.
Fixed using EventLatency as an example: https://screenshot.googleplex.com/NcmNg8JnWATzjEU
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 4 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
4 comments
WriteScrollTraceEvent();Does UKM ever not get called when something like our scroll jank metric tracker would be? I.E. should this be in the dropped frame tracker instead? If they both get called always then this is fine since it has all the UKM data, but if there are cases where it isn't called the other spot might be better.
// Use instant trace events and join in SQL based on the common frame
// event latency id, as on a few occasions, the end timestamp will occur
// after the next scroll starts.Comment needs updating.
perfetto::Track(base::trace_event::GetNextGlobalTraceId());If you want
you can name this track as described [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/perfetto/include/perfetto/tracing/track.h;l=65-69;drc=1286fac3db4268900d4683fd4f4e00a187fd92ff)
```
// auto desc = track.Serialize();
// desc.set_name("MyTrack");
// perfetto::TrackEvent::SetTrackDescriptor(track, desc);
```
And then you can continue to emit `Scroll ##` if you want.
I believe that works in the UI for track collapsing, but perhaps it doesn't so please double check me on that.
~ScrollTracingBrowserTest() override {}use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
Please fix.
Related details
- Harkiran Bolaria
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 5 comments![]( "Open in Gerrit")
Addressed all comments, once the try-bots confirm, I will add owners for final approvals.
Does UKM ever not get called when something like our scroll jank metric tracker would be? I.E. should this be in the dropped frame tracker instead? If they both get called always then this is fine since it has all the UKM data, but if there are cases where it isn't called the other spot might be better.
These are both called at the same time [here](https://source.chromium.org/chromium/chromium/src/+/main:cc/metrics/compositor_frame_reporter.cc;l=1620;drc=b6a0720ebf2f236e9ab95c65b9023b657c77e6f3).
And to confirm, a [screenshot](https://screenshot.googleplex.com/8fKjsc4jcSCdP8Q) showing when ScrollJankDroppedFrameTracker::OnScrollStarted and ScrollJankUkmReporter::WriteScrollTraceEvent are called, recording all details of the previous scroll.
// Use instant trace events and join in SQL based on the common frame
// event latency id, as on a few occasions, the end timestamp will occur
// after the next scroll starts.Harkiran BolariaComment needs updating.
Done
perfetto::Track(base::trace_event::GetNextGlobalTraceId());If you want
you can name this track as described [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/perfetto/include/perfetto/tracing/track.h;l=65-69;drc=1286fac3db4268900d4683fd4f4e00a187fd92ff)
```
// auto desc = track.Serialize();
// desc.set_name("MyTrack");
// perfetto::TrackEvent::SetTrackDescriptor(track, desc);
```And then you can continue to emit `Scroll ##` if you want.
I believe that works in the UI for track collapsing, but perhaps it doesn't so please double check me on that.
It looks like this use case is deprecated per the code [here](https://cs.android.com/android/platform/superproject/main/+/main:external/perfetto/include/perfetto/tracing/internal/track_event_data_source.h;l=507;drc=074bb2dc367bde42b837a8de4d4e88f26c074c32;bpv=1;bpt=1) and the documentation might be out of date.
Scroll ID was also an arbitrary ID I added based on the first event latency id, and each slice will have an id, so I think this version of the implementation, similar to EventLatency would be sufficient.
use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
Please fix.
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 3 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
3 comments
Note that recording a single TRACE_EVENT_BEGIN/TRACE_EVENT_END
event is not processed correctly in cases where different scroll
events have overlapping start/ends. Use event latency id to
join start and end timestamps to determine the actual scroll
duration.Does this need updating?
optional int64 first_event_latency_id = 1;What is this event for?
ValidateUkm(
url, ukm::builders::Event_Scroll::kEntryName,
{
{ukm::builders::Event_Scroll::kFrameCountName,
ConvertToHistogramValue(scroll_metrics[0])},
{ukm::builders::Event_Scroll::kVsyncCountName,
ConvertToHistogramValue(scroll_metrics[1])},
{ukm::builders::Event_Scroll::kScrollJank_MissedVsyncsMaxName,
ConvertToHistogramValue(scroll_metrics[2])},
{ukm::builders::Event_Scroll::kScrollJank_MissedVsyncsSumName,
ConvertToHistogramValue(scroll_metrics[3])},
{ukm::builders::Event_Scroll::kScrollJank_DelayedFrameCountName,
ConvertToHistogramValue(scroll_metrics[4])},
{ukm::builders::Event_Scroll::kPredictorJankyFrameCountName,
ConvertToHistogramValue(scroll_metrics[5])},
});this is strange, we have two computed things compared against each other, one of them should be compared against the real expected value as well.
I.E.
how many frame counts, if they all become zero because some bug nothing will catch that.
So I would do a
EXPECT_THAT(scroll_metrics, ....); expecting on real values. And then do this ValidateUkm using scroll_metrics we have verified.
Related details
- Harkiran Bolaria
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria voted and added 1 comment![]( "Open in Gerrit")
Votes added by Harkiran Bolaria
| Commit-Queue | +1 |
1 comment
Adding Jon for owners approval, we are just adding a trace event to be recorded whenever we record UKM for scrolls/janks from the compositor. The original patch adding this is available [here](https://chromium-review.googlesource.com/c/chromium/src/+/5088868) for context.
Related details
- Harkiran Bolaria
- Jonathan Ross
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria voted and added 2 comments![]( "Open in Gerrit")
Votes added by Harkiran Bolaria
| Commit-Queue | +1 |
2 comments
Note that recording a single TRACE_EVENT_BEGIN/TRACE_EVENT_END
event is not processed correctly in cases where different scroll
events have overlapping start/ends. Use event latency id to
join start and end timestamps to determine the actual scroll
duration.Harkiran BolariaDoes this need updating?
Done
ValidateUkm(
url, ukm::builders::Event_Scroll::kEntryName,
{
{ukm::builders::Event_Scroll::kFrameCountName,
ConvertToHistogramValue(scroll_metrics[0])},
{ukm::builders::Event_Scroll::kVsyncCountName,
ConvertToHistogramValue(scroll_metrics[1])},
{ukm::builders::Event_Scroll::kScrollJank_MissedVsyncsMaxName,
ConvertToHistogramValue(scroll_metrics[2])},
{ukm::builders::Event_Scroll::kScrollJank_MissedVsyncsSumName,
ConvertToHistogramValue(scroll_metrics[3])},
{ukm::builders::Event_Scroll::kScrollJank_DelayedFrameCountName,
ConvertToHistogramValue(scroll_metrics[4])},
{ukm::builders::Event_Scroll::kPredictorJankyFrameCountName,
ConvertToHistogramValue(scroll_metrics[5])},
});this is strange, we have two computed things compared against each other, one of them should be compared against the real expected value as well.
I.E.
how many frame counts, if they all become zero because some bug nothing will catch that.
So I would do a
EXPECT_THAT(scroll_metrics, ....); expecting on real values. And then do this ValidateUkm using scroll_metrics we have verified.
For context: this was the methodology requested by Alexander for testing CUI (see crrev.com/c/5307374 and crrev.com/c/5307703 for parity matching), rather than checking the actual values. However I will update the patch here with checks for the actual values.
Related details
- Jonathan Ross
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 2 comments![]( "Open in Gerrit")
What is this event for?
Initially used to join instant events, removed now.
ValidateUkm(
url, ukm::builders::Event_Scroll::kEntryName,
{
{ukm::builders::Event_Scroll::kFrameCountName,
ConvertToHistogramValue(scroll_metrics[0])},
{ukm::builders::Event_Scroll::kVsyncCountName,
ConvertToHistogramValue(scroll_metrics[1])},
{ukm::builders::Event_Scroll::kScrollJank_MissedVsyncsMaxName,
ConvertToHistogramValue(scroll_metrics[2])},
{ukm::builders::Event_Scroll::kScrollJank_MissedVsyncsSumName,
ConvertToHistogramValue(scroll_metrics[3])},
{ukm::builders::Event_Scroll::kScrollJank_DelayedFrameCountName,
ConvertToHistogramValue(scroll_metrics[4])},
{ukm::builders::Event_Scroll::kPredictorJankyFrameCountName,
ConvertToHistogramValue(scroll_metrics[5])},
});Harkiran Bolariathis is strange, we have two computed things compared against each other, one of them should be compared against the real expected value as well.
I.E.
how many frame counts, if they all become zero because some bug nothing will catch that.
So I would do a
EXPECT_THAT(scroll_metrics, ....); expecting on real values. And then do this ValidateUkm using scroll_metrics we have verified.
For context: this was the methodology requested by Alexander for testing CUI (see crrev.com/c/5307374 and crrev.com/c/5307703 for parity matching), rather than checking the actual values. However I will update the patch here with checks for the actual values.
I found that exact values weren't always consistent across devices or test runs, so I have implemented as close to exact matching as possible, with checks of approximate values instead. Will verify against the try bots to confirm if this works.
Related details
- Jonathan Ross
- Stephen Nusko
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 1 comment![]( "Open in Gerrit")
Investigating flake failures and will update accordingly.
Related details
- Jonathan Ross
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 2 comments![]( "Open in Gerrit")
Investigating flake failures and will update accordingly.
Done
ValidateUkm(
url, ukm::builders::Event_Scroll::kEntryName,
{
{ukm::builders::Event_Scroll::kFrameCountName,
ConvertToHistogramValue(scroll_metrics[0])},
{ukm::builders::Event_Scroll::kVsyncCountName,
ConvertToHistogramValue(scroll_metrics[1])},
{ukm::builders::Event_Scroll::kScrollJank_MissedVsyncsMaxName,
ConvertToHistogramValue(scroll_metrics[2])},
{ukm::builders::Event_Scroll::kScrollJank_MissedVsyncsSumName,
ConvertToHistogramValue(scroll_metrics[3])},
{ukm::builders::Event_Scroll::kScrollJank_DelayedFrameCountName,
ConvertToHistogramValue(scroll_metrics[4])},
{ukm::builders::Event_Scroll::kPredictorJankyFrameCountName,
ConvertToHistogramValue(scroll_metrics[5])},
});Harkiran Bolariathis is strange, we have two computed things compared against each other, one of them should be compared against the real expected value as well.
I.E.
how many frame counts, if they all become zero because some bug nothing will catch that.
So I would do a
EXPECT_THAT(scroll_metrics, ....); expecting on real values. And then do this ValidateUkm using scroll_metrics we have verified.
Harkiran BolariaFor context: this was the methodology requested by Alexander for testing CUI (see crrev.com/c/5307374 and crrev.com/c/5307703 for parity matching), rather than checking the actual values. However I will update the patch here with checks for the actual values.
I found that exact values weren't always consistent across devices or test runs, so I have implemented as close to exact matching as possible, with checks of approximate values instead. Will verify against the try bots to confirm if this works.
Done
Related details
- Jonathan Ross
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 3 comments![]( "Open in Gerrit")
void EmitPredictorJankUkm();Nit: move to private since new `UpdateLatestFrameAndEmitPredictorJank` will control emitting
if (first_frame_timestamp_ != base::TimeTicks::Min()) {If we have no timings, should we perform the rest of `EmitScrollJankUkm`?
if (final_frame_presentation_timestamp_ == base::TimeTicks::Min()) {Should this be a check? Or an early exit?
Related details
- Harkiran Bolaria
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 2 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
2 comments
final_frame_presentation_timestamp_ = latest_timestamp;Does this need to be a member variable? Could we just pass it to WriteScrollTraceEvent() right here?
EXPECT_GE(ConvertToHistogramValue(scroll_metrics[2]), 0);
EXPECT_LE(ConvertToHistogramValue(scroll_metrics[2]), 1);Lets introduce a jank here so we can go EXPECT_GE 1 and that way this test ensures we're tracking something.
I'll do some code diving but I think we can create a synthetic Gesture Controller ourselves and then inject explicitly the timestamps to ensure we hit a jank.
I talked a bit offline with you, but will also try to get you some code links tomorrow.
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 5 comments![]( "Open in Gerrit")
Partially addressed review comments; updated tests to be included in next upload, when I will add reviewers back to attention set.
Nit: move to private since new `UpdateLatestFrameAndEmitPredictorJank` will control emitting
Done
final_frame_presentation_timestamp_ = latest_timestamp;Does this need to be a member variable? Could we just pass it to WriteScrollTraceEvent() right here?
I think we'd want to keep it, as we call WriteScrollTraceEvent in EmitScrollJankUkm, which is called in a few other places where we won't necessarily know the latest frame timestamp without caching it (e.g. in the destructor)
if (first_frame_timestamp_ != base::TimeTicks::Min()) {If we have no timings, should we perform the rest of `EmitScrollJankUkm`?
No, we shouldn't; updated accordingly. For the first scroll of the session, there wouldn't really be any previous metrics we'd want to track (the only case where either timestamp value would be null.
if (final_frame_presentation_timestamp_ == base::TimeTicks::Min()) {Should this be a check? Or an early exit?
Addressed as with other comment on first_frame_timetamp_. Thanks!
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 2 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
2 comments
SyntheticSmoothScrollGestureParams params;
params.gesture_source_type = source;
params.anchor = gfx::PointF(point);
params.distances.push_back(-distance);
params.granularity = ui::ScrollGranularity::kScrollByPrecisePixel;
auto gesture = std::make_unique<SyntheticSmoothScrollGesture>(params);
// Runs until we get the SyntheticGestureCompleted callback
base::RunLoop run_loop;
GetRenderWidgetHostImpl()->QueueSyntheticGesture(
std::move(gesture),
base::BindLambdaForTesting([&](SyntheticGesture::Result result) {
EXPECT_EQ(SyntheticGesture::GESTURE_FINISHED, result);
run_loop.Quit();
}));
run_loop.Run();Okay getting back to this,
I think we can create a DoScroll function that is a bit more elaborate but then allows us to make it take a timestamp and a delta. Then we can generate a jank 100%
```
Position start = <position>
Event1 = std::pair<std::time, delta>{base::time::now(), -5}
Event2 = std::pair<std::time, delta>{event1.first + base::Milliseconds(5), -5}
DoScroll(start, {event1, event2});
DoScroll(Position start, vector<...> events) {
auto controller =
std::make_unique<SyntheticGestureController>(
GetRenderWidgetHostImpl(),
GetRenderWidgetHostImpl()->GetView()->CreateSyntheticGestureTarget(),
SequencedTaskRunner::GetCurrentDefault());
for (each event) {
// construct a gesture
controler->QueueSyntheticGesture(std::move(current_gesture), gesture_callback);
}
for (each event) {
DispatchNextEvent(event.first) // fire it with a inserted timestamp)
wait_on_gesture_callback // Or GiveItSomeTime();
}
}
```
Since we wait in the last for loop the event time will be in the past but we'll be after the presentation representing a jank.
EXPECT_GE(ConvertToHistogramValue(scroll_metrics[2]), 0);
EXPECT_LE(ConvertToHistogramValue(scroll_metrics[2]), 1);Lets introduce a jank here so we can go EXPECT_GE 1 and that way this test ensures we're tracking something.
I'll do some code diving but I think we can create a synthetic Gesture Controller ourselves and then inject explicitly the timestamps to ensure we hit a jank.
I talked a bit offline with you, but will also try to get you some code links tomorrow.
Okay getting back to this,
I think we can create a DoScroll function that is a bit more elaborate but then allows us to make it take a timestamp and a delta. Then we can generate a jank 100%
```
Position start = <position>
Event1 = std::pair<std::time, delta>{base::time::now(), -5}
Event2 = std::pair<std::time, delta>{base::time::now(), -5}
DoScroll(start, {event1, event2});
DoScroll(Position start, vector<...> events) {
auto controller =
std::make_unique<SyntheticGestureController>(
GetRenderWidgetHostImpl(),
GetRenderWidgetHostImpl()->GetView()->CreateSyntheticGestureTarget(),
SequencedTaskRunner::GetCurrentDefault());
}
```
Related details
- Harkiran Bolaria
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
final_frame_presentation_timestamp_ = latest_timestamp;This is only used in the WriteScrollTraceEvent method, Could we just pass in latest_timestamp to WriteScrollTraceEvent here in this method?
Or pass it to EmitPredictorJankUkm and then even the test can give the final presentation timestamp.
Related details
- Harkiran Bolaria
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 3 comments![]( "Open in Gerrit")
Partially addressed comments, will run tests against bots to see if changes to browser test fully work; will address remaining comments shortly
I have updated the implementation such that a jank is always present, and updated the checks accordingly. For some additional context on the current implementation:
- At least 1 frame is always dropped; however on rare occasions we may have more than one. As discussed, I have retained a check, GE(1) for all fields.
- I tried setting a field on the synthetic gesture itself to be used as the dispatch timestamp - however, due to the implementation of SyntheticGestureController and the process of dispatching events - dispatch is called more frequently (and we end up having multiple gestures in the queue being updated at with the same timestamps). Hence, I've implemented this so that our dispatch timestamp is on the order of inputs dispatch calls.
- We need to account for bucketization of UKM values, rather than the raw trace values, as this is applied when we write to UKM.
Will mark as resolved if this is consistent across the bots.
EXPECT_GE(ConvertToHistogramValue(scroll_metrics[2]), 0);
EXPECT_LE(ConvertToHistogramValue(scroll_metrics[2]), 1);Stephen NuskoLets introduce a jank here so we can go EXPECT_GE 1 and that way this test ensures we're tracking something.
I'll do some code diving but I think we can create a synthetic Gesture Controller ourselves and then inject explicitly the timestamps to ensure we hit a jank.
I talked a bit offline with you, but will also try to get you some code links tomorrow.
Okay getting back to this,
I think we can create a DoScroll function that is a bit more elaborate but then allows us to make it take a timestamp and a delta. Then we can generate a jank 100%
```
Position start = <position>
Event1 = std::pair<std::time, delta>{base::time::now(), -5}
Event2 = std::pair<std::time, delta>{base::time::now(), -5}DoScroll(start, {event1, event2});
DoScroll(Position start, vector<...> events) {
auto controller =
std::make_unique<SyntheticGestureController>(
GetRenderWidgetHostImpl(),
GetRenderWidgetHostImpl()->GetView()->CreateSyntheticGestureTarget(),
SequencedTaskRunner::GetCurrentDefault());
}
```
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 1 comment![]( "Open in Gerrit")
final_frame_presentation_timestamp_ = latest_timestamp;This is only used in the WriteScrollTraceEvent method, Could we just pass in latest_timestamp to WriteScrollTraceEvent here in this method?
Or pass it to EmitPredictorJankUkm and then even the test can give the final presentation timestamp.
The issue would be in the destructor of ScrollJankUkmReporter - we call WriteScrollTraceEvent from the destructor via EmitScrollJankUkm, and we wouldn't know the latest frame timestamp from here without direct access to the latest frame otherwise.
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 1 comment![]( "Open in Gerrit")
Updated for specific scrolling points/distances for certain architectures. Verifying against bots.
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 2 comments![]( "Open in Gerrit")
Updated testing so that we only use smoke tests, rather than full browser testing - see other resolved comment. Will add reviewers back to attention set once bots pass.
SyntheticSmoothScrollGestureParams params;
params.gesture_source_type = source;
params.anchor = gfx::PointF(point);
params.distances.push_back(-distance);
params.granularity = ui::ScrollGranularity::kScrollByPrecisePixel;
auto gesture = std::make_unique<SyntheticSmoothScrollGesture>(params);
// Runs until we get the SyntheticGestureCompleted callback
base::RunLoop run_loop;
GetRenderWidgetHostImpl()->QueueSyntheticGesture(
std::move(gesture),
base::BindLambdaForTesting([&](SyntheticGesture::Result result) {
EXPECT_EQ(SyntheticGesture::GESTURE_FINISHED, result);
run_loop.Quit();
}));
run_loop.Run();From offline conversation - we have opted for smoke tests here as it has been challenging to debug/replicate the failures on bots.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 1 comment![]( "Open in Gerrit")
Adding reviewers back to attention set, as we've opted for smoke tests rather than full browser testing.
Related details
- Jonathan Ross
- Stephen Nusko
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
1 comment
auto controller = std::make_unique<ScrollDelaySyntheticGestureController>(Since we're only doing a smoke test do we need our own controller now?
Related details
- Harkiran Bolaria
- Jonathan Ross
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harkiran Bolaria (Gerrit)
Harkiran Bolaria added 1 comment![]( "Open in Gerrit")
auto controller = std::make_unique<ScrollDelaySyntheticGestureController>(Since we're only doing a smoke test do we need our own controller now?
This is correct, updated accordingly, thanks for catching.
Related details
- Jonathan Ross
- Stephen Nusko
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 5 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
5 comments
auto controller = std::make_unique<SyntheticGestureController>(
GetRenderWidgetHostImpl(),
GetRenderWidgetHostImpl()->GetView()->CreateSyntheticGestureTarget(),
base::SequencedTaskRunner::GetCurrentDefault());Do we need this controller at all?
[GetRenderWidgetHostImpl()->QueueSyntheticGesture](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_widget_host_impl.h;l=602;drc=3c0517dbb233391c728684395d9aebb099665a09) creates a GestureController as needed.
// params.speed_in_pixels_s = 2;
// params.input_event_pattern =
// content::mojom::InputEventPattern::kEveryOtherVsync;Either remove or leave a comment mentioning these params in a TODO of some sort.
// NOTE: Mac doesn't support touch events, and will not record scrolls withcomment says mac, but build flag says only android. Perhaps change the comment to state
// Scroll with 3 updates to ensure:
// 1) One frame is missed (maximum 2 may be missed).
// 2) Predictor jank occurs.
#if defined(ARCH_CPU_X86_FAMILY) || defined(ARCH_CPU_ARM64)
DoScroll(gfx::Point(10, 10), {gfx::Vector2d(0, 100), gfx::Vector2d(0, -100)},
content::mojom::GestureSourceType::kTouchInput);
#else
DoScroll(gfx::Point(0, 10), {gfx::Vector2d(0, 15), gfx::Vector2d(0, -10)},
content::mojom::GestureSourceType::kTouchInput);
#endif
Do we need this build config check if we're not trying to address the flakiness of the bots?
// frame_count
EXPECT_GE(ConvertToHistogramValue(scroll_metrics[0]), 0);
// vsync_count
EXPECT_GE(ConvertToHistogramValue(scroll_metrics[1]), 0);
// missed_vsync_max
EXPECT_GE(ConvertToHistogramValue(scroll_metrics[2]), 0);
// missed_vsync_sum
EXPECT_GE(ConvertToHistogramValue(scroll_metrics[3]), 0);
// delayed_frame_count
EXPECT_GE(ConvertToHistogramValue(scroll_metrics[4]), 0);
// predictor_janky_frame_count.
EXPECT_GE(ConvertToHistogramValue(scroll_metrics[5]), 0);Anything GE to zero doesn't make sense to check (it fails or passes in all cases).
however Frame_cout and vsync_count should always be >= 1 right?
Related details
- Harkiran Bolaria
- Jonathan Ross
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |