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

Asset size regression in v3.17.0 #18796

Closed
Turbo87 opened this issue Mar 5, 2020 · 15 comments
Closed

Asset size regression in v3.17.0 #18796

Turbo87 opened this issue Mar 5, 2020 · 15 comments

Comments

@Turbo87
Copy link
Member

Turbo87 commented Mar 5, 2020

We noticed in one of our apps that the Ember v3.17.0 release significantly increase our asset sizes:

Before

  • dist/assets/app.js: 613.18 KB (78.46 KB gzipped)
  • dist/assets/vendor.js: 2.34 MB (558.13 KB gzipped)

After

  • dist/assets/app.js: 690.43 KB (89.22 KB gzipped)
  • dist/assets/vendor.js: 2.42 MB (572.39 KB gzipped)

/cc @pzuraq @rwjblue

@makepanic
Copy link
Contributor

makepanic commented Mar 9, 2020

Can confirm in a fresh new ember project:

2.16.0

  • vendor: 692.58 KB (176.09 KB gzipped)
  • app: 9.82 KB (2.15 KB gzipped)

2.17.0

  • vendor: 715.12 KB (183.64 KB gzipped)
  • app: 9.94 KB (2.2 KB gzipped)

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

@boris-petrov
Copy link
Contributor

boris-petrov commented Mar 10, 2020

Same here.

2.16.3

  • vendor: 886KB (213KB brotli)
  • app: 1.01MB (126KB brotli)

2.17.0

  • vendor: 907KB (219KB brotli)
  • app: 1.15MB (137KB brotli)

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?

@rwjblue
Copy link
Member

rwjblue commented Mar 10, 2020

@boris-petrov - Mind checking that for us? I think you should be able to remove the minifier from your apps package.json...

@rwjblue rwjblue added the Bug label Mar 10, 2020
@krisselden
Copy link
Contributor

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

@boris-petrov
Copy link
Contributor

@rwjblue - I believe @krisselden is correct - this has nothing to do with ember-hbs-minifier. I tried a brand new Ember app with the following helper:

import Helper from '@ember/component/helper';
export default Helper.helper(() => true);

And two components:

other-component.hbs

{{some-component foo=1}}

some-component.hbs

{{some-helper 1 2 3 @foo}}

3.16.3 produced for the two components:

other-component.hbs:

block: "{\"symbols\":[],\"statements\":[[1,[28,\"some-component\",null,[[\"foo\"],[1]]],false],[0,\"\\n\"]],\"hasEval\":false}",

some-component.hbs:

block: "{\"symbols\":[\"@foo\"],\"statements\":[[1,[28,\"some-helper\",[1,2,3,[23,1,[]]],null],false],[0,\"\\n\"]],\"hasEval\":false}",

The same for 3.17:

other-component.hbs:

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\"]}",

some-component.hbs:

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.

@acorncom
Copy link
Contributor

The change in wire format seems to be related to the work done here: glimmerjs/glimmer-vm#972

@krisselden
Copy link
Contributor

glimmerjs/glimmer-vm#1056

@krisselden
Copy link
Contributor

Ember 3.16

- dist/assets/ember-observer.js: 706.89 KB (66.89 KB gzipped)

Ember 3.17

 - dist/assets/ember-observer.js: 743.7 KB (69.63 KB gzipped)

After const enum string to number

 - dist/assets/ember-observer.js: 739.58 KB (69.44 KB gzipped)

Removing loc data

 - dist/assets/ember-observer.js: 723.55 KB (68.06 KB gzipped)

Flatten Get + GetPath + GetContextualFree expressions

- dist/assets/ember-observer.js: 721.99 KB (68.04 KB gzipped)

@krisselden
Copy link
Contributor

I made a util, if you wouldn't mind running on your apps to let me know how the patch does

https://www.npmjs.com/package/ember-template-size

@Turbo87
Copy link
Member Author

Turbo87 commented Mar 20, 2020

$ npx ember-template-size .
npx: installed 11 in 1.766s
{
  '3.16.3': {
    version: '3.16.3',
    original: 793656,
    compiled: 1223295,
    brotli: 242598,
    gzip: 242606
  },
  '3.17.0-patched': {
    version: '3.17.0-patched',
    original: 793656,
    compiled: 1299096,
    brotli: 258803,
    gzip: 258826
  },
  '3.17.0': {
    version: '3.17.0',
    original: 793656,
    compiled: 1624741,
    brotli: 294760,
    gzip: 294780
  }
}

@Turbo87
Copy link
Member Author

Turbo87 commented Mar 20, 2020

and for https://github.com/rust-lang/crates.io:

{
  '3.16.3': {
    version: '3.16.3',
    original: 73613,
    compiled: 143264,
    brotli: 32477,
    gzip: 32608
  },
  '3.17.0-patched': {
    version: '3.17.0-patched',
    original: 73613,
    compiled: 160526,
    brotli: 34663,
    gzip: 34803
  },
  '3.17.0': {
    version: '3.17.0',
    original: 73613,
    compiled: 183711,
    brotli: 36903,
    gzip: 37070
  }
}

@makepanic
Copy link
Contributor

for us:

{
  '3.16.3': {
    version: '3.16.3',
    original: 281240,
    compiled: 450040,
    brotli: 98120,
    gzip: 98142
  },
  '3.17.0-patched': {
    version: '3.17.0-patched',
    original: 281240,
    compiled: 494042,
    brotli: 105152,
    gzip: 105178
  },
  '3.17.0': {
    version: '3.17.0',
    original: 281240,
    compiled: 596287,
    brotli: 117757,
    gzip: 117784
  }
}

@krisselden
Copy link
Contributor

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.

@rwjblue
Copy link
Member

rwjblue commented May 1, 2020

FYI - #18941 addresses the bulk of the growth vs 3.16 (though still larger by ~ 3%, which we are working on).

@locks
Copy link
Contributor

locks commented Jul 22, 2023

Closing as mostly addressed. Thanks everyone for the efforts to get back on track!

@locks locks closed this as completed Jul 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants