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.

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

Explicit resource management (using) transform improvements #9699

Open
3 of 8 tasks
overlookmotel opened this issue Mar 12, 2025 · 3 comments
Open
3 of 8 tasks

Explicit resource management (using) transform improvements #9699

overlookmotel opened this issue Mar 12, 2025 · 3 comments
Assignees
Labels
A-transformer Area - Transformer / Transpiler

Comments

@overlookmotel
Copy link
Contributor

overlookmotel commented Mar 12, 2025

#9310 implemented explicit resource management (using declarations) transform.

A few tasks for us as follow-ups:

cc @camc314.

@overlookmotel overlookmotel added the A-transformer Area - Transformer / Transpiler label Mar 12, 2025
@overlookmotel
Copy link
Contributor Author

overlookmotel commented Mar 12, 2025

@camc314 has pointed out a problem with moving transform into exit phase to improve perf - there's an incorrect interaction with async-to-generator transform.

Currently transforming this works fine:

async function fn() {
    await using x = whatever();
}

But if we move transforming the function into exit_function or exit_statements, then the output is:

function fn() {
  return _fn.apply(this, arguments);
}
function _fn() {
  _fn = babelHelpers.asyncToGenerator(function*() {
    try {
      var _usingCtx = babelHelpers.usingCtx();
      const x = _usingCtx.a(y);
    } catch (_) {
      _usingCtx.e = _;
    } finally {
      await _usingCtx.d(); // <-- `await` invalid here. Should be `yield`.
    }
  });
  return _fn.apply(this, arguments);
}

Problem is that using transform adds await _usingCtx.d() after async-to-generator has transformed the function, so it doesn't convert the await to yield.

Unclear what best way to solve this is.

@overlookmotel
Copy link
Contributor Author

@camc314 I See you closed the PR stack up to #9716. Sorry I was slow to get to it. I was reviewing it when you closed it!

I am guessing the reason for the perf regression was the number of stacks. I would have thought that a single stack would be sufficient. I assume that it's either a static block or a statement block or a switch statement that has to handle the using statements, so they could all use the same stack.

Or, as you suggested previously, an FxHashSet<Address> containing the Addresses of nodes (statement block/program/etc) which contain a using declaration would work too, and might be more performant when the hashmap is empty (no using declarations).

@camc314
Copy link
Contributor

camc314 commented Mar 18, 2025

all good, i'm going to fix the semantic issues first and get it correct, then. come back to perf. the changes to the soping in oxc_semantic cause the semantic stuff to no longer match

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

No branches or pull requests

2 participants