#5176: fix await element performance regression #5177
Conversation
twschiller
changed the title
Codecov Report
@@ Coverage Diff @@
## main #5177 +/- ##
==========================================
- Coverage 61.18% 61.18% -0.01%
==========================================
Files 984 984
Lines 30440 30440
Branches 5870 5870
==========================================
- Hits 18626 18625 -1
- Misses 11814 11815 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -163,6 +163,7 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "@fortawesome/fontawesome-common-types": "^0.2.36", | |||
| "@shopify/jest-dom-mocks": "^4.1.0", | |||
There was a problem hiding this comment.
Includes mocks for requestIdleCallback. I think we could also use for location mock instead of the other we added a couple weeks ago
| if (isMatchinInProgress) { | ||
| return; | ||
| } | ||
|
|
||
| // Don't accumulate idle callbacks | ||
| if (idleCallbackHandle) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
| if (isMatchinInProgress) { | |
| return; | |
| } | |
| // Don't accumulate idle callbacks | |
| if (idleCallbackHandle) { | |
| return; | |
| } | |
| // Don't accumulate idle callbacks | |
| if (isMatchinInProgress || idleCallbackHandle) { | |
| return; | |
| } |
| // Wrap in requestIdleCallback to prevent impacting performance | ||
| idleCallbackHandle = requestIdleCallback( | ||
| () => { | ||
| requestAnimationFrame(() => { |
There was a problem hiding this comment.
We are probably good to go without the requestAnimationFrame inside requestIdleCallback. The callback of requestIdleCallback is only called when browser has no tasks in the queue.
There was a problem hiding this comment.
The callback of requestIdleCallback is only called when browser has no tasks in the queue.
It can also run in the timeout for requestIdleCallback is called. It waits a maximum of 150ms
| idleCallbackHandle = null; | ||
| }); | ||
| }, | ||
| { timeout: 150 } |
There was a problem hiding this comment.
What is the reason for the nested timeout? Should we just throttle for 150 milliseconds instead?
Personally, I struggle to predict how multiple timeouts will work, this make them hard for me to maintain and test.
There was a problem hiding this comment.
The original code throttled to calls every 150 ms.
Personally, I struggle to predict how multiple timeouts will work
Agree, I think multiple are to be avoided. The way to think about this code now is:
- Called every 150 ms
- But will only run if idle, or 150 ms, whichever occurs first
So when page is non-idle, runs at most every 300ms. The requestIdle timeout I added because I'm not confident how Chrome calculates idle, so wanted to have a stop gap
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
|
Can we compare the performance of |
I tested it on GitHub, and I at least perceived it to be faster. There's a couple other sites I'm going to try. There's a chance that the mutation observer itself is an issue on some sites (especially because of the subtree). If we're still seeing performance issues, could use the mutation observer to queue up elements to check We don't have a way of quantifying PixieBrix's impact on site performance yet. But definitely something I want to figure out how to add the in the future so we can warn users on inefficient selectors/triggers. Adding performance timings to the mutation observer and throttled whole-document check would likely be the way to go there |
|
Merging this in for additional testing as part of 1.7.19 QA |
What does this PR do?
Checklist