Skip to content

Possible performance improvement around types with this argument #54260

Open

Description

@beerose

Bug Report

I've found a hotspot in a graphql-code-generator project where a comparison of types was taking a significant amount of time:

CleanShot 2023-05-16 at 11 15 26@2x

It comes from a packages/presets/client/src/babel.ts file, and it's related to the usage of a declare function. Specifically, the comparison between PluginObj types was costly.

The declare function from Babel, has the following signature:

export function declare<
    O extends Record<string, any>,
    R extends babel.PluginObj = babel.PluginObj
>(
    builder: (api: BabelAPI, options: O, dirname: string) => R,
): (api: object, options: O | null | undefined, dirname: string) => R;

I added the first generic parameter to the usage of declare, and it successfully eliminated the hotspot:

type ClientBabelPresetOptions = {
  artifactDirectory?: string;
  gqlTagName?: string;
};
export default declare<ClientBabelPresetOptions>((api, opts): PluginObj => {
// ...

Trace after that change:
CleanShot 2023-05-16 at 11 09 40@2x

I understand that by doing this, TS could use a default for the second parameter, R, and hence skip the inference.

But here comes an interesting find — when I provided both generic parameters, it went back to the same (slow) performance result as in the beginning:

export default declare<ClientBabelPresetOptions, PluginObj>((api, opts): PluginObj => {

Note: using PluginObj vs PluginObj<PluginPass> or even PluginObj<any> yields the same problem.

I paired with Mateusz Burzyński (@Andarist), and he found out that even though the types looked the same, the target type (PluginObj) had an extra this argument. That led to getVariancesWorker being called on rather expensive types.

Now, the question is: why in checkTypeArguments the fillMissingTypeArguments doesn't use the this type anyhow but the constraint's type is obtained with the this type:

getTypeWithThisArgument(instantiateType(constraint, mapper), typeArgument)

If they would share the same ID then this particular case could work much faster.

🔎 Search Terms

performance, this, babel, getTypeWithThisArgument

🕗 Version & Regression Information

  • TypeScript version: 5.0.4

⏯ Playground Link

Playground link with relevant code

💻 Code

The code could be found here: https://github.com/dotansimha/graphql-code-generator/blob/86ec182887698742af8e9f47ffe39f07772e54a4/packages/presets/client/src/babel.ts#L16.

Currently, this file has an "improved" version, but upon playing with it you can notice long check times when:

  1. You remove ClientBabelPresetOptions
  2. You add a second generic parameter, PluginObj

To reproduce this in graphql-code-generator:

  1. Clone
  2. Install deps: yarn
  3. Run TSC

🙁 Actual behavior

Comparing two similar types takes a long time.

🙂 Expected behavior

Skip getVariancesWorker being called in this case.

Activity

  1. RyanCavanaugh commented on May 16, 2023

    @RyanCavanaugh
    Member

    Mateusz Burzyński (@Andarist), leaving this backlog because it seems like you might want to take it. But if not, let me know, and I will reassign it to someone on the team. Thanks to both of you for the neat investigation.

  2. Andarist commented on May 16, 2023

    @Andarist
    Contributor

    Ryan Cavanaugh (@RyanCavanaugh) I could try to investigate this further but right now I'm blocked~ on understanding the role of getTypeWithThisArgument and why it's only applied on contraint~ and not on the provided type arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Domain: PerformanceReports of unusually slow behaviorHelp WantedYou can do thisPossible ImprovementThe current behavior isn't wrong, but it's possible to see that it might be better in some cases

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions