Cache Execution Scope #6076
Merged
Cache Execution Scope #6076
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- `createExecutionScope` caches the scopes by filename and the project contents as a whole.
seanparsons
requested review from
gbalint,
Rheeseyb,
ruggi,
balazsbajorics,
liady and
bkrmendy
#13365 Bundle Size — 62.65MiB (~+0.01%).
Warning Bundle contains 70 duplicate packages – View duplicate packages Bundle metrics
|
| Current #13365 |
Baseline #13362 |
|
|---|---|---|
 Initial JS |
45.72MiB(~+0.01%) |
45.72MiB |
 Initial CSS |
0B |
0B |
 Cache Invalidation |
21.56% |
21.54% |
| Chunks |
31 |
31 |
| Assets |
34 |
34 |
| Modules |
4371 |
4371 |
| Duplicate Modules |
521 |
521 |
| Duplicate Code |
31.68% |
31.68% |
| Packages |
469 |
469 |
| Duplicate Packages |
70 |
70 |
Bundle size by type 2 changes 1 regression 
1 improvement
| Current #13365 |
Baseline #13362 |
|
|---|---|---|
| JS |
62.63MiB (~+0.01%) |
62.63MiB |
| HTML |
11.16KiB (-0.33%) |
11.2KiB |
Bundle analysis report Branch performance/cache-execution-scop... Project dashboard
balazsbajorics
approved these changes
Jul 15, 2024
Rheeseyb
approved these changes
Jul 15, 2024
ruggi
approved these changes
Jul 15, 2024
2 tasks
balazsbajorics
added a commit
that referenced
this pull request
Jul 19, 2024
**Problem:** When anything in the projectContents changes, the DerivedState.remixData gets recreated. (This should be improved to only recreate the remixData if we actually change the file-based routing) The `UtopiaRemixRootComponent` uses **illegal** (🙂) useEditorState hooks to access `DerivedState.remixData`. Since `DerivedState.remixData` is referentially new, the useEditorState hooks will trigger a re-render of every UtopiaRemixRootComponent. The UtopiaRemixRootComponent rerenders then basically trigger an almost full rerender of the canvas before the "real" canvas rerender step. on master: <img width="1097" alt="image" src="https://github.com/user-attachments/assets/d15c8a69-0bbc-47de-be08-c896b8e70f1c"> with Sean's createExecutionScope cache fix (currently disabled and blocked by #6091): <img width="849" alt="image" src="https://github.com/user-attachments/assets/e649e8da-8b1b-43f7-b207-927d67bcfbbf"> On the image you can see that in the sample store's `/` route this takes 15ms every single frame. **Fix:** **20ms -> 10ms -> 5ms** <img width="885" alt="image" src="https://github.com/user-attachments/assets/c78e1ec7-993f-4bc7-b109-8030bd649e5c"> When the canvas is in selective rerender mode, DO NOT let `UtopiaRemixRootComponent` rerender itself. Not updating UtopiaRemixRootComponent during the selective mode should be safe, since the continuous canvas interactions never cause the file based routing or loaders etc to update. **Commit Details:** (< vv pls delete this section if's not relevant) - Wrapping the canvas in a EditorStateContext provider with the value forced to `null` which means we will throw an error in the future every time new code accidentally uses the useEditorState hooks (which are _not_ compatible with selective re-rendering). Hopefully this means selective re-rendering will be less brittle in the future, but in a follow up PR I will write tests for this too - Added new hook `useCanvasState` which is almost the same as `useEditorState` except it takes an extra callback `allowRerender`. If AllowRerender is false, then no matter what changes in the editor state, we will passively reject the component update and the hook will return the _old_ state value. - Using the new hook in `utopia-remix-root-component` - Profit! **Notes:** This PR depends on #6091 because the selective re-rendering of the remix root component causes the same css issue as Sean's original #6076 **Manual Tests:** I hereby swear that: - [x] I opened a hydrogen project and it loaded - [x] I could navigate to various routes in Preview mode
liady
pushed a commit
that referenced
this pull request
Dec 13, 2024
- `createExecutionScope` caches the scopes by filename and the project contents as a whole.
liady
pushed a commit
that referenced
this pull request
Dec 13, 2024
**Problem:** When anything in the projectContents changes, the DerivedState.remixData gets recreated. (This should be improved to only recreate the remixData if we actually change the file-based routing) The `UtopiaRemixRootComponent` uses **illegal** (🙂) useEditorState hooks to access `DerivedState.remixData`. Since `DerivedState.remixData` is referentially new, the useEditorState hooks will trigger a re-render of every UtopiaRemixRootComponent. The UtopiaRemixRootComponent rerenders then basically trigger an almost full rerender of the canvas before the "real" canvas rerender step. on master: <img width="1097" alt="image" src="https://github.com/user-attachments/assets/d15c8a69-0bbc-47de-be08-c896b8e70f1c"> with Sean's createExecutionScope cache fix (currently disabled and blocked by #6091): <img width="849" alt="image" src="https://github.com/user-attachments/assets/e649e8da-8b1b-43f7-b207-927d67bcfbbf"> On the image you can see that in the sample store's `/` route this takes 15ms every single frame. **Fix:** **20ms -> 10ms -> 5ms** <img width="885" alt="image" src="https://github.com/user-attachments/assets/c78e1ec7-993f-4bc7-b109-8030bd649e5c"> When the canvas is in selective rerender mode, DO NOT let `UtopiaRemixRootComponent` rerender itself. Not updating UtopiaRemixRootComponent during the selective mode should be safe, since the continuous canvas interactions never cause the file based routing or loaders etc to update. **Commit Details:** (< vv pls delete this section if's not relevant) - Wrapping the canvas in a EditorStateContext provider with the value forced to `null` which means we will throw an error in the future every time new code accidentally uses the useEditorState hooks (which are _not_ compatible with selective re-rendering). Hopefully this means selective re-rendering will be less brittle in the future, but in a follow up PR I will write tests for this too - Added new hook `useCanvasState` which is almost the same as `useEditorState` except it takes an extra callback `allowRerender`. If AllowRerender is false, then no matter what changes in the editor state, we will passively reject the component update and the hook will return the _old_ state value. - Using the new hook in `utopia-remix-root-component` - Profit! **Notes:** This PR depends on #6091 because the selective re-rendering of the remix root component causes the same css issue as Sean's original #6076 **Manual Tests:** I hereby swear that: - [x] I opened a hydrogen project and it loaded - [x] I could navigate to various routes in Preview mode
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem:
For Remix projects there's a lot of calls which create execution scopes multiple times per frame and even potentially for those multiple times for the same files as a result of them being nested.
Fix:
I added a very simple caching scheme for the execution scopes which clears the entire cache if a new
projectContentsis seen and otherwise caches the entire result if building the scope by file. This has taken a block that could take around 18ms down to a seeming maximum of 6ms.Before:
After:
Commit Details:
createExecutionScopecaches the scopes by filename and the project contents as a whole.Manual Tests:
I hereby swear that:
Fixes #6064