fix(awslint): linting is slow #27860

mrgrain · 2023-11-06T19:57:04Z

Reduces runtime of awslint against aws-cdk-lib from ~70s down to ~15s.

Speed up 1: Reduce rule definitions (~1s)
Speed up 2: Make core checks fqn based only (~5s)
Speed up 3: Optimize code paths to defer expensive checks (~4s)
Speed up 4: Locked typesystem (~25s)
Speed up 5: Faster camel casing (~15s)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mrgrain · 2023-11-09T15:53:49Z

packages/awslint/.eslintrc.js

@@ -15,7 +15,7 @@ module.exports = {
  parserOptions: {
    ecmaVersion: '2018',
    sourceType: 'module',
-    project: './tsconfig.json',
+    project: __dirname + '/tsconfig.json',


This was just broken before 🤷🏻

mrgrain · 2023-11-09T15:54:49Z

packages/awslint/bin/awslint.ts

+    process.exitCode = 1;
+    return;


Unrelated change to use best practice.

mrgrain · 2023-11-09T15:55:17Z

packages/awslint/bin/awslint.ts

 });

 async function loadModule(dir: string) {
  const ts = new reflect.TypeSystem();
  await ts.load(dir, { validate: false }); // Don't validate to save 66% of execution time (20s vs 1min).
  // We run 'awslint' during build time, assemblies are guaranteed to be ok.

+  // We won't load any more assemblies. Lock the typesystem to benefit from performance improvements.
+  ts.lock();


This enables performance improvements in jsii-reflect that rely on a locked typesystem.

mrgrain · 2023-11-09T15:56:31Z

packages/awslint/lib/case.ts

@@ -0,0 +1,18 @@
+import * as changeCase from 'change-case';


I tested a bunch.

fast-case is the fastest, but cannot deal with cases like DBProxy (converts it to dBProxy)
lodash and change-case are of similar speed
camelcase is by far the slowest.

mrgrain · 2023-11-09T15:57:00Z

packages/awslint/lib/case.ts

@@ -0,0 +1,18 @@
+import * as changeCase from 'change-case';
+
+function cachedTransform(transform: (x: string) => string): (x: string) => string {


Throwing this in for good measure, so we never convert the same string twice. It saves 3-4s.

mrgrain · 2023-11-09T15:59:04Z

packages/awslint/lib/rules/api.ts

@@ -126,7 +126,7 @@ apiLinter.add({
          return;
        }

-        if (type.type.fqn === e.ctx.core.constructClass.fqn) {
+        if (type.type.fqn === e.ctx.core.baseConstructClassFqn) {


Bypasses loading of the type just to compare the fqn.
I am making the assumption here that our core types are well tested and definitely exist in the assembly. If not jsii would blow up anyway.

mrgrain · 2023-11-09T16:02:31Z

packages/awslint/lib/rules/construct.ts

-    let baseType;
-    if (process.env.AWSLINT_BASE_CONSTRUCT && !CoreTypes.isCfnResource(e.ctx.classType)) {
-      baseType = e.ctx.core.baseConstructClass;
-    } else {
-      baseType = e.ctx.core.constructClass;
-    }


This check was redundant because e.ctx.core.baseConstructClass and e.ctx.core.constructClass are the same.

mrgrain · 2023-11-09T16:03:21Z

packages/awslint/lib/rules/core-types.ts

-    for (const fqn of Object.values(CoreTypesFqn)) {
-      if (!this.sys.tryFindFqn(fqn)) {
-        throw new Error(`core FQN type not found: ${fqn}`);
-      }
-    }


This is already checked by jsii

mrgrain · 2023-11-09T16:27:33Z

packages/awslint/lib/rules/imports.ts

-      const baseType = process.env.AWSLINT_BASE_CONSTRUCT ? e.ctx.resource.core.baseConstructClass :
-        e.ctx.resource.core.constructClass;
+      const baseType = e.ctx.resource.core.baseConstructClass;


Both values are the same.

mrgrain · 2023-11-09T16:27:42Z

packages/awslint/lib/rules/imports.ts

-    const baseType = process.env.AWSLINT_BASE_CONSTRUCT ? e.ctx.resource.core.baseConstructClass
-      : e.ctx.resource.core.constructClass;
+    const baseType = e.ctx.resource.core.baseConstructClass;


Both values are the same.

mrgrain · 2023-11-09T16:28:48Z

packages/awslint/lib/rules/resource.ts

-      .findAllConstructs(assembly)
-      .filter(c => CoreTypes.isResourceClass(c.classType))


This loops twice over all constructs.

mrgrain · 2023-11-09T16:29:06Z

packages/awslint/lib/rules/resource.ts

@@ -266,7 +265,7 @@ function tryResolveCfnResource(resourceClass: reflect.ClassType): CfnResourceRef
  }

  // try to resolve through ancestors
-  for (const base of resourceClass.getAncestors()) {
+  for (const base of resourceClass.ancestors) {


Using the memoized version of getAncestors

Speed up 2: Make core checks fqn based only (~4s) Speed up 3: Optimize code paths to defer expensive checks (~3s) Speed up 4: Locked typesystem (~25s) Speed up 5: Faster camel casing (~15s)

mrgrain · 2023-11-09T16:29:36Z

packages/awslint/lib/rules/util.ts

-    if (documentable.overrides.isClassType() || documentable.overrides.isInterfaceType()) {
-      const baseMembers = documentable.overrides.allMembers.filter(m => m.name === documentable.name);
+  if ((documentable instanceof reflect.Property || documentable instanceof reflect.Method)) {
+    const overrides = documentable.overrides;


Make sure we only query (the expensive) overrides if we really need to.

mrgrain · 2023-11-17T15:45:11Z

packages/aws-cdk-lib/awslint.json

Folding a bunch of rules into * rules. This is mostly enums.

vinayak-kukreja

This is amazing 😍

vinayak-kukreja · 2023-11-17T16:57:31Z

packages/awslint/lib/rules/construct.ts

-  /**
-   * @deprecated - use `CoreTypes.constructClass()` or `CoreTypes.baseConstructClass()` as appropriate
-   */
-  public readonly ROOT_CLASS: reflect.ClassType; // constructs.Construct


Are we the only ones using this package? If not, this would be breaking for any users referring it.

It's public but in "dev preview". Has been for ages.
https://www.npmjs.com/package/awslint

Never mind, this isn't even a public API. It's not exported at the top-level.

It's also not a library. It's a cli tool. 😅

mergify · 2023-11-17T18:08:42Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-11-17T18:36:22Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 6f2f40e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-11-17T18:36:45Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation requested a review from a team November 6, 2023 19:57

github-actions bot added the p2 label Nov 6, 2023

mergify bot added the contribution/core This is a PR that came from AWS. label Nov 6, 2023

This comment was marked as outdated.

Sign in to view

mrgrain force-pushed the mrgrain/chore/awslint-go-vroom-vroom branch 4 times, most recently from b7d0ebd to 24a93f8 Compare November 9, 2023 15:49

mrgrain added pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Nov 9, 2023

mrgrain commented Nov 9, 2023

View reviewed changes

Speed up 1: Reduce rule definitions (~1s)

9bb8e6b

Speed up 2: Make core checks fqn based only (~4s) Speed up 3: Optimize code paths to defer expensive checks (~3s) Speed up 4: Locked typesystem (~25s) Speed up 5: Faster camel casing (~15s)

mrgrain force-pushed the mrgrain/chore/awslint-go-vroom-vroom branch from 24a93f8 to 9bb8e6b Compare November 17, 2023 15:38

mrgrain marked this pull request as ready for review November 17, 2023 15:39

mrgrain commented Nov 17, 2023

View reviewed changes

packages/aws-cdk-lib/awslint.json Outdated

Copy link

Contributor Author

mrgrain Nov 17, 2023

Choose a reason for hiding this comment

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

Folding a bunch of rules into * rules. This is mostly enums.

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 17, 2023

vinayak-kukreja self-assigned this Nov 17, 2023

vinayak-kukreja reviewed Nov 17, 2023

View reviewed changes

vinayak-kukreja approved these changes Nov 17, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 17, 2023

Merge branch 'main' into mrgrain/chore/awslint-go-vroom-vroom

6f2f40e

mergify bot merged commit 0607b2c into main Nov 17, 2023
9 checks passed

mergify bot deleted the mrgrain/chore/awslint-go-vroom-vroom branch November 17, 2023 18:36

github-actions bot mentioned this pull request Dec 1, 2023

Monthly PR metrics report - November 2023 #28212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(awslint): linting is slow #27860

fix(awslint): linting is slow #27860

mrgrain commented Nov 6, 2023 •

edited

Loading

This comment was marked as outdated.

mrgrain Nov 9, 2023

mrgrain Nov 9, 2023

mrgrain Nov 9, 2023

mrgrain Nov 9, 2023

mrgrain Nov 9, 2023

mrgrain Nov 9, 2023 •

edited

Loading

mrgrain Nov 9, 2023

mrgrain Nov 9, 2023

mrgrain Nov 9, 2023

mrgrain Nov 9, 2023

mrgrain Nov 9, 2023

mrgrain Nov 9, 2023

mrgrain Nov 9, 2023 •

edited

Loading

mrgrain Nov 17, 2023

vinayak-kukreja left a comment

vinayak-kukreja Nov 17, 2023

mrgrain Nov 17, 2023

mrgrain Nov 17, 2023

mrgrain Nov 17, 2023

mergify bot commented Nov 17, 2023

aws-cdk-automation commented Nov 17, 2023

mergify bot commented Nov 17, 2023

		@@ -0,0 +1,18 @@
		import * as changeCase from 'change-case';

		function cachedTransform(transform: (x: string) => string): (x: string) => string {

		.findAllConstructs(assembly)
		.filter(c => CoreTypes.isResourceClass(c.classType))

fix(awslint): linting is slow #27860

fix(awslint): linting is slow #27860

Conversation

mrgrain commented Nov 6, 2023 • edited Loading

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrgrain Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrgrain Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinayak-kukreja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Nov 17, 2023

aws-cdk-automation commented Nov 17, 2023

AWS CodeBuild CI Report

mergify bot commented Nov 17, 2023

mrgrain commented Nov 6, 2023 •

edited

Loading

mrgrain Nov 9, 2023 •

edited

Loading

mrgrain Nov 9, 2023 •

edited

Loading