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

[Bug] Template compilation significantly slower starting in Ember 3.25 #19750

Closed
brendenpalmer opened this issue Sep 10, 2021 · 3 comments
Closed

Comments

@brendenpalmer
Copy link
Contributor

brendenpalmer commented Sep 10, 2021

🐞 Describe the Bug

We recently upgraded a large Ember application (with nearly 4k template files) from 3.24 to 3.28, and noticed that our build speed has regressed (more than a 10% regression) quite a bit due to this upgrade. After some debugging we've traced the issue back to 3.25 where it seems template compilation is significantly slower.

🔬 Minimal Reproduction

To reproduce this:

  1. Clone https://github.com/brendenpalmer/repro-ember-3-25-template-regression
  2. Run yarn install
  3. Run yarn bench

😕 Actual Behavior

Slower template compilation after upgrading to Ember 3.28 (the regression looks to be introduced as part of Ember 3.25)

🤔 Expected Behavior

(Ideally) no regression to template compilation speeds

🌍 Environment

  • Ember: - 3.24/3.25
  • Node.js/npm: - node 14.15.4
  • OS: - macOS Big Sur
  • Browser: - N/A
chriskrycho added a commit to chriskrycho/glimmer-vm that referenced this issue Sep 10, 2021
The updates to the template compiler made in support of strict mode
introduced a number of uses of private class fields, some of them in
hot paths. Since `tsc` currently transpiles private class fields to
`WeakMap`s for all values of `target` lower than `ESNEXT`, this results
in dramatically higher memory usage and garbage collection than in the
previous version of the template compiler. This in turn causes at least
a large portion of the regression noted in emberjs/ember.js#19750.

Here, we replace *all* private class fields with `_` private fields,
which substantially (thought not 100%) closed the gap with the original.

Co-authored-by: Brenden Palmer <brendenpalmer@gmail.com>
chriskrycho added a commit to chriskrycho/glimmer-vm that referenced this issue Sep 10, 2021
The updates to the template compiler made in support of strict mode
introduced a number of uses of private class fields, some of them in
hot paths. Since `tsc` currently transpiles private class fields to
`WeakMap`s for all values of `target` lower than `ESNEXT`, this results
in dramatically higher memory usage and garbage collection than in the
previous version of the template compiler. This in turn causes at least
a large portion of the regression noted in [emberjs/ember.js#19750][1].

[1]: emberjs/ember.js#19750

Here, we replace *all* private class fields with `_` private fields,
which substantially (thought not 100%) closed the gap with the original.

Co-authored-by: Brenden Palmer <brendenpalmer@gmail.com>
chriskrycho added a commit to chriskrycho/glimmer-vm that referenced this issue Sep 10, 2021
The updates to the template compiler made in support of strict mode
introduced a number of uses of private class fields, some of them in hot
paths. Although all supported versions of Node support private class
fields, `tsc` transpiles private class fields to `WeakMap`s for all
values of `target` lower than `ESNEXT`. As a result, the use of private
class fields results in dramatically higher memory usage and garbage
collection than in the previous version of the template compiler. This
in turn causes at least a large portion of the regression noted in
[emberjs/ember.js#19750][1].

[1]: emberjs/ember.js#19750

Here, we replace *all* private class fields with `_` private fields,
which substantially (thought not 100%) closed the gap with the original.

Co-authored-by: Brenden Palmer <brendenpalmer@gmail.com>
@chriskrycho
Copy link
Contributor

chriskrycho commented Oct 5, 2021

A status update for anyone following along with this: we've landed a number of improvements on the VM—

—but unfortunately, while they seem to be improving both CPU and memory impact in our repro repo they are not having a meaningful effect in our app, because all the impact from them is being swamped by increased memory pressure, in two ways:

  1. We continue to see significantly higher GC times. In fact, the GC times alone account for nearly half of the delta between 3.24 and 3.281 in our app when I tested yesterday.2
  2. From what I can tell, the increased memory pressure is also leading to other slowdowns across the build. For example, I noticed that with no change other than bumping to 3.28, a totally unrelated piece of code3 slowed down by 50% in that same in-repo addon.

I currently suspect that this is simply the result of all the extra allocation involved with having two versions of the AST in the VM—the duplication resulting in both the higher memory pressure and associated slowdowns across other functionality and the extra GC when we drop the v1 AST after normalization. I'm going to try to validate that hypothesis today, and if I do, we’ll reevaluate next steps then.

Footnotes

  1. specifically, 3.28 when patched with all of these VM changes, but there is no meaningful difference between the patched and unpatched version here

  2. tested in an in-repo addon in a setup which allowed us to test only our host app's templates, not any of the many hundreds of in-repo addons (including engines)—a non-trivial number of templates, but also a manageable set so I could actually get a useful CPU profile

  3. a library which does schema analysis for our data layer—and has no dependencies on ember-source, direct or transitive

@chriskrycho
Copy link
Contributor

Update on the above: I had a measurement failure,1 and the cumulative effective of those changes was actually substantial within our app:

  • Memory usage was resolved entirely: total heap usage, resident set size, etc. were all within the error bars/noise of our measurements. 🎉
  • Total build time was improved to a sufficient degree: roughly a 5% regression (20 seconds against a ~340s baseline) vs. the original 13–14% regression (45–50 seconds against baseline) we had seen previously. We consider this acceptable (and indeed to some degree expected) given the substantial amount of increased functionality provided by the updated compiler—not only strict mode template support and named blocks but also much better error reporting.

I will be opening back-ports of those fixes to the Glimmer v0.80 branch today, and we will then aim to land a bug fix release with those changes on Ember 3.28. I'll mark that bug fix PR as resolving this issue, so it should be closed within the next few days!

Footnotes

  1. I had installed patch-package and generated the patches, but failed to add the patch-package invocation to postinstall. 🤦🏼‍♂️

chriskrycho added a commit to chriskrycho/glimmer-vm that referenced this issue Oct 22, 2021
The updates to the template compiler made in support of strict mode
introduced a number of uses of private class fields, some of them in hot
paths. Although all supported versions of Node support private class
fields, `tsc` transpiles private class fields to `WeakMap`s for all
values of `target` lower than `ESNEXT`. As a result, the use of private
class fields results in dramatically higher memory usage and garbage
collection than in the previous version of the template compiler. This
in turn causes at least a large portion of the regression noted in
[emberjs/ember.js#19750][1].

[1]: emberjs/ember.js#19750

Here, we replace *all* private class fields with `_` private fields,
which substantially (thought not 100%) closed the gap with the original.

Co-authored-by: Brenden Palmer <brendenpalmer@gmail.com>
@chriskrycho
Copy link
Contributor

We fixed this particular thing, so I'm closing this. Thanks again to everyone who helped get it sorted!

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

3 participants