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

Introduce use-optimized-selector #1226

Closed
wants to merge 3 commits into from

Conversation

Rheeseyb
Copy link
Contributor

@Rheeseyb Rheeseyb commented May 7, 2021

Part way to a solution to #1186

Problem:
We're still stuck on an old build of use-context-selector as we're relying on a patch of that for adding an equality check param.

Fix:
Introduce use-optimized-selector for applying a layer of caching around selectors used by the above, as discussed in this comment thread. I'm opening this PR primarily to see how the performance tests run with this, since it completely removes the passing of that equality function to the patched build of use-context-selector.

I have also tested this out with the latest version of use-context-selector (1.3.7), which pushes the render count tests up again, so there is still further work to be done to allow us to move onto the latest version of that.

Commit Details:

  • Added a new function useContextSelector to react-performance.ts, which combines the above two libraries
  • Point all uses of use-context-selector to the above function
  • Ensure each selector function is either a constant or is memoised (as is required)

Annoyingly, the react-hooks/exhaustive-deps eslint rule was tripped in a few cases by generic type symbols (e.g. in useGetMultiselectedProps the rule complains that P is not included in the list of dependencies, which is nonsense!), so I've had to disable those on a few lines. I'll check to see if there is an update for that rule to fix the issue.

@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2021

Staging deploy success. Link to test editor: https://utopia.pizza/p/?branch_name=performance-use-optimized-selector
Performance: "There was an error with Puppeteer: Error – Error during measurements TimeoutError: waiting for XPath //a[contains(., 'P E')] failed: timeout 30000ms exceeded"

@balazsbajorics
Copy link
Contributor

Annoyingly, the react-hooks/exhaustive-deps eslint rule was tripped in a few cases by generic type symbols (e.g. in useGetMultiselectedProps the rule complains that P is not included in the list of dependencies, which is nonsense!), so I've had to disable those on a few lines. I'll check to see if there is an update for that rule to fix the issue.

I believe there is an update that fixes the issues!

@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2021

Staging deploy success. Link to test editor: https://utopia.pizza/p/?branch_name=performance-use-optimized-selector
Performance: Scroll Canvas: 25.2ms | Resize: 20ms | Selection: 196.1ms SummaryChart

@github-actions
Copy link
Contributor

Link to test editor
Scroll Canvas: 46.2ms (31.6-117.9ms) | Resize: 132.1ms (101.3-633ms) | Selection: 208.3ms (200.2-233.9ms) (Chart)

@github-actions
Copy link
Contributor

Link to test editor
Scroll Canvas: 33.2ms (26.4-247.1ms) | Resize: 106.2ms (92.5-514.1ms) | Selection: 172.8ms (163.5-208.1ms) (Chart)

@maltenuhn
Copy link
Member

Is this PR still something we want to merge?

@Rheeseyb
Copy link
Contributor Author

Rheeseyb commented Aug 9, 2021

Tagged and closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants