Use Object.assign instead of custom copy function

[chriskrycho]

While investigating possible ways of mitigating the build time costs identified in emberjs/ember.js#19750, we noticed this custom copy implementation. Since it performs the same basic behavior as Object.assign, I replaced it with Object.assign.

This has no impact on runtime performance (see below), and drops about 1.1KB of non-minified, uncompressed bytes across the wire.

Runtime Performance

I ran it against the set of templates we pulled together in https://github.com/brendenpalmer/repro-ember-3-25-template-regression, using this PR with the following results.

While it looks like a small percentage decrease against the baseline, the variance between runs is significant enough (even with 5 runs per compiler) that I don't think this is meaningful. That said, I'm still going to run this change through our app as well, and if that looks more meaningful I'll report that below.

With current Glimmer VM master

source	time
[email protected] (ember-source-3-24)	1958.2
[email protected] (ember-source-3-25)	12742.2
[email protected] (ember-source-3-28)	12499.4
@glimmer/[email protected] (glimmer-compiler-ember-source-3-24)	1981.2
@glimmer/[email protected] (glimmer-compiler-ember-source-3-28)	13433.6
@glimmer/[email protected] (glimmer-compiler-experiment)	4201.4

With Object.assign

This adds in Object.assign replacing copy.

source	time
[email protected] (ember-source-3-24)	2147.2
[email protected] (ember-source-3-25)	19060.6
[email protected] (ember-source-3-28)	15271.6
@glimmer/[email protected] (glimmer-compiler-ember-source-3-24)	2246
@glimmer/[email protected] (glimmer-compiler-ember-source-3-28)	21358
@glimmer/[email protected] (glimmer-compiler-experiment)	4369

[chriskrycho]

This is failing b/c the repo hard-errors against use of Object.assign:

Error: Unexpected use of Object.assign. Object.assign cannot be used because it is unavailable in IE11. This error was likely caused by using syntax like the spread operator ({ ...obj }) which transpiles to Object.assign. Please use alternate syntax that is compatible with IE11.

It's not clear to me why we don't just let users configure an Object.assign polyfill if needed?

[chriskrycho]

Addendum on the above comment: Ember's own build process does polyfill this correctly anyway, so it's unclear why Glimmer's own test suite is forbidding it.

[chriskrycho]

@brendenpalmer and I ran this change against the LinkedIn app and it looks like the impact scales with the amount of templates compiled. (This app has about 2½× the number of template files as the repro repo, but almost 6× the number of lines of templates.)

TL;DR

This seems to be a meaningful win on a large codebase: it gets us back another almost 20 seconds on the LinkedIn app (more even than the private class fields fix got us), and drops RSS by over 1 GB. Although it doesn't seem to have a meaningful impact on Heap Total, we are within spitting distance of the original value here already. Hopefully we can find a few more of these optimizations!

Comparison	RSS	Heap Total	Total Time
Baseline	–	–	–
Glimmer 0.80.0	+10.8%	+8.8%	+13.2%
All fixes so far	+ 3.8%	+2.2%	+6.5%

Definitions

A handful of terms with non-obvious meanings:

Cold Build

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

Warm Build

A cache exists and can be used for the build.

RSS

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

Glimmer VM v0.65.3 (Ember 3.24) baseline

Averages:

Build Type	RSS	Heap Total	Heap Used	External	Array Buffers	Total Time
Cold Build	9.90 GB	2.72 GB	2.69 GB	71.20 MB	66.16 MB	340204ms
Warm Build	2.62 GB	2.42 GB	2.38 GB	73.95 MB	68.84 MB	169507.66666666666ms

see details across all runs

Build Type	RSS	Heap Total	Heap Used	External	Array Buffers	Total Time
Cold Build #1	9.51 GB	2.65 GB	2.61 GB	74.89 MB	69.34 MB	340158ms
Cold Build #2	10.21 GB	2.85 GB	2.82 GB	68.73 MB	63.92 MB	340658ms
Cold Build #3	9.98 GB	2.67 GB	2.63 GB	69.97 MB	65.23 MB	339796ms
Warm Build #1	2.71 GB	2.51 GB	2.47 GB	75.32 MB	70.63 MB	168872ms
Warm Build #2	2.56 GB	2.37 GB	2.33 GB	72.55 MB	67.18 MB	171258ms
Warm Build #3	2.58 GB	2.38 GB	2.35 GB	73.98 MB	68.71 MB	168393ms

Glimmer VM v0.80.0 baseline

Averages:

Build Type	RSS	Heap Total	Heap Used	External	Array Buffers	Total Time
Cold Build	10.98 GB	2.96 GB	2.92 GB	68.87 MB	64.05 MB	385323.6666666667ms
Warm Build	2.63 GB	2.43 GB	2.39 GB	75.09 MB	70.11 MB	170908.33333333334ms

see details across all runs

Build Type	RSS	Heap Total	Heap Used	External	Array Buffers	Total Time
Cold Build #1	11.20 GB	3.05 GB	3.01 GB	68.64 MB	63.93 MB	391179ms
Cold Build #2	10.53 GB	2.73 GB	2.68 GB	68.70 MB	63.86 MB	382586ms
Cold Build #3	11.21 GB	3.10 GB	3.06 GB	69.26 MB	64.36 MB	382206ms
Warm Build #1	2.63 GB	2.44 GB	2.39 GB	74.64 MB	69.39 MB	170861ms
Warm Build #2	2.63 GB	2.43 GB	2.40 GB	77.02 MB	72.23 MB	168272ms
Warm Build #3	2.63 GB	2.43 GB	2.39 GB	73.61 MB	68.70 MB	173592ms

With Glimmer VM master (not yet released)

Build Type	RSS	Heap Total	Heap Used	External	Array Buffers	Total Time
Cold Build	11.17 GB	2.77 GB	2.73 GB	71.74 MB	67.03 MB	379733.3333333333ms
Warm Build	2.64 GB	2.44 GB	2.40 GB	75.26 MB	70.58 MB	185741.66666666666ms

see details across all runs

Build Type	RSS	Heap Total	Heap Used	External	Array Buffers	Total Time
Cold Build #1	10.97 GB	2.79 GB	2.74 GB	69.86 MB	65.13 MB	370682ms
Cold Build #2	11.29 GB	2.98 GB	2.95 GB	68.27 MB	63.49 MB	375612ms
Cold Build #3	11.23 GB	2.54 GB	2.50 GB	77.10 MB	72.49 MB	392906ms
Warm Build #1	2.65 GB	2.45 GB	2.41 GB	74.20 MB	69.56 MB	180750ms
Warm Build #2	2.64 GB	2.44 GB	2.41 GB	76.85 MB	72.24 MB	183096ms
Warm Build #3	2.62 GB	2.43 GB	2.39 GB	74.74 MB	69.93 MB	193379ms

With object.assign fix (this PR)

Averages:

Build Type	RSS	Heap Total	Heap Used	External	Array Buffers	Total Time
Cold Build	10.28 GB	2.78 GB	2.74 GB	70.09 MB	64.04 MB	361844.6666666667ms
Warm Build	2.64 GB	2.44 GB	2.40 GB	71.31 MB	66.03 MB	168402.66666666666ms

see details across all runs

Build Type	RSS	Heap Total	Heap Used	External	Array Buffers	Total Time
Cold Build #1	11.03 GB	2.65 GB	2.62 GB	69.64 MB	64.85 MB	362833ms
Cold Build #2	9.74 GB	2.83 GB	2.79 GB	68.43 MB	63.54 MB	362586ms
Cold Build #3	10.09 GB	2.85 GB	2.81 GB	72.21 MB	63.71 MB	360115ms
Warm Build #1	2.63 GB	2.43 GB	2.39 GB	74.54 MB	69.55 MB	168283ms
Warm Build #2	2.64 GB	2.44 GB	2.40 GB	66.58 MB	61.24 MB	168562ms
Warm Build #3	2.65 GB	2.45 GB	2.41 GB	72.82 MB	67.30 MB	168363ms

Analysis

This makes sense intuitively. The previous approach using copy() (and its helper keys() function) had to do a lot more work:

• It had to iterate twice for every copied rather than once: once for Object.keys, and then over the returned keys
• It had to allocate (and then gC) an intermediate array for the keys.
• The second iteration had to happen in JS rather than in native code.

[lifeart]

@chriskrycho one more idea here: could we have cache for nodeType classes? Right now new class instance created for every node type, likely we should have only one base class per type

// existing codebase (compiled):

import { assign } from '@glimmer/util';
export function node(name) {
    if (name !== undefined) {
        const type = name;
        return {
            fields() {
                return class {
                    constructor(fields) {
                        this.type = type;
                        assign(this, fields);
                    }
                };
            },
        };
    }
    else {
        return {
            fields() {
                return class {
                    constructor(fields) {
                        assign(this, fields);
                    }
                };
            },
        };
    }
}

// proposed:

const nodeCache = {};

function klassForNode(name) {
    if ((!name in nodeCache)) {
        nodeCache[name] = class {
            constructor(fields) {
                if (name !== undefined) {
                    this.type = type;
                }
                assign(this, fields);
            }
        };
    }
    return nodeCache[name];
}

export function node(name) {
    return {
        fields() {
            return klassForNode(name); 
        },
    };
}

[chriskrycho]

@lifeart that's an interesting direction to explore (on a separate PR, of course).

However, notice that your proposed approach doesn't actually keep the semantics the same. The implementation intentionally creates a new base class for each invocation where name isn't supplied as well as in the case where it is, and your approach would clobber that. (That might be fine, but it isn't clear to me without further investigation!) Keeping the semantics the same would require piping around a bit more information than we currently do, because right now in a number of places it's only using the types to distinguish between the anonymous classes you get back from calling node() (vs. node(someName)).

It's definitely worth doing an experiment on, if you have time, and if you do I'm happy to pull it down and measure it in our app, but we'd definitely need to measure. The tradeoff there isn't super obvious to me. The cache would have its own overhead. Memory-wise, it should be minimal and possibly have fewer allocations—just a single extra Map instance and then fewer base classes allocated. CPU-wise, though, it would have to do at least one extra function dispatch and lookup for every invocation of node(). Could end up with lower memory but higher time!

[lifeart]

@chriskrycho could you try #1339?