Skip to content

Comments

fix(transformer/async-to-generator): arguments isn't correct after transformation#7234

Merged
graphite-app[bot] merged 1 commit intomainfrom
11-11-feat_transformer_async-to-generator_arguments_isn_t_correct_after_transformation
Nov 17, 2024
Merged

fix(transformer/async-to-generator): arguments isn't correct after transformation#7234
graphite-app[bot] merged 1 commit intomainfrom
11-11-feat_transformer_async-to-generator_arguments_isn_t_correct_after_transformation

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Nov 11, 2024

Fix due to this plugin transforming the async method and async arrow function, it caused arguments no longer point the original function.

For example:

Before

class Cls {
  async method() {
    () => {
    	console.log(arguments)
    }
  }
}

After:

class Cls {
  method() {
    var _arguments = arguments;
    return babelHelpers.asyncToGenerator(function* () {
      () => {
        console.log(_arguments);
      };
    })();
  }
}

In this way, the _arguments is its original function's arguments

For performance regression

It seems we need to check the IdentifierReference and BindingIdentifier if it's an arguments, that causes a significant regression, we may need a cheap way to do checking

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 11, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request labels Nov 11, 2024
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 11, 2024

CodSpeed Performance Report

Merging #7234 will degrade performances by 10.17%

Comparing 11-11-feat_transformer_async-to-generator_arguments_isn_t_correct_after_transformation (7d75130) with main (d445e0f)

Summary

❌ 3 regressions
✅ 27 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main 11-11-feat_transformer_async-to-generator_arguments_isn_t_correct_after_transformation Change
transformer[antd.js] 45.3 ms 48.7 ms -6.84%
transformer[checker.ts] 18 ms 20 ms -10.17%
transformer[pdf.mjs] 6.6 ms 7.1 ms -6.57%

@Dunqing Dunqing force-pushed the 11-11-feat_transformer_async-to-generator_arguments_isn_t_correct_after_transformation branch from 0476547 to 1de58d2 Compare November 11, 2024 15:52
@github-actions github-actions bot added the A-semantic Area - Semantic label Nov 11, 2024
@Dunqing Dunqing changed the title feat(transformer/async-to-generator): arguments isn't correct after transformation fix(transformer/async-to-generator): arguments isn't correct after transformation Nov 11, 2024
@github-actions github-actions bot added the C-bug Category - Bug label Nov 11, 2024
@Dunqing Dunqing force-pushed the 11-11-feat_transformer_async-to-generator_arguments_isn_t_correct_after_transformation branch from 1de58d2 to 52b99d2 Compare November 11, 2024 16:23
@Dunqing Dunqing changed the title fix(transformer/async-to-generator): arguments isn't correct after transformation fix(transformer/async-to-generator): arguments isn't correct after transformation Nov 11, 2024
@Dunqing Dunqing force-pushed the 11-11-feat_transformer_async-to-generator_arguments_isn_t_correct_after_transformation branch 2 times, most recently from b78a431 to 43e9c99 Compare November 12, 2024 04:55
@Dunqing Dunqing marked this pull request as ready for review November 12, 2024 05:14
@overlookmotel overlookmotel force-pushed the 11-11-feat_transformer_async-to-generator_arguments_isn_t_correct_after_transformation branch from 43e9c99 to b56cc1a Compare November 16, 2024 12:13
Copy link
Member

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've squashed the 3 commits and rebased on main, updating the code to take into account #7300. I caused that problem by delaying in reviewing this, so I've cleaned up the mess I made!

I can't say I really understand the logic of async function transform, so can't give a review on the "meat" of this PR. I've pushed a few commits with small optimizations and style nits. Feel free to revert any of them that you don't like.

I assume the perf impact is due to string comparisons on every single IdentifierReference and BindingIdentifier. Not much we can do about that at present I don't think.

I do think the fixtures should be altered, but @Dunqing feel free to merge and then do that in follow-up PR if you prefer.

Copy link
Member Author

Dunqing commented Nov 17, 2024

Merge activity

  • Nov 17, 12:01 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Nov 17, 12:01 AM EST: A user added this pull request to the Graphite merge queue.
  • Nov 17, 12:06 AM EST: A user merged this pull request with the Graphite merge queue.

…transformation (#7234)

Fix due to this plugin transforming the async method and async arrow function, it caused arguments no longer point the original function.

For example:

Before

```js
class Cls {
  async method() {
    () => {
    	console.log(arguments)
    }
  }
}
```

After:

```js
class Cls {
  method() {
    var _arguments = arguments;
    return babelHelpers.asyncToGenerator(function* () {
      () => {
        console.log(_arguments);
      };
    })();
  }
}
```

In this way, the `_arguments` is its original function's arguments

### For performance regression

It seems we need to check the IdentifierReference and BindingIdentifier if it's an `arguments`, that causes a significant regression, we may need a cheap way to do checking
@Dunqing Dunqing force-pushed the 11-11-feat_transformer_async-to-generator_arguments_isn_t_correct_after_transformation branch from 3aa07f7 to 7d75130 Compare November 17, 2024 05:01
@graphite-app graphite-app bot merged commit 7d75130 into main Nov 17, 2024
@graphite-app graphite-app bot deleted the 11-11-feat_transformer_async-to-generator_arguments_isn_t_correct_after_transformation branch November 17, 2024 05:06
Dunqing pushed a commit that referenced this pull request Nov 17, 2024
…eeds transform (#7321)

Track whether `arguments` needs to be transformed. If it doesn't, can skip expensive checks for whether `IdentifierReference`s and `BindingIdentifier`s names are `arguments` or not.

This recovers about half the performance hit of #7234.
Dunqing pushed a commit that referenced this pull request Nov 17, 2024
… aid inlining (#7322)

Move the cheap "does arguments need to be transformed" checks introduced in #7321 into `enter_identifier_reference` and `enter_binding_identifier`, and mark those methods `#[inline]`. These hot paths can then usually execute without a function call.

This wins back the other half of the perf hit of #7234.
Dunqing added a commit that referenced this pull request Nov 17, 2024
…transformation (#7234)

Fix due to this plugin transforming the async method and async arrow function, it caused arguments no longer point the original function.

For example:

Before

```js
class Cls {
  async method() {
    () => {
    	console.log(arguments)
    }
  }
}
```

After:

```js
class Cls {
  method() {
    var _arguments = arguments;
    return babelHelpers.asyncToGenerator(function* () {
      () => {
        console.log(_arguments);
      };
    })();
  }
}
```

In this way, the `_arguments` is its original function's arguments

### For performance regression

It seems we need to check the IdentifierReference and BindingIdentifier if it's an `arguments`, that causes a significant regression, we may need a cheap way to do checking
overlookmotel added a commit that referenced this pull request Nov 17, 2024
… aid inlining (#7322)

Move the cheap "does arguments need to be transformed" checks introduced in #7321 into `enter_identifier_reference` and `enter_binding_identifier`, and mark those methods `#[inline]`. These hot paths can then usually execute without a function call.

This wins back the other half of the perf hit of #7234.
overlookmotel added a commit that referenced this pull request Nov 17, 2024
… aid inlining (#7322)

Move the cheap "does arguments need to be transformed" checks introduced in #7321 into `enter_identifier_reference` and `enter_binding_identifier`, and mark those methods `#[inline]`. These hot paths can then usually execute without a function call.

This wins back the other half of the perf hit of #7234.
overlookmotel added a commit that referenced this pull request Nov 17, 2024
…eeds transform (#7321)

Track whether `arguments` needs to be transformed. If it doesn't, can skip expensive checks for whether `IdentifierReference`s and `BindingIdentifier`s names are `arguments` or not.

This recovers about half the performance hit of #7234.
overlookmotel added a commit that referenced this pull request Nov 17, 2024
… aid inlining (#7322)

Move the cheap "does arguments need to be transformed" checks introduced in #7321 into `enter_identifier_reference` and `enter_binding_identifier`, and mark those methods `#[inline]`. These hot paths can then usually execute without a function call.

This wins back the other half of the perf hit of #7234.
Dunqing added a commit that referenced this pull request Nov 18, 2024
…transformation (#7234)

Fix due to this plugin transforming the async method and async arrow function, it caused arguments no longer point the original function.

For example:

Before

```js
class Cls {
  async method() {
    () => {
    	console.log(arguments)
    }
  }
}
```

After:

```js
class Cls {
  method() {
    var _arguments = arguments;
    return babelHelpers.asyncToGenerator(function* () {
      () => {
        console.log(_arguments);
      };
    })();
  }
}
```

In this way, the `_arguments` is its original function's arguments

### For performance regression

It seems we need to check the IdentifierReference and BindingIdentifier if it's an `arguments`, that causes a significant regression, we may need a cheap way to do checking
Dunqing added a commit that referenced this pull request Nov 18, 2024
…transformation (#7234)

Fix due to this plugin transforming the async method and async arrow function, it caused arguments no longer point the original function.

For example:

Before

```js
class Cls {
  async method() {
    () => {
    	console.log(arguments)
    }
  }
}
```

After:

```js
class Cls {
  method() {
    var _arguments = arguments;
    return babelHelpers.asyncToGenerator(function* () {
      () => {
        console.log(_arguments);
      };
    })();
  }
}
```

In this way, the `_arguments` is its original function's arguments

### For performance regression

It seems we need to check the IdentifierReference and BindingIdentifier if it's an `arguments`, that causes a significant regression, we may need a cheap way to do checking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-merge Merge with Graphite Merge Queue A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler C-bug Category - Bug C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants