@babel/plugin-transform-classses(createSuper)@7.9.x may have performance issue

[shrinktofit]

Classes with super class are very slow to new, in comparison to what babel@7.8.8 does. The difference can be found at here.

function test(Vec3) {
    const t1 = Date.now();
    (() => {
        for (let i = 0; i < 1000000; ++i) {
            const x = Math.random();
            new Vec3(x, x, x); // Key line
        }
    })();
    const t2 = Date.now();
    console.log(`Past: ${t2 - t1}ms`);
}

test(require('./out/7.8.8/vec3.js').Vec3); // 18ms
test(require('./out/7.9.x/vec3.js').Vec3); // Oops, 2451ms!!

I have upload the test code to this repository. You can try it just by node ./run-perf.js.

[babel-bot]

Hey @shrinktofit! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

[jridgewell]

Unfortunately, this is the cost of Reflect.construct with a NewTarget. We could better optimize _createSuper to avoid duplicate code checks and return specialized wrappers based on environment, but we're still going to be much slower.

[nicolo-ribaudo]

We can at least disable createSuper (or use a reduced version) in loose mode.

In "normal mode" the first goal is to be spec-compliant and fully compatible, but in loose mode we can assume that everything is used "normally" and prefer speed over correctness.

Also @shrinktofit any help in optimizing the current helper would be appreciated 🙏

[jridgewell]

We can at least disable createSuper (or use a reduced version) in loose mode.

Oh, I thought we avoided that for loose already. We should definitely do that.

[nicolo-ribaudo]

@shrinktofit I investigated a bit the performance issue, here is what I found:

• The performance regression only happens in engines with native Reflect.construct support. You can test it, for example, in Node.js 5 (I had to replace let/const with var, and to remove destructuring).
• All the engines I know with Reflect.construct also support native classes, so maybe you can avoid transpiling them for those engines.
• When using loose mode, there isn't any performance difference between 7.8.8 and 7.9.x
• If in the createSuper helper we move the call to _isNativeReflectConstruct in the outer function, we get a ~5% speed improvement.

[shrinktofit]

When using loose mode, there isn't any performance difference between 7.8.8 and 7.9.x

This looks like the most appropriate solution for us, for now, thank you~

@nicolo-ribaudo BTW, why can't we substitute Reflect.construct with operator new?

[nicolo-ribaudo]

It's because with new you can't specify the third parameter of Reflect.construct (new.target).

[benmccann]

Is there anyway to turn off just this feature without enabling loose? It's causing a 10x slowdown on our library overall, but our code breaks with loose

[jridgewell]

You could delete Reflect.construct, which should bring it back to almost 7.8's speed.

Or, we may be able to add an explicit option to the transorm?

[nicolo-ribaudo]

I'd be ok with a assumeNoExtendNative option to the classes plugin.

[shrinktofit]

I'd be ok with a assumeNoExtendNative option to the classes plugin.

@nicolo-ribaudo Any progress on this option?

[JLHwung]

On the assumeNoExtendNative, we can consider add it to the "assumptions" introduced in #12219

[nicolo-ribaudo]

I want to keep that RFC/PR without adding new capabilities to the transforms (so that it only focuses on moving the assumptions from plugins to the top level), but yes this is a good candidate for a new assumption.

[shrinktofit]

@nicolo-ribaudo Will this be added into the big list of "assumptions"?

[nicolo-ribaudo]

Now that we shipped assumptions in 7.13.0, I was looking into this. Does the superIsCallableConstructor assumption work for you? It basically skips the createSuper call.

[nicolo-ribaudo]

I'm closing this since I think superIsCallableConstructor does exactly what's needed for this feature request. If I'm wrong ping me and I'll reopen this issue.