Skip to content

Suboptimal compilation for rest parameters #19182

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

Closed
a-bronx opened this issue Oct 14, 2017 · 11 comments
Closed

Suboptimal compilation for rest parameters #19182

a-bronx opened this issue Oct 14, 2017 · 11 comments
Labels
Duplicate An existing issue was already created
Milestone

Comments

@a-bronx
Copy link

a-bronx commented Oct 14, 2017

There is an TS/ES.next idiom for the JS arguments array: (...args: any[]). But using it may produce suboptimal transpilation result, for example:

function debug(...args: any[]) {
    if (!settings.debug) return;
    console.log("DEBUG:", ...args);
}

produces following JavaScript (ES5) code:

function debug() {
    var args = [];
    for (var _i = 0; _i < arguments.length; _i++) {
        args[_i] = arguments[_i];
    }
    if (!settings.debug)
        return;
    console.log.apply(console, ["DEBUG:"].concat(args));
}

Note that the arguments is copied to the args array immediately at the beginning of the function, before its real usage. This causes the code executed every time the debug() function is called, even if it is unnecessary.

Expected behavior would be something more intelligent: either moving the arguments unrolling code closer to the usage place (may be tricky if there are multiple usage points), or using Array.prototype.slice.call(arguments) in-place without copying it into the temporary args.

Compare with Babel's transpilation result:

function debug() {
    var _console;

    if (!settings.debug) return;

    for (var _len = arguments.length, args = Array(_len), _key = 0; _key < _len; _key++) {
        args[_key] = arguments[_key];
    }

    (_console = console).log.apply(_console, ["DEBUG"].concat(args));
}

Note that here the copying was moved past the conditional expression and happens only when needed.

@DanielRosenwasser DanielRosenwasser changed the title Suboptimal spread operator transpilation Suboptimal compilation for rest parameters Oct 14, 2017
@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Oct 15, 2017
@RyanCavanaugh RyanCavanaugh added Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Oct 30, 2017
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Oct 30, 2017
@RyanCavanaugh
Copy link
Member

Accepting PRs for the following optimizations (doesn't need to have all of them):

  • Move the array copy emit to the first top-level statement which is before any statement which references it
  • Elide the array copy emit if it's never referenced
  • Elide the array copy emit if all accesses of the rest arg are of the form x[k] where k is a numeric literal, and replace k with k + n (where n is the correct offset)

@charlespierce
Copy link
Contributor

@Kingwl Are you still working on this issue? I'd like to take a swing at implementing some of the optimizations but don't want to duplicate work.

@Kingwl
Copy link
Contributor

Kingwl commented Nov 28, 2017

@charlespierce No, I'm not on it, please feel free

charlespierce added a commit to charlespierce/TypeScript that referenced this issue Nov 28, 2017
@charlespierce
Copy link
Contributor

PR Opened with implementation of the first two bullet points: #20307

@RyanCavanaugh
Copy link
Member

@ORESoftware please don't leave +1 comments; use the GitHub reactions feature (👍 on the issue). And there's certainly no point to multiple +1 comments on the same issue

@ORESoftware
Copy link

oh sorry I didn't realize I had already done that! yeah I haven't figured out how to use the reactions but I will now

@charlespierce
Copy link
Contributor

@RyanCavanaugh The PR for this issue #20307 is still awaiting review. I recognize it's a big diff, touching so many existing baselines, so is there anything I can do to help make it more manageable?

@Kingwl
Copy link
Contributor

Kingwl commented May 11, 2018

i'd like to work on this (again) 😅

@DanielRosenwasser
Copy link
Member

I think this is a duplicate of #498

@DanielRosenwasser DanielRosenwasser added Duplicate An existing issue was already created and removed Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Suggestion An idea for TypeScript Help Wanted You can do this labels Jul 8, 2020
@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@a-bronx
Copy link
Author

a-bronx commented Jul 10, 2020

I think it is a different issue. The #498 is about TS generating code for unused rest parameter. This issue is about rest parameters that are used in some (possibly rare) code paths, but generated code is placed into all code paths, making it suboptimal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

7 participants