fix(pacmak): re-introduce parallelism for Python and Go builds (#3124)

In #3045, the `OneByOneBuilder` was switched to build all packages
in series (instead of all in parallel), because there would
be race conditions in case of inter-package dependencies.

This is correct, but leaves a bunch of possible parallelism on the
table that is notably blowing up pack times for Python.

Re-introduce a (limited) form of parallelism by retaining the
sets of mutually independent packages, as toposorted, and
doing those in parallel.

Rename the class to `IndependentPackageBuilder` to more clearly
describe the intent behind the class.


---

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

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0

packages/jsii-pacmak/lib/builder.ts

@@ -5,7 +5,8 @@ import * as logging from './logging';
 import { JsiiModule } from './packaging';
 import { TargetConstructor, Target } from './target';
 import { TargetName } from './targets';
-import { Scratch } from './util';
+import { Toposorted } from './toposort';
+import { Scratch, flatten } from './util';
 
 export interface BuildOptions {
   /**
@@ -56,25 +57,38 @@ export interface TargetBuilder {
 }
 
 /**
- * Builds the targets for the given language sequentially
+ * Base implementation, building the package targets for the given language independently of each other
+ *
+ * Some languages can gain substantial speedup in preparing an "uber project" for all packages
+ * and compiling them all in one go (Those will be implementing a custom Builder).
+ *
+ * For languages where it doesn't matter--or where we haven't figured out how to
+ * do that yet--this class can serve as a base class: it will build each package
+ * independently, taking care to build them in the right order.
  */
-export class OneByOneBuilder implements TargetBuilder {
+export class IndependentPackageBuilder implements TargetBuilder {
   public constructor(
     private readonly targetName: TargetName,
     private readonly targetConstructor: TargetConstructor,
-    private readonly modules: readonly JsiiModule[],
+    private readonly modules: Toposorted<JsiiModule>,
     private readonly options: BuildOptions,
   ) {}
 
   public async buildModules(): Promise<void> {
-    for (const module of this.modules) {
-      if (this.options.codeOnly) {
-        // eslint-disable-next-line no-await-in-loop
-        await this.generateModuleCode(module, this.options);
-      } else {
-        // eslint-disable-next-line no-await-in-loop
-        await this.buildModule(module, this.options);
-      }
+    if (this.options.codeOnly) {
+      await Promise.all(
+        flatten(this.modules).map((module) =>
+          this.generateModuleCode(module, this.options),
+        ),
+      );
+      return;
+    }
+
+    for (const modules of this.modules) {
+      // eslint-disable-next-line no-await-in-loop
+      await Promise.all(
+        modules.map((module) => this.buildModule(module, this.options)),
+      );
     }
   }
 

packages/jsii-pacmak/lib/index.ts

@@ -6,6 +6,8 @@ import { findJsiiModules, updateAllNpmIgnores } from './npm-modules';
 import { JsiiModule } from './packaging';
 import { ALL_BUILDERS, TargetName } from './targets';
 import { Timers } from './timer';
+import { Toposorted } from './toposort';
+import { flatten } from './util';
 
 //#region Exported APIs
 
@@ -46,42 +48,51 @@ export async function pacmak({
     await rosetta.loadTabletFromFile(rosettaTablet);
   }
 
-  const modulesToPackage = await findJsiiModules(inputDirectories, recurse);
-  logging.info(`Found ${modulesToPackage.length} modules to package`);
-  if (modulesToPackage.length === 0) {
+  const modulesToPackageSorted = await findJsiiModules(
+    inputDirectories,
+    recurse,
+  );
+  const modulesToPackageFlat = flatten(modulesToPackageSorted);
+
+  logging.info(`Found ${modulesToPackageFlat.length} modules to package`);
+  if (modulesToPackageFlat.length === 0) {
     logging.warn('Nothing to do');
     return;
   }
 
   if (outputDirectory) {
-    for (const mod of modulesToPackage) {
+    for (const mod of modulesToPackageFlat) {
       mod.outputDirectory = outputDirectory;
     }
   } else if (updateNpmIgnoreFiles) {
     // if outdir is coming from package.json, verify it is excluded by .npmignore. if it is explicitly
     // defined via --out, don't perform this verification.
-    await updateAllNpmIgnores(modulesToPackage);
+    await updateAllNpmIgnores(modulesToPackageFlat);
   }
 
   await timers.recordAsync('npm pack', () => {
     logging.info('Packaging NPM bundles');
-    return Promise.all(modulesToPackage.map((m) => m.npmPack()));
+    return Promise.all(modulesToPackageFlat.map((m) => m.npmPack()));
   });
 
   await timers.recordAsync('load jsii', () => {
     logging.info('Loading jsii assemblies and translations');
     const system = new TypeSystem();
     return Promise.all(
-      modulesToPackage.map(async (m) => {
+      modulesToPackageFlat.map(async (m) => {
         await m.load(system, validateAssemblies);
         return rosetta.addAssembly(m.assembly.spec, m.moduleDirectory);
       }),
     );
   });
 
   try {
-    const targetSets = sliceTargets(modulesToPackage, targets, forceTarget);
-    if (targetSets.every((s) => s.modules.length === 0)) {
+    const targetSets = sliceTargets(
+      modulesToPackageSorted,
+      targets,
+      forceTarget,
+    );
+    if (targetSets.every((s) => s.modulesSorted.length === 0)) {
       throw new Error(
         `None of the requested packages had any targets to build for '${targets.join(
           ', ',
@@ -103,15 +114,19 @@ export async function pacmak({
           );
           return timers
             .recordAsync(targetSet.targetType, () =>
-              buildTargetsForLanguage(targetSet.targetType, targetSet.modules, {
-                argv,
-                clean,
-                codeOnly,
-                fingerprint,
-                force,
-                perLanguageDirectory,
-                rosetta,
-              }),
+              buildTargetsForLanguage(
+                targetSet.targetType,
+                targetSet.modulesSorted,
+                {
+                  argv,
+                  clean,
+                  codeOnly,
+                  fingerprint,
+                  force,
+                  perLanguageDirectory,
+                  rosetta,
+                },
+              ),
             )
             .then(
               () => logging.info(`${targetSet.targetType} finished`),
@@ -128,10 +143,10 @@ export async function pacmak({
     if (clean) {
       logging.debug('Cleaning up');
       await timers.recordAsync('cleanup', () =>
-        Promise.all(modulesToPackage.map((m) => m.cleanup())),
+        Promise.all(modulesToPackageFlat.map((m) => m.cleanup())),
       );
     } else {
-      logging.debug('Temporary directories retained (--no-clean)');
+      logging.info('Temporary directories retained (--no-clean)');
     }
   }
 
@@ -279,7 +294,7 @@ export interface PacmakOptions {
 
 async function buildTargetsForLanguage(
   targetLanguage: string,
-  modules: readonly JsiiModule[],
+  modules: Toposorted<JsiiModule>,
   {
     argv,
     clean,
@@ -324,21 +339,25 @@ async function buildTargetsForLanguage(
  */
 interface TargetSet {
   targetType: string;
-  modules: readonly JsiiModule[];
+
+  // Sorted into toposorted tranches
+  modulesSorted: Toposorted<JsiiModule>;
 }
 
 function sliceTargets(
-  modules: readonly JsiiModule[],
+  modulesSorted: Toposorted<JsiiModule>,
   requestedTargets: readonly TargetName[],
   force: boolean,
 ): readonly TargetSet[] {
   const ret = new Array<TargetSet>();
   for (const target of requestedTargets) {
     ret.push({
       targetType: target,
-      modules: modules.filter(
-        (m) => force || m.availableTargets.includes(target),
-      ),
+      modulesSorted: modulesSorted
+        .map((modules) =>
+          modules.filter((m) => force || m.availableTargets.includes(target)),
+        )
+        .filter((ms) => ms.length > 0),
     });
   }
   return ret;
@@ -374,10 +393,11 @@ function mapParallelOrSerial<T, R>(
 //#region Misc. Utilities
 
 function describePackages(target: TargetSet) {
-  if (target.modules.length > 0 && target.modules.length < 5) {
-    return target.modules.map((m) => m.name).join(', ');
+  const modules = flatten(target.modulesSorted);
+  if (modules.length > 0 && modules.length < 5) {
+    return modules.map((m) => m.name).join(', ');
   }
-  return `${target.modules.length} modules`;
+  return `${modules.length} modules`;
 }
 
 //#endregion

packages/jsii-pacmak/lib/npm-modules.ts

@@ -4,7 +4,7 @@ import * as path from 'path';
 
 import * as logging from '../lib/logging';
 import { JsiiModule } from './packaging';
-import { topologicalSort } from './toposort';
+import { topologicalSort, Toposorted } from './toposort';
 import { resolveDependencyDirectory } from './util';
 
 /**
@@ -17,7 +17,7 @@ import { resolveDependencyDirectory } from './util';
 export async function findJsiiModules(
   directories: readonly string[],
   recurse: boolean,
-): Promise<JsiiModule[]> {
+): Promise<Toposorted<JsiiModule>> {
   const ret: JsiiModule[] = [];
   const visited = new Set<string>();
 

packages/jsii-pacmak/lib/targets/index.ts

@@ -1,5 +1,11 @@
-import { OneByOneBuilder, TargetBuilder, BuildOptions } from '../builder';
+import {
+  IndependentPackageBuilder,
+  TargetBuilder,
+  BuildOptions,
+} from '../builder';
 import { JsiiModule } from '../packaging';
+import { Toposorted } from '../toposort';
+import { flatten } from '../util';
 import { DotnetBuilder } from './dotnet';
 import { Golang } from './go';
 import { JavaBuilder } from './java';
@@ -15,16 +21,18 @@ export enum TargetName {
 }
 
 export type BuilderFactory = (
-  modules: readonly JsiiModule[],
+  modules: Toposorted<JsiiModule>,
   options: BuildOptions,
 ) => TargetBuilder;
 
 export const ALL_BUILDERS: { [key in TargetName]: BuilderFactory } = {
-  dotnet: (ms, o) => new DotnetBuilder(ms, o),
-  go: (ms, o) => new OneByOneBuilder(TargetName.GO, Golang, ms, o),
-  java: (ms, o) => new JavaBuilder(ms, o),
-  js: (ms, o) => new OneByOneBuilder(TargetName.JAVASCRIPT, JavaScript, ms, o),
-  python: (ms, o) => new OneByOneBuilder(TargetName.PYTHON, Python, ms, o),
+  dotnet: (ms, o) => new DotnetBuilder(flatten(ms), o),
+  go: (ms, o) => new IndependentPackageBuilder(TargetName.GO, Golang, ms, o),
+  java: (ms, o) => new JavaBuilder(flatten(ms), o),
+  js: (ms, o) =>
+    new IndependentPackageBuilder(TargetName.JAVASCRIPT, JavaScript, ms, o),
+  python: (ms, o) =>
+    new IndependentPackageBuilder(TargetName.PYTHON, Python, ms, o),
 };
 
 export const INCOMPLETE_DISCLAIMER_COMPILING =

packages/jsii-pacmak/lib/toposort.ts

@@ -4,6 +4,8 @@ export type DepFunc<T> = (x: T) => string[];
 /**
  * Return a topological sort of all elements of xs, according to the given dependency functions
  *
+ * Returns tranches of packages that do not have a dependency on each other.
+ *
  * Dependencies outside the referenced set are ignored.
  *
  * Not a stable sort, but in order to keep the order as stable as possible, we'll sort by key
@@ -17,14 +19,14 @@ export function topologicalSort<T>(
   xs: Iterable<T>,
   keyFn: KeyFunc<T>,
   depFn: DepFunc<T>,
-): T[] {
+): Toposorted<T> {
   const remaining = new Map<string, TopoElement<T>>();
   for (const element of xs) {
     const key = keyFn(element);
     remaining.set(key, { key, element, dependencies: depFn(element) });
   }
 
-  const ret = new Array<T>();
+  const ret = new Array<T[]>();
   while (remaining.size > 0) {
     // All elements with no more deps in the set can be ordered
     const selectable = Array.from(remaining.values()).filter((e) =>
@@ -33,8 +35,9 @@ export function topologicalSort<T>(
 
     selectable.sort((a, b) => (a.key < b.key ? -1 : b.key < a.key ? 1 : 0));
 
+    ret.push(selectable.map((s) => s.element));
+
     for (const selected of selectable) {
-      ret.push(selected.element);
       remaining.delete(selected.key);
     }
 
@@ -56,3 +59,24 @@ interface TopoElement<T> {
   dependencies: string[];
   element: T;
 }
+
+/**
+ * For now, model a toposorted list as a list of tranches.
+ *
+ * Modeling it like this allows for SOME parallelism between nodes,
+ * although not maximum. For example, let's say we have A, B, C with
+ * C depends-on A, and we sort to:
+ *
+ *    [[A, B], [C]]
+ *
+ * Now, let's say A finishes quickly and B takes a long time: we still have
+ * to wait for B to finish before we could start C in this modeling.
+ *
+ * The better alternative would be to model a class that keeps the dependency
+ * graph and unlocks nodes as we go through them. That's a lot of effort
+ * for now, so we don't do that yet.
+ *
+ * We do declare the type `Toposorted<A>` here so that if we ever change
+ * the type, we can find all usage sites quickly.
+ */
+export type Toposorted<A> = readonly A[][];

packages/jsii-pacmak/lib/util.ts

@@ -302,3 +302,7 @@ export async function filterAsync<A>(
 export async function wait(ms: number): Promise<void> {
   return new Promise((ok) => setTimeout(ok, ms));
 }
+
+export function flatten<A>(xs: readonly A[][]): A[] {
+  return Array.prototype.concat.call([], ...xs);
+}

packages/jsii-pacmak/test/npm-modules.test.ts

@@ -3,6 +3,7 @@ import { tmpdir } from 'os';
 import { join } from 'path';
 
 import { findJsiiModules } from '../lib/npm-modules';
+import { flatten } from '../lib/util';
 
 describe(findJsiiModules, () => {
   let workDir = tmpdir();
@@ -82,7 +83,7 @@ describe(findJsiiModules, () => {
       ],
       false,
     );
-    expect(mods.map((m) => m.name)).toEqual([
+    expect(flatten(mods).map((m) => m.name)).toEqual([
       'packageB',
       'packageA',
       'packageC',
@@ -116,6 +117,6 @@ describe(findJsiiModules, () => {
       [join(workDir, 'packageA'), join(workDir, 'packageB')],
       false,
     );
-    expect(mods.map((m) => m.name)).toEqual(['packageA', 'packageB']);
+    expect(flatten(mods).map((m) => m.name)).toEqual(['packageA', 'packageB']);
   });
 });

packages/jsii-rosetta/test/record-references.test.ts

@@ -25,7 +25,7 @@ beforeAll(async () => {
   );
 });
 
-afterAll(async () => assembly.cleanup());
+afterAll(() => assembly.cleanup());
 
 test('detect class instantiations', () => {
   const translator = assembly.successfullyCompile(`