-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Asset size regression in v3.17.0 #18796
Comments
Can confirm in a fresh new ember project: 2.16.0
2.17.0
Did some vendor diffs and it seems to be caused by some glimmer-vm updates, e.g. glimmerjs/glimmer-vm@4c56c21#diff-64c3cabffb164d9a2c4bf2d427094e19 via 699439c |
Same here. 2.16.3
2.17.0
Also, I noticed a ~5% performance degradation in our integration tests going from 2.16.3 to 2.17.0. P.S. @Turbo87 - could it be because of the HBS minifier? I'm also using it. Could it have "broken" somehow for the new templates generated by the updated Glimmer VM? |
@boris-petrov - Mind checking that for us? I think you should be able to remove the minifier from your apps |
Looks like this is coming from changes in the glimmer wireformat that is bloating the template sizes a bit. In particular doing a diff on the app is https://github.com/glimmerjs/glimmer-vm/blob/11cc83b2adf220bef298a1a6298c9e4e750c9fe1/packages/%40glimmer/interfaces/lib/compile/wire-format.d.ts#L76 |
@rwjblue - I believe @krisselden is correct - this has nothing to do with import Helper from '@ember/component/helper';
export default Helper.helper(() => true); And two components:
{{some-component foo=1}}
{{some-helper 1 2 3 @foo}} 3.16.3 produced for the two components:
block: "{\"symbols\":[],\"statements\":[[1,[28,\"some-component\",null,[[\"foo\"],[1]]],false],[0,\"\\n\"]],\"hasEval\":false}",
block: "{\"symbols\":[\"@foo\"],\"statements\":[[1,[28,\"some-helper\",[1,2,3,[23,1,[]]],null],false],[0,\"\\n\"]],\"hasEval\":false}", The same for 3.17:
block: "{\"symbols\":[],\"statements\":[[1,0,0,0,[31,2,14,[27,[26,0,\"CallHead\"],[]],null,[[\"foo\"],[1]]]],[1,1,0,0,\"\\n\"]],\"hasEval\":false,\"upvars\":[\"some-component\"]}",
block: "{\"symbols\":[\"@foo\"],\"statements\":[[1,0,0,0,[31,2,11,[27,[26,0,\"CallHead\"],[]],[1,2,3,[27,[24,1],[]]],null]],[1,1,0,0,\"\\n\"]],\"hasEval\":false,\"upvars\":[\"some-helper\"]}", I wouldn't call that a "bug", it's just not very nice. The bigger the app you have, the bigger the templates will have gotten. |
The change in wire format seems to be related to the work done here: glimmerjs/glimmer-vm#972 |
Ember 3.16
Ember 3.17
After const enum string to number
Removing loc data
Flatten Get + GetPath + GetContextualFree expressions
|
I made a util, if you wouldn't mind running on your apps to let me know how the patch does |
|
and for https://github.com/rust-lang/crates.io:
|
for us:
|
The patch represents most the low hanging fruit ideas I had. I have a couple more ideas, but based on the results so far, not going to recover the rest of the regression. The refactor that caused the regression was large and enables some future direction and I have limited time. I have 2 more simple ideas, flatten [Append, 1 | 0, ...] into [TrustingAppend | Append, ..] and then there is wire format tuple that uses a boolean flag that can use 1 | 0 instead. |
FYI - #18941 addresses the bulk of the growth vs 3.16 (though still larger by ~ 3%, which we are working on). |
Closing as mostly addressed. Thanks everyone for the efforts to get back on track! |
We noticed in one of our apps that the Ember v3.17.0 release significantly increase our asset sizes:
Before
After
/cc @pzuraq @rwjblue
The text was updated successfully, but these errors were encountered: