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

Hovering the screenshot track can be slow #1532

Closed
mstange opened this issue Nov 28, 2018 · 4 comments · Fixed by #1588
Closed

Hovering the screenshot track can be slow #1532

mstange opened this issue Nov 28, 2018 · 4 comments · Fixed by #1588
Labels
assigned A developer has requested to work on this issue. good first issue Good issue for new contributors, the issue must have clear instructions on how to complete the work help wanted Things ready to be worked on by anyone. Issues must include instructions on how to complete the task perf Issues where the profiler itself is slow. ready Issue has defined requirements. It can be grabbed and worked on timeline Issues related to the header timeline view

Comments

@mstange
Copy link
Contributor

mstange commented Nov 28, 2018

STR:

  1. Load http://bit.ly/2DQ4wwy
  2. Move the mouse back and forth inside the screenshot track.

Expected results:
The page should update at 60fps and the screenshot should follow your cursor closely.

Actual results:
The frame rate is more like 20fps, and most of it is due to too much JS execution in perf.html.

Profile: http://bit.ly/2DNJajw

@mstange mstange added the perf Issues where the profiler itself is slow. label Nov 28, 2018
@julienw
Copy link
Contributor

julienw commented Nov 29, 2018

This is the react code updating the Screenshots component while moving the mouse. The problem is that the strip and the hover preview are both in the same component, so moving the mouse also rerender the full strip.

The solution (or part of the solution) would be to split this in 2 separate pure components:
https://github.com/devtools-html/perf.html/blob/68c36b3ef032598afc969f436da2e53108d66316/src/components/timeline/TrackScreenshots.js#L224-L225

Because the screenshot strip doesn't need the props that change as a result of the mousemove, it shouldn't rerender.

@julienw julienw added help wanted Things ready to be worked on by anyone. Issues must include instructions on how to complete the task good first issue Good issue for new contributors, the issue must have clear instructions on how to complete the work ready Issue has defined requirements. It can be grabbed and worked on timeline Issues related to the header timeline view labels Nov 29, 2018
@Rohanhacker
Copy link
Contributor

/claim

@Rohanhacker
Copy link
Contributor

I'd like to take this up

@julienw julienw added the assigned A developer has requested to work on this issue. label Jan 2, 2019
@julienw
Copy link
Contributor

julienw commented Jan 2, 2019

@Rohanhacker sure, please do! I added the label assigned to reflect that the issue isn't available anymore. You can find some basic information in our contributing documentation but feel free to ask here or on Slack if you need anything!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned A developer has requested to work on this issue. good first issue Good issue for new contributors, the issue must have clear instructions on how to complete the work help wanted Things ready to be worked on by anyone. Issues must include instructions on how to complete the task perf Issues where the profiler itself is slow. ready Issue has defined requirements. It can be grabbed and worked on timeline Issues related to the header timeline view
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants