TsProject build performance: persistent workers are never reused across rules

[ulfjack]

🐞 bug report

Affected Rule

The issue is caused by the rule: ts_project

Is this a regression?

Unknown.

Description

When enabling persistent workers for ts_project, the persistent workers are never reused across different ts_project rules in the same repository because each such rule has a different node modules manifest. The --bazel_node_modules_manifest flag is added by the run_node function, which uses a local args element that doesn't get moved to the parameter file.

run_node: https://github.com/bazelbuild/rules_nodejs/blob/stable/internal/providers/node_runtime_deps_info.bzl#L98

ts_project: https://github.com/bazelbuild/rules_nodejs/blob/stable/packages/typescript/internal/ts_project.bzl#L109

This might be intentional if it is impossible to reuse a worker if the node modules manifest differs (note that it always differs by name regardless of content), but severely reduces the usefulness of persistent workers within a single build, as multiple ts_project rules can't reuse the worker given the current rule design.

🔬 Minimal Reproduction

Worker restarts have no user-visible effect except slower builds, but it should reproduce in any project that has at least two ts_project rules.

🔥 Exception or Error

None.

🌍 Your Environment

Rules_nodejs version: I confirmed this from the source code at HEAD.

Anything else relevant?

[ulfjack]

The documentation says:

In theory, Persistent Workers (via the supports_workers attribute) remedies the slow compilation time, however it adds additional complexity because the worker process can only see one set of dependencies, and so it cannot be shared between different ts_project rules. That attribute is documented as experimental, and may never graduate to a better support contract.

[ulfjack]

FWIW, this is on a project with several hundred ts_project rules.

[alexeagle]

Yeah you found the right docs, and that is indeed a limitation which we don't have a plan to fix.

The fundamental problem is we use a "linker" program to dynamic-link the libraries users bring to the action at runtime (those which aren't dependencies of the typescript compiler, tsc which can be statically linked). In order for a persistent worker process to accept WorkRequest's from multiple ts_project rules, we would need a way to unlink the dependencies from previous run and link new ones, but right now the linker is designed to just run once before the Node process evaluates the entry point.

We've discussed possibly writing a new linker to make this scenario possible, but Persistent Workers will still require a custom compiler binary, and have subtle hermeticity bugs when state leaks from one WorkRequest to the next. My hope is that the new transpiler attribute obviates the need for them in practice, if type-checking can always be treated like a test that doesn't block the developer inner loop where speed is critical.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had any activity for 6 months. It will be closed if no further activity occurs in 30 days. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

[lfpino]

Hey @alexeagle, just curious, are there any new developments on this since we last talked about it 6 months ago? Thanks

[alexeagle]

Yes, this is fixed in aspect_rules_ts which uses a linker which runs as a standard Bazel action rather than as a bootstrap in the nodejs process.

[lfpino]

Thanks @alexeagle is this issue fixed then? Sorry I don't understand all the technical details to assess.

[gregmagolan]

Yes. We have workers on by default on aspect_rules_ts: https://github.com/aspect-build/rules_ts/releases/tag/v1.0.0-rc1. This is a port of @bazel/typescript to be based on top of rules_js: https://github.com/aspect-build/rules_js.

There were technical limitations in rules_nodejs with the dynamic linker which made it impossible to create persistent workers that spans multiple targets. The best that we could do in rules_nodejs was a worker per target, which doesn't scale.

rules_js takes a completely different approach to linking which made it possible to make ts_project workers scalable.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had any activity for 6 months. It will be closed if no further activity occurs in 30 days. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!