Skip to content

perf(bytecode): elide arguments object instructions when its never used #686

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented May 2, 2025

What This PR Does

  • Makes oxc's semantic analysis information available when compiling scripts
  • Use that info to not generate instructions that create a function's arguments object when that function's body never references it

@aapoalas
Copy link
Member

aapoalas commented May 3, 2025

issue: I don't think you can take the Semantic out: a single source code can contain X amount of functions, and they compile separately.

Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

As mentioned, the semantics should remain usable multiple times over.

Otherwise, this is a great addition that will definitely be helpful in the future! Us not taking advantage of the semantic analysis oxc performs is a big mistake :)

@aapoalas aapoalas force-pushed the don/perf/elise-arguments-object branch from 04ad395 to 2f67c9a Compare May 19, 2025 13:52
@aapoalas
Copy link
Member

I did a refactoring pass on this because I wanted to merge this but found a couple of issues with using the Semantic's unresolved functions.

First, just checking the body isn't enough: we need to also check the formal parameters, but that was easy to solve.

A problem that is not easy to solve is that we don't get "arguments" as unresolved here:

var arguments = undefined;
function foo() {
  return arguments.length;
}

So the check says that the foo function doesn't need the arguments object, but it does.

Another issue is that in this function we again get the wrong result from the check:

function __func(){
    return typeof arguments;
    var arguments = "foo";
};

arguments is not unresolved, so the object doesn't get created.

A third issue is that eval can also mess with arguments, and of course this check doesn't guard for eval. So, that also needs checking.

It honestly seems like it'd be easier to not use the Semantic object at all, and instead just walk the AST and check references in it manually. That would also mean that Program could be handled like previously. Right now I had to move it into the SourceCode because Semantic holds a reference to Program.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants