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.

Sign up for GitHub

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

Jump to bottom

__createBinding performance issue on web browsers #165

Closed
jramsley opened this issue Nov 30, 2021 · 6 comments
Closed

__createBinding performance issue on web browsers #165

jramsley opened this issue Nov 30, 2021 · 6 comments

Comments

@jramsley
Copy link

__createBinding, which is used in __exportStar, uses accessor descriptors to get exported modules. In our product, we see a 10x performance degradation referencing exported enums this way compared to using a direct access object. We've verified that by refactoring our enum exports to be explicitly defined and not *, this issue is not seen.

Likewise, this issue is not seen in v1.14.0 when __createBinding looks like

    __createBinding = function(o, m, k, k2) {
        if (k2 === undefined) k2 = k;
        o[k2] = m[k];
    };

current implementation:

    __createBinding = Object.create ? (function(o, m, k, k2) {
        if (k2 === undefined) k2 = k;
        Object.defineProperty(o, k2, { enumerable: true, get: function() { return m[k]; } });
    }) : (function(o, m, k, k2) {
        if (k2 === undefined) k2 = k;
        o[k2] = m[k];
    });

JSBench:
https://jsbench.me/yokwmk1mau/1
elibarzilay added a commit to elibarzilay/TypeScript that referenced this issue Dec 2, 2021
@elibarzilay
optimize __createBinding
bd4b284 
When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

Also related to microsoft/tslib#165.
elibarzilay added a commit to elibarzilay/TypeScript that referenced this issue Dec 2, 2021
@elibarzilay
optimize __createBinding
0dc4b00 
When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

Also related to microsoft#46744 and to microsoft/tslib#165.
@elibarzilay elibarzilay mentioned this issue Dec 2, 2021
Merged
@DanielRosenwasser DanielRosenwasser mentioned this issue Dec 14, 2021
Closed
elibarzilay added a commit to elibarzilay/TypeScript that referenced this issue Jan 10, 2022
@elibarzilay
optimize __createBinding
7733452 
When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

In addition, reuse a descriptor if the bindings is marked as
non-writable and non-configurable, which makes a getter not
necessary.  (This can be done manually if needed, even though tsc
doesn't do it now.)

Also related to microsoft#46744 and to microsoft/tslib#165.
elibarzilay added a commit to elibarzilay/TypeScript that referenced this issue Jan 11, 2022
@elibarzilay
optimize __createBinding
2a983d8 
When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

In addition, reuse a descriptor if the bindings is marked as
non-writable and non-configurable, which makes a getter not
necessary.  (This can be done manually if needed, even though tsc
doesn't do it now.)

Also related to microsoft#46744 and to microsoft/tslib#165.
elibarzilay added a commit to elibarzilay/TypeScript that referenced this issue Jan 12, 2022
@elibarzilay
optimize __createBinding
0876b6b 
When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

In addition, reuse a descriptor if the bindings is marked as
non-writable and non-configurable, which makes a getter not
necessary.  (This can be done manually if needed, even though tsc
doesn't do it now.)

Also related to microsoft#46744 and to microsoft/tslib#165.
elibarzilay added a commit to microsoft/TypeScript that referenced this issue Jan 13, 2022
@elibarzilay
optimize __createBinding
ce18254 
When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

In addition, reuse a descriptor if the bindings is marked as
non-writable and non-configurable, which makes a getter not
necessary.  (This can be done manually if needed, even though tsc
doesn't do it now.)

Also related to #46744 and to microsoft/tslib#165.
elibarzilay added a commit that referenced this issue Jan 13, 2022
@elibarzilay
optimize __createBinding
95a77b9 
Reflect microsoft/TypeScript#46997:

When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

In addition, reuse a descriptor if the bindings is marked as
non-writable and non-configurable, which makes a getter not
necessary.  (This can be done manually if needed, even though tsc
doesn't do it now.)

Could be considered as a fix for #165 -- first, this PR prevents piling
up multiple layers of getters.  Second, it allows a hack of adding

    if (typeof exports === "object") exports = Object.freeze(exports);

to avoid getters altogether.  (And in the future, tsc could mark `const`
exports as non-writable and non-configurable which would make it
possible to avoid this hack.)
@elibarzilay elibarzilay mentioned this issue Jan 13, 2022
Merged
elibarzilay added a commit that referenced this issue Jan 14, 2022
@elibarzilay
optimize __createBinding
5ad0250 
Reflect microsoft/TypeScript#46997:

When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

In addition, reuse a descriptor if the bindings is marked as
non-writable and non-configurable, which makes a getter not
necessary.  (This can be done manually if needed, even though tsc
doesn't do it now.)

Could be considered as a fix for #165 -- first, this PR prevents piling
up multiple layers of getters.  Second, it allows a hack of adding

    if (typeof exports === "object") exports = Object.freeze(exports);

to avoid getters altogether.  (And in the future, tsc could mark `const`
exports as non-writable and non-configurable which would make it
possible to avoid this hack.)
rbuckton pushed a commit that referenced this issue Feb 8, 2022
@elibarzilay
optimize __createBinding (#168)
15f98b6 
Reflect microsoft/TypeScript#46997:

When the binding is itself one that was created by `__createBinding`,
re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.

In addition, reuse a descriptor if the bindings is marked as
non-writable and non-configurable, which makes a getter not
necessary.  (This can be done manually if needed, even though tsc
doesn't do it now.)

Could be considered as a fix for #165 -- first, this PR prevents piling
up multiple layers of getters.  Second, it allows a hack of adding

    if (typeof exports === "object") exports = Object.freeze(exports);

to avoid getters altogether.  (And in the future, tsc could mark `const`
exports as non-writable and non-configurable which would make it
possible to avoid this hack.)
@jakebailey
Copy link
Member

With #168 merged, is this issue fixed? @jramsley can you still reproduce this in the latest tslib?

@jramsley
Copy link
Author

It helps, but it is still tangibly slower using descriptors than accessing the object directly. It is still a 2x performance degradation compared to overwriting tslib with

    __createBinding = function(o, m, k, k2) {
        if (k2 === undefined) k2 = k;
        o[k2] = m[k];
    };

@ExE-Boss
Copy link
Contributor

@jramsley

    __createBinding = function(o, m, k, k2) {
        if (k2 === undefined) k2 = k;
        o[k2] = m[k];
    };

The issue with that is that if m[k] is later mutated (or is a getter), the mutation won’t propagate when accessing o[k2], like it does in the case of real ES modules.

@jakebailey
Copy link
Member

jakebailey commented Jun 23, 2022 •
edited

Yeah, that's the case I was trying to remember (thanks for remembering). One can write:

// @filename: /mod.ts
export let foo = 1234;

// @filename: /index.ts
import { foo } from "./mod"

And foo in the second module will change when mod.ts changes its value.

@jramsley
Copy link
Author

The issue with that is that if m[k] is later mutated (or is a getter), the mutation won’t propagate when accessing o[k2], like it does in the case of real ES modules.

This is correct, but for our use case the * import is for classes and enums which aren't being mutated.

@jakebailey
Copy link
Member

Unfortunately, I don't think TypeScript is going to be able to provide the info required to prove that we can make that optimization; if you're doing export * from "./otherFile" in some file, it can't know which of those bindings are mutable and which aren't (and that can continue onward as it's reexported). I think that this is about as fast as we're going to be able to make it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ExE-Boss @jramsley @jakebailey