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

[BUGFIX beta] Upgrade glimmer-vm for some perf improvements #20746

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Sep 12, 2024

@NullVoxPopuli NullVoxPopuli changed the title Upgrade glimmer-vm for some perf improvements [Bugfix] Upgrade glimmer-vm for some perf improvements Sep 17, 2024
@kategengler kategengler changed the title [Bugfix] Upgrade glimmer-vm for some perf improvements [BUGFIX beta] Upgrade glimmer-vm for some perf improvements Sep 18, 2024
@@ -195,6 +195,7 @@ export function exposedDependencies() {
'@glimmer/owner',
'@glimmer/opcode-compiler',
'@glimmer/runtime',
'@glimmer/validator',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ember-source does directly use @glimmer/validator, this seems correct. But why wasn't it needed before now?

I based this list on the classical behavior, which did not have @glimmer/validator on the list of top-level glimmer packages to include. It got included in the build output because it's a dependency of some of these others. Did the other packages drop their dependency on it?

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also found this sus.

Did the other packages drop their dependency on it?

that is my leading hypothesis, yea.

The whole package graph between the vm-provided packages changed quite a bit in the ember 5.5 to 5.6 bump, and then it changed again with this change as @glimmer/debug is now much slimmer (due to removing a bunch of VM-specific debug code), it makes sense that the import/module graph would change -- the imports in debug are different --- but I don't know why that affected glimmer/validator as debug imports from the vm and previously also the utils -- but neither vm/index.js nor util/index.js import from validator.

I need a good tool to diff folders with regex filtering or something.

tangentially -- while this change to the VM is an improvement to prod builds, the work on speeding up the VM isn't done -- but I do think for some perf-aware teams, it'll unblock the 5.5 -> 5.12 upgrade, while I / others keep poking around looking for perf opportunities (I already know of two more to look at, but I'm not sure how to make the tooling do the build time cleanup for me yet).
(( I'd want additional updates to also be backported to 5.12 ))

@kategengler kategengler merged commit bf12787 into emberjs:main Sep 20, 2024
24 checks passed
@NullVoxPopuli NullVoxPopuli deleted the nvp-upgrade-glimmer-vm-92.x branch September 20, 2024 18:33
NullVoxPopuli pushed a commit to NullVoxPopuli/ember.js that referenced this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants