Use _private instead of #private properties

[chriskrycho]

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 WeakMaps 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.

As alternatives here, we could:

• only update the hot path fields (e.g. in HbsSpan)
• update our target output for TS, and then do a further targeted compilation pass using Babel to downlevel the output to use features supported by Node 12

cc. @brendenpalmer @rwjblue @krisselden

[rwjblue]

As alternatives here, we could

I don't think we need to do something else here. All of this code is private already and we really don't have to protect ourselves to the extent that private fields are a fundamental requirement.

[rwjblue]

@chriskrycho - Can you test this against the reproduction created in https://github.com/brendenpalmer/repro-ember-3-25-template-regression? I'm happy to help set up a simplified pipeline to test those templates in just @glimmer/syntax (vs requiring the full ember-template-compiler)...

[chriskrycho]

@rwjblue agreed re: not needing to do those things; just wanted to call them out for context historically. If you have that simplified pipeline that'd be great. I will note that we tested these changes against the LinkedIn.com app, and this fix got us about 22% of our total degradation back. Which is good! …but also means that we will need to find another 78% elsewhere.

[chriskrycho]

Using this PR on the repro repo, here are the timings reported:

Total precompile time using '[email protected] (ember-source-3-28)' 13067
Total precompile time using '[email protected] (ember-source-3-24)' 2234
Total precompile time using '[email protected] (ember-source-3-25)' 12299
Total precompile time using '@glimmer/[email protected] (glimmer-compiler-ember-source-3-24)' 2288
Total precompile time using '@glimmer/[email protected] (glimmer-compiler-experiment)' 4667
Total precompile time using '@glimmer/[email protected] (glimmer-compiler-ember-source-3-28)' 12434

Here, glimmer-compiler-experiment is the version with the fix represented by this PR.

I'll add some stats from our build analysis internally later as well.

[rwjblue]

This is a really nice improvement (looks like it brings us back to right around 2x slower than the 3.24 speed vs the current 5.5x slower than 3.24 of master)!

[chriskrycho]

This is definitely a good improvement, so landing it will help! However, I'm also going to keep pushing, as it's still a non-trivial regression for our app.

Results from our app

Here are results comparing our app's behavior on 3.24, 3.28, and 3.28 with this patch.

Some definitions:

Cold Build

no cache available (i.e. because node_modules changed)

Warm Build

A cache exists and can be used for the build.

CI Production Build

Just what it sounds like. No cache, using --production instead of the default (--development) mode.

RSS

resident set size: basically the total process memory (that isn't in swap)

3.24

Build Type	RSS	Heap Total	Heap Used	External	Array Buffers	Total Time
Cold Build	10.44 GB	2.94 GB	2.90 GB	79.29 MB	65.42 MB	337473.6666666667ms
Warm Build	2.58 GB	2.39 GB	2.35 GB	75.40 MB	67.26 MB	160771ms
CI Production Build	16.61 GB	5.16 GB	5.07 GB	72.96 MB	42.41 MB	490599ms

3.28

Build Type	RSS	Heap Total	Heap Used	External	Array Buffers	Total Time
Cold Build	11.07 GB	3.12 GB	3.08 GB	76.16 MB	71.07 MB	377178.3333333333ms
Warm Build	2.68 GB	2.46 GB	2.42 GB	87.59 MB	82.81 MB	161692.66666666666ms
CI Production Build	18.33 GB	5.29 GB	5.18 GB	69.76 MB	44.46 MB	531570.6666666666ms

3.28 with this patch

Build Type	RSS	Heap Total	Heap Used	External	Array Buffers	Total Time
Cold Build	10.50 GB	3.01 GB	2.97 GB	76.19 MB	68.30 MB	361865ms
Warm Build	2.65 GB	2.45 GB	2.42 GB	73.78 MB	68.52 MB	162581.66666666666ms
CI Production Build	17.59 GB	5.28 GB	5.11 GB	71.09 MB	43.03 MB	523085.6666666667ms

Summary

This change helps, but it helps our app significantly less than it helps in the benchmark for cold builds:

• 3.24: 2.94GB heap total, ~5m 37s
• 3.28: 3.12GB heap total, ~6m 17s
• 3.28 patched: 3.01G heap total, ~6m 2s

We currently suspect this is a function of the fact that the new work still load ends up including more memory pressure, and thus causing more GCs, causing new allocations, or both, so in a larger app: we have more than 2.5× the templates in our app and ~5.67× as many lines of template code.

[rwjblue]

Thanks for gathering those numbers @chriskrycho! Definitely still room to grow, but this is still a nice improvement. So it's good to land...