Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UI] Race condition when clicking on a large track immediately before performing area selection #981

Open
cphlipot1 opened this issue Jan 8, 2025 · 4 comments
Labels
bug Something isn't working

Comments

@cphlipot1
Copy link
Contributor

It seems like on recent versions of the UI there is an issue where it takes the UI a long time to respond after clicking on a counter track with a large number of values. If the user performs an area selection after clicking the area selection will complete first, and then later the output will get overridden later with the single value selection.

Based on the symptoms like looks like a possible race condition that is being triggered by a recent regression is single counter value selection latency.

This issue seems to impact any trace that has a large counter track, but all of the simplest reproducers for this issues will require https://android-review.googlesource.com/c/platform/external/perfetto/+/3441062 in order not to crash

How to reproduce

  1. apply https://android-review.googlesource.com/c/platform/external/perfetto/+/3441062 locally
  2. build the UI from source and run the ui devserver
  3. open large_power_rail_trace.pftrace.gz in the UI
  4. expand the power rail tracks.
  5. click on one of the tracks, and then immediately perform an area selection on that track.
  6. Observe that when the UI finally responds a single counter value is selected instead of the area which was expected.

Note: it may not be obvious in the video, but i do left click (press & release) on the 2nd power rail track before i perform the area selection at about 1/2 second into the video

Screen.Recording.2025-01-07.at.4.06.10.PM.mov
@LalitMaganti
Copy link
Collaborator

I've asked @stevegolton to look at this. It may take us a bit of time to fix this though: we are in the process of overhauling how aggregation/search work.

@cphlipot1
Copy link
Contributor Author

Also just wanted to note this seems to impact stable, canary, and autopush for traces that are not impacted by #980

I don't I was very clear on that in the original issue description.

I've asked @stevegolton to look at this.

Thanks!

@cphlipot1
Copy link
Contributor Author

While the race condition would still be present, it looks like it could be significantly mitigated by optimizing the queries.

I put up https://android-review.googlesource.com/c/platform/external/perfetto/+/3439249 which should make the queries involved here run about 1 order of magnitude faster with the trace attached to this bug.

This does not make the race condition go away, but it makes it more likely for the first query to finish before the user can make a second click, and i think perf improvements here are worthwhile for UX regardless as single counter value selection would take ~10 sec on the attached trace without this CL.

primiano pushed a commit that referenced this issue Jan 9, 2025
Using subqueries to extract data for the prev/next item in a counter
series significatly out performs LEAD/LAG in this case by roughly
one order of magnitude. Update these queries to use this method so
that they perform better.

By optimizing these queries, we make it less likely for a user
to accidentally trigger the race condition outlined in
#981 athough the race
condidition is still present.

Change-Id: I07c537823f3a3bd729adf801f878605966e8c902
@LalitMaganti LalitMaganti added the bug Something isn't working label Jan 9, 2025
@stevegolton
Copy link
Member

primiano pushed a commit that referenced this issue Jan 10, 2025
This CL addresses two issues:

1. Avoid races when making selections:
- User selects a track event.
- Details start loading.
- User makes a different selection that loads quickly (i.e. area).
- Some time later the details finish loading and overwrite the latter
  selection.

2. Fixes the slight delay between clicking a track event and the
event being highlighted, which can be disconcerting and make the UI
feel unresponsive.

Note: This change makes the TrackEvent.details optional, as they can
take some time to load. Thus, any areas that use these details must
now deal with the fact that they can be possibly be undefined, but in
all cases we can do something sensible, so it's not a huge issue.

Bug: #981
Change-Id: I04a051e4df3e1fe3264fed543c31dfa768156697
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants