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

Drop superfluous __name() calls #2062

Merged
merged 9 commits into from
Mar 14, 2022
Merged

Drop superfluous __name() calls #2062

merged 9 commits into from
Mar 14, 2022

Conversation

indutny
Copy link
Contributor

@indutny indutny commented Feb 27, 2022

When the output is generated with --keep-names flag -
__name(fn, "name") statements are inserted for every function/class
with a name. However, in many situations the renamer doesn't change
the name of the function/class, and thus the call to __name has no
effect other than imposing an extra runtime cost on the application.

This change adds IsKeepName flag to ECall struct and uses it in
js_printer to detect such situations and omit the __name() from being
printed. Sadly, it would be hard to drop it completely without
--minify flag, but at least we can drop the runtime cost associated
with it by just replacing the __name() with its first argument in such
situations.

@indutny
Copy link
Contributor Author

indutny commented Feb 27, 2022

cc @evanw ?

@indutny
Drop superfluous __name() calls
02db122 
When the output is generated with `--keep-names` flag -
`__name(fn, "name")` statements are inserted for every function/class
with a name. However, in many situations the renamer doesn't change
the name of the function/class, and thus the call to `__name` has no
effect other than imposing an extra runtime cost on the application.

This change adds `IsKeepName` flag to `ECall` struct and uses it in
js_printer to detect such situations and omit the `__name()` from being
printed. Sadly, it would be hard to drop it completely without
`--minify` flag, but at least we can drop the runtime cost associated
with it by just replacing the `__name()` with its first argument in such
situations.
@indutny indutny force-pushed the feature/drop-unused-keep-name branch from 2869f7d to 02db122 Compare February 27, 2022 19:42
@indutny-signal
Copy link

indutny-signal commented Feb 28, 2022 •
edited
Loading

This still leaves:

          var memoized = /* @__PURE__ */ __name(function() {
            // ...
          }, "memoized");

which should probably dropped too, but it might be too heavy of an optimization for the compiler. Any thoughts?

Once could argue that keeping name in this case may not be necessary? I can see how V8 would report different stack traces in case name changed, but I'm not sure how the expectation for this behavior are set amongst users.

@indutny
Copy link
Contributor Author

indutny commented Mar 14, 2022

Oh wow! This PR looks much better now. Thanks @evanw !

@evanw
Copy link
Owner

evanw commented Mar 14, 2022

which should probably dropped too, but it might be too heavy of an optimization for the compiler. Any thoughts?

This one is pretty trivial. I just added code for that too.

Once could argue that keeping name in this case may not be necessary? I can see how V8 would report different stack traces in case name changed, but I'm not sure how the expectation for this behavior are set amongst users.

This is the same as the other cases. The name preservation is not necessary in this case either. It's true that just like the other cases, changing the name of the relevant identifier changes the .name property. But if you're running another tool after esbuild runs that changes identifier names, using esbuild's --keep-names feature alone isn't enough. You'd need to make sure the tool that has an equivalent of the --keep-names feature as well. It's not esbuild's responsibility to ensure that name is preserved through such a transformation.

@evanw evanw merged commit 730df4f into evanw:master Mar 14, 2022
@indutny indutny deleted the feature/drop-unused-keep-name branch March 14, 2022 01:09
@indutny
Copy link
Contributor Author

indutny commented Mar 14, 2022

Thanks for merging! You are awesome ❤️

zhusjfaker pushed a commit to speedy-js/esbuild that referenced this pull request Mar 28, 2022
@indutny @zhusjfaker
Drop superfluous __name() calls (evanw#2062)
0e01f21
hardfist pushed a commit to speedy-js/esbuild that referenced this pull request Mar 28, 2022
@indutny @hardfist
Drop superfluous __name() calls (evanw#2062)
22acf58
@evanw evanw mentioned this pull request Apr 3, 2022
Closed
@evanw
Copy link
Owner

evanw commented Apr 7, 2022

FYI: I am reverting this PR to fix #2149. It turns out the assumption that the symbol initializer should get the correct name if the symbol has the correct name is incorrect in the case when the symbol initializer is moved into a temporary variable, which happens sometimes during esbuild's syntax transformations.

evanw added a commit that referenced this pull request Apr 7, 2022
@evanw
Revert "Drop superfluous __name() calls (#2062)"
046f82f 
This reverts commit 730df4f.
evanw added a commit that referenced this pull request Apr 7, 2022
@evanw
fix #2149: changelog entry about reverting #2062
cbe422f
@indutny-signal
Copy link

Oh, sorry for that! This is probably going to be tricky to fix correctly. 😭

tmattio added a commit to tmattio/opam-repository that referenced this pull request Apr 9, 2022
@tmattio
[new release] esbuild (0.14.34)
6701b08 
CHANGES:

* Fix a regression regarding `super` ([tmattio/opam-esbuild#2158](evanw/esbuild#2158))

    This fixes a regression from the previous release regarding classes with a super class, a private member, and a static field in the scenario where the static field needs to be lowered but where private members are supported by the configured target environment. In this scenario, esbuild could incorrectly inject the instance field initializers that use `this` into the constructor before the call to `super()`, which is invalid. This problem has now been fixed (notice that `this` is now used after `super()` instead of before):

    ```js
    // Original code
    class Foo extends Object {
      static FOO;
      constructor() {
        super();
      }
      #foo;
    }

    // Old output (with --bundle)
    var _foo;
    var Foo = class extends Object {
      constructor() {
        __privateAdd(this, _foo, void 0);
        super();
      }
    };
    _foo = new WeakMap();
    __publicField(Foo, "FOO");

    // New output (with --bundle)
    var _foo;
    var Foo = class extends Object {
      constructor() {
        super();
        __privateAdd(this, _foo, void 0);
      }
    };
    _foo = new WeakMap();
    __publicField(Foo, "FOO");
    ```

    During parsing, esbuild scans the class and makes certain decisions about the class such as whether to lower all static fields, whether to lower each private member, or whether calls to `super()` need to be tracked and adjusted. Previously esbuild made two passes through the class members to compute this information. However, with the new `super()` call lowering logic added in the previous release, we now need three passes to capture the whole dependency chain for this case: 1) lowering static fields requires 2) lowering private members which requires 3) adjusting `super()` calls.

    The reason lowering static fields requires lowering private members is because lowering static fields moves their initializers outside of the class body, where they can't access private members anymore. Consider this code:

    ```js
    class Foo {
      get #foo() {}
      static bar = new Foo().#foo
    }
    ```

    We can't just lower static fields without also lowering private members, since that causes a syntax error:

    ```js
    class Foo {
      get #foo() {}
    }
    Foo.bar = new Foo().#foo;
    ```

    And the reason lowering private members requires adjusting `super()` calls is because the injected private member initializers use `this`, which is only accessible after `super()` calls in the constructor.

* Fix an issue with `--keep-names` not keeping some names ([tmattio/opam-esbuild#2149](evanw/esbuild#2149))

    This release fixes a regression with `--keep-names` from version 0.14.26. PR [tmattio/opam-esbuild#2062](evanw/esbuild#2062) attempted to remove superfluous calls to the `__name` helper function by omitting calls of the form `__name(foo, "foo")` where the name of the symbol in the first argument is equal to the string in the second argument. This was assuming that the initializer for the symbol would automatically be assigned the expected `.name` property by the JavaScript VM, which turned out to be an incorrect assumption. To fix the regression, this PR has been reverted.

    The assumption is true in many cases but isn't true when the initializer is moved into another automatically-generated variable, which can sometimes be necessary during the various syntax transformations that esbuild does. For example, consider the following code:

    ```js
    class Foo {
      static get #foo() { return Foo.name }
      static get foo() { return this.#foo }
    }
    let Bar = Foo
    Foo = { name: 'Bar' }
    console.log(Foo.name, Bar.name)
    ```

    This code should print `Bar Foo`. With `--keep-names --target=es6` that code is lowered by esbuild into the following code (omitting the helper function definitions for brevity):

    ```js
    var _foo, foo_get;
    const _Foo = class {
      static get foo() {
        return __privateGet(this, _foo, foo_get);
      }
    };
    let Foo = _Foo;
    __name(Foo, "Foo");
    _foo = new WeakSet();
    foo_get = /* @__PURE__ */ __name(function() {
      return _Foo.name;
    }, "#foo");
    __privateAdd(Foo, _foo);
    let Bar = Foo;
    Foo = { name: "Bar" };
    console.log(Foo.name, Bar.name);
    ```

    The injection of the automatically-generated `_Foo` variable is necessary to preserve the semantics of the captured `Foo` binding for methods defined within the class body, even when the definition needs to be moved outside of the class body during code transformation. Due to a JavaScript quirk, this binding is immutable and does not change even if `Foo` is later reassigned. The PR that was reverted was incorrectly removing the call to `__name(Foo, "Foo")`, which turned out to be necessary after all in this case.

* Print some large integers using hexadecimal when minifying ([tmattio/opam-esbuild#2162](evanw/esbuild#2162))

    When `--minify` is active, esbuild will now use one fewer byte to represent certain large integers:

    ```js
    // Original code
    x = 123456787654321;

    // Old output (with --minify)
    x=123456787654321;

    // New output (with --minify)
    x=0x704885f926b1;
    ```

    This works because a hexadecimal representation can be shorter than a decimal representation starting at around 10<sup>12</sup> and above.

    _This optimization made me realize that there's probably an opportunity to optimize printed numbers for smaller gzipped size instead of or in addition to just optimizing for minimal uncompressed byte count. The gzip algorithm does better with repetitive sequences, so for example `0xFFFFFFFF` is probably a better representation than `4294967295` even though the byte counts are the same. As far as I know, no JavaScript minifier does this optimization yet. I don't know enough about how gzip works to know if this is a good idea or what the right metric for this might be._

* Add Linux ARM64 support for Deno ([tmattio/opam-esbuild#2156](evanw/esbuild#2156))

    This release adds Linux ARM64 support to esbuild's [Deno](https://deno.land/) API implementation, which allows esbuild to be used with Deno on a Raspberry Pi.
@tmattio tmattio mentioned this pull request Apr 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@indutny @indutny-signal @evanw