support ObjectRestSpread transformation on modern targets for V8 perf

[KeunwooLee-at]

Hi. At Airtable, we've been experimenting with esbuild in our toolchain on a large modern Typescript project, and in general we think it's amazing. However, we discovered an unexpected runtime performance regression in the generated JS.

Formerly, our (Babel-based) transpilation process would transform object spreads into invocations of a polyfill. esbuild, with es2018 and later targets, will pass through object spreads as-is. This triggers a surprisingly large perf hit in V8 when using spreads on large objects:
https://bugs.chromium.org/p/v8/issues/detail?id=11536
Empirically, we have unfortunately discovered that this regression (particularly the memory behavior) is large enough that we can't deploy esbuild in production as-is.

Unfortunately, we use BigInt and other modern features in our codebase, and therefore we cannot simply change the esbuild target to es2017 (or, in fact, any other target: the jsTable entries in js_table.go for ObjectRestSpread are all strictly earlier than the entries for BigInt).

We would love to find a solution to situations like this, aside from postprocessing esbuild output with another transpiler, especially because esbuild already has the infrastructure internally to transform object spreads.

We understand that supporting arbitrary subsets of language features as a compilation target is a non-goal. However, what do you think about adding a way of specifying output constraints that target workarounds for known major perf issues in JS engine versions? As a straw man, this could look something like this:

• a new Engine in js_table.go named V8 (or V8Perf, or...), which is defined not to support ObjectRestSpread (currently; if V8 closes the issue linked above, we can note that support, with the appropriate version number).
• add this engine to command line parsing in cli_impl.go in the obvious way

I think this would get us everything we want, without doing violence to esbuild's architecture or design goals. We can put together a PR along these lines if you'd like, but I thought I'd discuss it first since it could be controversial.

Also open to other approaches of course.

[evanw]

Thanks for the report. It's good to be aware of this issue. Some thoughts:

• I'd like to hear what the V8 team has to say first. Although I can reproduce the performance issue myself (2.2-2.3x slower). This is a bummer but it isn't the first time esbuild has had to work around performance cliffs with modern ECMAScript features (another example: Safari deopt with a large ESBuild bundle #478).

• I try to avoid adding random hyper-specific configuration options to esbuild to keep esbuild's API approachable. Also adding a target called "V8" is confusing IMO. V8 is already represented in the compatibility table multiple times (as Chrome/Edge/Node). If it's really a bad idea to use this feature in V8, another alternative could be to just remove support for it from the existing targets which would cause it to be transpiled. Then people would automatically benefit from improved performance without having to discover this obscure configuration preference. Support could be added back once V8 fixes the performance of this syntax.

[KeunwooLee-at]

Thanks for the quick & thoughtful reply!

It looks like the V8 team has assigned the issue. Waiting a bit to see what they do before committing to a plan makes sense.

Just speaking for my team, we'd be happy with your suggestion, i.e.:

1. disable ObjectSpreadRest on the existing V8 targets.
2. re-enable it behind the appropriate versions if/when a fix ships in Chrome/Edge/Node.

p.s. FYI the 2x slowdown is considerably less serious, for us, than the heap behavior. V8 generates a stupendous number of hidden classes for high cardinality object spreads with variable keysets, which leads to the gc behavior documented in the issue my colleague filed against V8.

[evanw]

Ah is that what's happening. No wonder it's so slow. I think it's ok to ship this without waiting for V8 then. Here's a demonstration of the improvement after this change:

$ node v8perf.js
objectSpread 3483ms
objectSpreadAssign 1446ms
objectRest 1458ms
objectRestAssign 636ms

$ esbuild --target=node14 v8perf.js | node
objectSpread 1353ms
objectSpreadAssign 1339ms
objectRest 696ms
objectRestAssign 630ms

Click to expand v8perf.js

function objectSpread(m, n) {
  return { ...m, ...n };
}

function objectSpreadAssign(m, n) {
  return Object.assign({}, m, n);
}

function objectRest(m) {
  var { ...o } = m;
  return o;
}

function objectRestAssign(m) {
  return Object.assign({}, m);
}

function testObjectSpread(i) {
  const m = {}, n = {};
  for (let k = 0; k < i; k++) {
    m[Math.random()] = 0;
    n[Math.random()] = 0;
  }
  return objectSpread(m, n);
}

function testObjectSpreadAssign(i) {
  const m = {}, n = {};
  for (let k = 0; k < i; k++) {
    m[Math.random()] = 0;
    n[Math.random()] = 0;
  }
  return objectSpreadAssign(m, n);
}

function testObjectRest(i) {
  const m = {}
  for (let k = 0; k < i; k++) {
    m[Math.random()] = 0;
  }
  return objectRest(m);
}

function testObjectRestAssign(i) {
  const m = {}
  for (let k = 0; k < i; k++) {
    m[Math.random()] = 0;
  }
  return objectRestAssign(m);
}

{
  let start = Date.now();
  const a = [];
  for (let j = 0; j < 5625; j++) {
    a.push(testObjectSpread(75));
  }
  let end = Date.now();
  console.log(`objectSpread ${end - start}ms`);
}

{
  let start = Date.now();
  const a = [];
  for (let j = 0; j < 5625; j++) {
    a.push(testObjectSpreadAssign(75));
  }
  let end = Date.now();
  console.log(`objectSpreadAssign ${end - start}ms`);
}

{
  let start = Date.now();
  const a = [];
  for (let j = 0; j < 5625; j++) {
    a.push(testObjectRest(75));
  }
  let end = Date.now();
  console.log(`objectRest ${end - start}ms`);
}

{
  let start = Date.now();
  const a = [];
  for (let j = 0; j < 5625; j++) {
    a.push(testObjectRestAssign(75));
  }
  let end = Date.now();
  console.log(`objectRestAssign ${end - start}ms`);
}

[evanw]

FYI: I am planning to change the generated code for this case due to #1017.

It turns out Object.assign is not a correct implementation of object spread due to the use of assignment vs. property definition semantics. A correct implementation would need to loop over each property and call Object.defineProperty to copy it over. This only matters for unusual objects such as those that have a getter in their prototype chain.

However, I have tested the performance for this and it looks like a manual loop that copies the properties over one-by-one is somehow even faster than calling the built-in Object.assign function, at least on this benchmark. So it doesn't appear to be a performance regression.

[KeunwooLee-at]

Thanks for the update! FWIW the Babel transpilation pipeline we were using previously generated this polyfill:

function _objectSpread(target) {
    for (var i = 1; i < arguments.length; i++) {
        var source = arguments[i] != null ? arguments[i] : {};
        if (i % 2) {
            ownKeys(Object(source), true).forEach(function (key) {
                _defineProperty(target, key, source[key]);
            });
        } else if (Object.getOwnPropertyDescriptors) {
            Object.defineProperties(target, Object.getOwnPropertyDescriptors(source));
        } else {
            ownKeys(Object(source)).forEach(function (key) {
                Object.defineProperty(target, key, Object.getOwnPropertyDescriptor(source, key));
            });
        }
    }
    return target;
}
function _defineProperty(obj, key, value) {
    if (key in obj) {
        Object.defineProperty(obj, key, {
            value: value,
            enumerable: true,
            configurable: true,
            writable: true,
        });
    } else {
        obj[key] = value;
    }
    return obj;
}

and this was the baseline against which we originally observed the perf regression in native object spread. So we would also expect a loop with defineProperty calls to be faster than native spread in current V8. (Yes, we were pretty surprised that the above mess is faster.)

[evanw]

Heads up that this behavior has changed recently in 0.14.46. TL;DR you now need to do --supported:object-rest-spread=false to get this behavior.