search

emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.45k stars 4.21k forks source link

Asset size regression in v3.17.0 #18796

Closed Turbo87 closed 1 year ago

Turbo87 commented 4 years ago

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

Before

After

/cc @pzuraq @rwjblue

makepanic commented 4 years ago

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. https://github.com/glimmerjs/glimmer-vm/commit/4c56c216465410f5bd6f4f53fb7d4508f8048f09#diff-64c3cabffb164d9a2c4bf2d427094e19 via https://github.com/emberjs/ember.js/commit/699439c0aaa1917754490efeb5751d10f77e5230

boris-petrov commented 4 years ago

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?

rwjblue commented 4 years ago

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

krisselden commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

The change in wire format seems to be related to the work done here: https://github.com/glimmerjs/glimmer-vm/pull/972

krisselden commented 4 years ago

https://github.com/glimmerjs/glimmer-vm/pull/1056

krisselden commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago 
$ 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

FYI - https://github.com/emberjs/ember.js/pull/18941 addresses the bulk of the growth vs 3.16 (though still larger by ~ 3%, which we are working on).

locks commented 1 year ago

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