[codegen/node] Implement support for lazy-loaded Node modules #10538

t0yv0 · 2022-08-30T23:09:28Z

Description

This PR implements opt-in support for generating lazy load code for Node modules. The changes should be entirely non-breaking for consumers of the providers.

#7653 for more history and motivation

In addition to PR checks, this has been tested manually on azure-native. It compiles and delivers speedups in node startup time: from 7.6s to 1.6s when importing "@pulumi/azure-native" and from 800ms to 450ms when importing "@pulumi/azure-native/resources" and "@pulumi/azure-native/storage".

How this works:

modules that define functions and resources are loaded lazily
generated code may use import type .. form when importing enums ("useTypeOnlyReferences" opt-in flag in schema)

pulumi.runtime.registerResourceModule side-effects are not affected and happen as before.

Our prior approach on 7653 went for lazily importing module-re-exports; this would still be preferable but we run into some corner cases where we can't seem to properly make TypeScript re-export the module as a "namespace", leading to a class of programs that can break. This is explored in some detail in pulumi/pulumi-azure-native#1944

Further work: we should be able to do a lot better here with subsequent work by making sure we emit more precise imports, this will optimize not only node time but also TS compilation time - this is tracked in #10442 - but recommend adopting this for now.

Fixes #7653

Checklist

I have added tests that prove my fix is effective or that my feature works

I have updated the CHANGELOG-PENDING file with my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

pulumi-bot · 2022-08-30T23:09:42Z

Please view the results of the Downstream Codegen Tests Here

t0yv0 · 2022-08-30T23:12:12Z

pkg/codegen/testing/test/testdata/azure-native-nested-types/nodejs/documentdb/index.ts

@@ -5,10 +5,12 @@ import * as pulumi from "@pulumi/pulumi";
 import * as utilities from "../utilities";

 // Export members:
-export * from "./sqlResourceSqlContainer";


So this is a typical change example. Instead of reexporting * which generates require() at runtime we reexport equivalent definitions below.

t0yv0 · 2022-08-30T23:12:28Z

pkg/codegen/testing/test/testdata/azure-native-nested-types/nodejs/documentdb/index.ts


-// Import resources to register:
-import { SqlResourceSqlContainer } from "./sqlResourceSqlContainer";
+export { SqlResourceSqlContainerArgs } from "./sqlResourceSqlContainer";


re-export all interfaces, type-only, no require()

t0yv0 · 2022-08-30T23:12:56Z

pkg/codegen/testing/test/testdata/azure-native-nested-types/nodejs/documentdb/index.ts

-// Import resources to register:
-import { SqlResourceSqlContainer } from "./sqlResourceSqlContainer";
+export { SqlResourceSqlContainerArgs } from "./sqlResourceSqlContainer";
+export type SqlResourceSqlContainer = import("./sqlResourceSqlContainer").SqlResourceSqlContainer;


re-export the type of the resource class; this is needed if it's used for declaring types and type-checking

t0yv0 · 2022-08-30T23:13:27Z

pkg/codegen/testing/test/testdata/azure-native-nested-types/nodejs/documentdb/index.ts

-import { SqlResourceSqlContainer } from "./sqlResourceSqlContainer";
+export { SqlResourceSqlContainerArgs } from "./sqlResourceSqlContainer";
+export type SqlResourceSqlContainer = import("./sqlResourceSqlContainer").SqlResourceSqlContainer;
+export const SqlResourceSqlContainer: typeof import("./sqlResourceSqlContainer").SqlResourceSqlContainer = null as any


mock-reexport the value of the resource class, this is needed to type-check expressions such as new SqlResourceSqlContainer(); still this generates no require()

t0yv0 · 2022-08-30T23:14:04Z

pkg/codegen/testing/test/testdata/azure-native-nested-types/nodejs/documentdb/index.ts

+export { SqlResourceSqlContainerArgs } from "./sqlResourceSqlContainer";
+export type SqlResourceSqlContainer = import("./sqlResourceSqlContainer").SqlResourceSqlContainer;
+export const SqlResourceSqlContainer: typeof import("./sqlResourceSqlContainer").SqlResourceSqlContainer = null as any
+utilities.lazy_load_property(exports, "./sqlResourceSqlContainer", "SqlResourceSqlContainer");


patch up the export at runtime (no impact on types) to call require lazily

pulumi-bot · 2022-08-31T16:15:12Z

Please view the results of the Downstream Codegen Tests Here

pulumi-bot · 2022-08-31T16:51:21Z

Please view the results of the Downstream Codegen Tests Here

t0yv0 · 2022-08-31T17:20:37Z

I've found and fixed some issues by trying it on pulumi-azure-native (not caught by codegen tests? need to look into this).

This helps a bit! Test: pulumi new azure-javascript (for some reason this returns the "wrong" style import).

After

Benchmark 1: node index.js
  Time (mean ± σ):      4.476 s ±  0.191 s    [User: 3.973 s, System: 0.959 s]
  Range (min … max):    4.257 s …  4.859 s    10 runs

Before

Benchmark 1: node index.js
  Time (mean ± σ):      7.944 s ±  0.399 s    [User: 7.208 s, System: 1.670 s]
  Range (min … max):    7.304 s …  8.521 s    10 runs

t0yv0 · 2022-08-31T17:26:09Z

Tracing which modules get loaded, it's the function modules mostly. This is very promising. IF we lazy-load them also we get a real win here.

NODE_DEBUG=module node index.js 2>module-load.log
cat module-load.log | grep "load[ ]" | grep -v REQUEST | grep azure-native/sdk/nodejs/bin | grep -v get  | grep -v list | grep -v index

t0yv0 · 2022-08-31T17:27:27Z

I'm starting to think this approach has an advantage over lazy top-level submodules only - this optimizes imports of @pulumi/azure-native/resources as well as top-level @pulumi/azure-native

t0yv0 · 2022-08-31T17:27:44Z

CC @RobbieMcKinstry

pkg/codegen/testing/test/testdata/azure-native-nested-types/nodejs/utilities.ts

pkg/codegen/testing/test/testdata/dash-named-schema/nodejs/utilities.ts

pkg/codegen/testing/test/testdata/dashed-import-schema/nodejs/index.ts

pulumi-bot · 2022-08-31T18:41:23Z

Please view the results of the Downstream Codegen Tests Here

pulumi-bot · 2022-08-31T18:45:06Z

Please view the results of the Downstream Codegen Tests Here

pulumi-bot · 2022-08-31T18:51:57Z

Please view the results of the Downstream Codegen Tests Here

pulumi-bot · 2022-08-31T19:11:48Z

Please view the results of the Downstream Codegen Tests Here

t0yv0 · 2022-08-31T19:13:44Z

Reminder to self: this PR still needs to address the fate of resource registrations. AFAIK these must be eagerly registered when importing any of the modules as before. The lazy loading may defer the registrations too late.

t0yv0 · 2022-08-31T19:23:25Z

The lazy-load of functions had impact! Now on azure-native:

Benchmark 1: node index.js
  Time (mean ± σ):      1.652 s ±  0.058 s    [User: 1.697 s, System: 0.233 s]
  Range (min … max):    1.593 s …  1.773 s    10 runs
 
  Warning: Ignoring non-zero exit code.

t0yv0 · 2022-08-31T19:30:04Z

Remaining modules automatically loaded by require("@pulumi/azure-native"):

  2844 modules loaded total
    1336 enum-related modules
    1508 non-enum related modules
      1501 index.js modules

t0yv0 · 2022-08-31T19:59:43Z

Now measure the example rewritten for recommended imports "azure-native/storage" and "azure-native/resources".

BEFORE:

Benchmark 1: node index.js
  Time (mean ± σ):     870.1 ms ±  20.2 ms    [User: 879.4 ms, System: 133.5 ms]
  Range (min … max):   850.4 ms … 917.1 ms    10 runs

AFTER:

Benchmark 1: node index.js
  Time (mean ± σ):     871.9 ms ±  39.1 ms    [User: 893.2 ms, System: 126.5 ms]
  Range (min … max):   846.0 ms … 975.6 ms    10 runs

It's a wash because it manages to load all enums from all modules, which dominates. This needs addressing somehow to be a real benefit to users. Tinkering further.

pulumi-bot · 2022-08-31T21:33:39Z

Please view the results of the Downstream Codegen Tests Here

pulumi-bot · 2022-08-31T21:37:25Z

Please view the results of the Downstream Codegen Tests Here

pkg/codegen/nodejs/gen.go

t0yv0 · 2022-08-31T21:40:33Z

Ok good, with the latest tweak to avoid importing types/index.js that gets us all enum modules imported, we reclaim the last milliseconds.

Code:

"use strict";

const pulumi = require("@pulumi/pulumi");
const resources = require("@pulumi/azure-native/resources");
const storage = require("@pulumi/azure-native/storage");

After:

Benchmark 1: node index.js
  Time (mean ± σ):     436.7 ms ±  34.8 ms    [User: 450.0 ms, System: 51.9 ms]
  Range (min … max):   394.0 ms … 496.1 ms    10 runs

  Warning: Ignoring non-zero exit code.

Before:

Benchmark 1: node index.js
  Time (mean ± σ):     818.4 ms ±  21.5 ms    [User: 823.3 ms, System: 124.5 ms]
  Range (min … max):   798.2 ms … 870.2 ms    10 runs

  Warning: Ignoring non-zero exit code.

Code:

"use strict";

const pulumi = require("@pulumi/pulumi");
const az = require("@pulumi/azure-native");
const resources = az.resources;
const storage = az.storage;

After:

Benchmark 1: node index.js
  Time (mean ± σ):      1.625 s ±  0.028 s    [User: 1.669 s, System: 0.231 s]
  Range (min … max):    1.596 s …  1.698 s    10 runs

Before:

Benchmark 1: node index.js
  Time (mean ± σ):      8.391 s ±  0.427 s    [User: 7.620 s, System: 1.759 s]
  Range (min … max):    7.743 s …  9.120 s    10 runs

This is clearly better and we only import 50 modules from azure-native now when using recommended imports (mostly the index.js ones), out of 650 total modules imported. And when not using recommended imports it's not ideal but still a lot better. I think this is good enough.

t0yv0 · 2022-08-31T21:53:00Z

Reminder to self: this PR still needs to address the fate of resource registrations. AFAIK these must be eagerly registered when importing any of the modules as before. The lazy loading may defer the registrations too late.

Ah nevermind! We are lucky here. The calls to registerResourceModule are all sitting in index.ts modules files which are not affected by this change. So they will happen exactly as before. As to enum files, they never call registerResourceModule. So this concern is irrelevant for the PR.

pulumi-bot · 2022-08-31T22:31:19Z

Please view the results of the Downstream Codegen Tests Here

pulumi-bot · 2022-08-31T22:38:25Z

Please view the results of the Downstream Codegen Tests Here

pkg/codegen/nodejs/importer.go

pulumi-bot · 2022-09-01T15:24:21Z

Please view the results of the Downstream Codegen Tests Here

pulumi-bot · 2022-09-01T17:19:51Z

Please view the results of the Downstream Codegen Tests Here

pulumi-bot · 2022-09-01T19:10:13Z

Please view the results of the Downstream Codegen Tests Here

pulumi-bot · 2022-09-01T19:13:29Z

Please view the results of the Downstream Codegen Tests Here

pulumi-bot · 2022-09-01T19:25:33Z

Please view the results of the Downstream Codegen Tests Here

RobbieMcKinstry

This looks great! Well done! :D

RobbieMcKinstry · 2022-09-01T19:36:13Z

pkg/codegen/testing/test/testdata/external-enum/nodejs/utilities.ts

+                let m = cache.module || (cache.module = loadModule());
+                return m[property];


I'm not sure this change is any better than the original syntax (though it doesn't hurt either). NodeJS caches calls to require(), so "reloading" the module is free.

pulumi-bot · 2022-09-01T19:44:30Z

Please view the results of the Downstream Codegen Tests Here

t0yv0 · 2022-09-01T22:42:39Z

Downstream checks actually passed here: #10576 after a fixup to the GHA definition.

t0yv0 commented Aug 30, 2022

View reviewed changes

RobbieMcKinstry reviewed Aug 31, 2022

View reviewed changes

pkg/codegen/testing/test/testdata/azure-native-nested-types/nodejs/utilities.ts Outdated Show resolved Hide resolved

RobbieMcKinstry reviewed Aug 31, 2022

View reviewed changes

pkg/codegen/testing/test/testdata/dash-named-schema/nodejs/utilities.ts Outdated Show resolved Hide resolved

pkg/codegen/testing/test/testdata/dashed-import-schema/nodejs/index.ts Outdated Show resolved Hide resolved

t0yv0 commented Aug 31, 2022

View reviewed changes

pkg/codegen/nodejs/gen.go Outdated Show resolved Hide resolved

t0yv0 commented Aug 31, 2022

View reviewed changes

pkg/codegen/nodejs/importer.go Outdated Show resolved Hide resolved

t0yv0 added 3 commits September 1, 2022 11:23

Accept codegen tests

db8d80e

Illustrate on azure-native-nested-types

edb4bff

Test lazy-loaded functions on output-funcs

d57a597

t0yv0 force-pushed the t0yv0/lazy-loading branch from 836cda5 to d57a597 Compare September 1, 2022 15:24

t0yv0 added 5 commits September 1, 2022 12:14

Start factoring out

6726f3d

More cases

9849cf4

More unit tests

583b3b9

Reduce flag surface

45400c6

Separate fileInfo structs into its own module

a4d31de

t0yv0 added 2 commits September 1, 2022 15:08

Merge lazyloads

0f9bcf1

Accept codegen changes

626480d

CHANGELOG

1106e5a

t0yv0 requested a review from RobbieMcKinstry September 1, 2022 19:17

Lint

edae2af

RobbieMcKinstry approved these changes Sep 1, 2022

View reviewed changes

t0yv0 added 2 commits September 1, 2022 15:43

Revert caching require()

2f4eaaa

Accept codegen

a04841a

t0yv0 merged commit 9597386 into master Sep 1, 2022

pulumi-bot deleted the t0yv0/lazy-loading branch September 1, 2022 22:42

This was referenced Sep 1, 2022

Lazily export nodejs sub-modules #7653

Closed

[codegen/nodejs] Implement Lazy Loading #10517

Closed

[nodejs/codegen] Lazy-load leaf enum modules #10607

Open

joeduffy mentioned this pull request May 29, 2023

Directly importing a type leads to exception pulumi/pulumi-aws#772

Closed

		let m = cache.module \|\| (cache.module = loadModule());
		return m[property];

[codegen/node] Implement support for lazy-loaded Node modules #10538

[codegen/node] Implement support for lazy-loaded Node modules #10538

Conversation

t0yv0 commented Aug 30, 2022 • edited

Description

Checklist

pulumi-bot commented Aug 30, 2022

t0yv0 Aug 30, 2022

Choose a reason for hiding this comment

t0yv0 Aug 30, 2022

Choose a reason for hiding this comment

t0yv0 Aug 30, 2022

Choose a reason for hiding this comment

t0yv0 Aug 30, 2022

Choose a reason for hiding this comment

t0yv0 Aug 30, 2022

Choose a reason for hiding this comment

pulumi-bot commented Aug 31, 2022

pulumi-bot commented Aug 31, 2022

t0yv0 commented Aug 31, 2022 • edited

After

Before

t0yv0 commented Aug 31, 2022

t0yv0 commented Aug 31, 2022

t0yv0 commented Aug 31, 2022

pulumi-bot commented Aug 31, 2022

pulumi-bot commented Aug 31, 2022

pulumi-bot commented Aug 31, 2022

pulumi-bot commented Aug 31, 2022

t0yv0 commented Aug 31, 2022

t0yv0 commented Aug 31, 2022

t0yv0 commented Aug 31, 2022

t0yv0 commented Aug 31, 2022

pulumi-bot commented Aug 31, 2022

pulumi-bot commented Aug 31, 2022

t0yv0 commented Aug 31, 2022 • edited

t0yv0 commented Aug 31, 2022

pulumi-bot commented Aug 31, 2022

pulumi-bot commented Aug 31, 2022

pulumi-bot commented Sep 1, 2022

pulumi-bot commented Sep 1, 2022

pulumi-bot commented Sep 1, 2022

pulumi-bot commented Sep 1, 2022

pulumi-bot commented Sep 1, 2022

RobbieMcKinstry left a comment

Choose a reason for hiding this comment

RobbieMcKinstry Sep 1, 2022

Choose a reason for hiding this comment

pulumi-bot commented Sep 1, 2022

t0yv0 commented Sep 1, 2022

t0yv0 commented Aug 30, 2022 •

edited

t0yv0 commented Aug 31, 2022 •

edited

t0yv0 commented Aug 31, 2022 •

edited