Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addressing scaling and performance issues with Typescript #107648

Closed
tylersmalley opened this issue Aug 4, 2021 · 7 comments
Closed

Addressing scaling and performance issues with Typescript #107648

tylersmalley opened this issue Aug 4, 2021 · 7 comments
Labels
Team:Operations Team label for Operations Team

Comments

@tylersmalley
Copy link
Contributor

tylersmalley commented Aug 4, 2021

Where we are

While Typescript has been very useful, it does come at a cost. In an attempt to minimize this to something reasonable, we have migrated to project references. This has drastically helped when working in an isolated package or plugin, but all projects must be built on bootstrap, which is lengthy. Currently, this can take more than 10 minutes uncached. Since this is mostly used for IDE support, some developers opt to set BUILD_TS_REFS_DISABLE=true in order to disable the building of projects references during bootstrap.

With migrating to Bazel for our build tool, we were planning to build all packages and plugins with tsc. This would allow us to include types with the code cached within Bazel, allowing us to get rid of project references. Now with all packages migrated to Bazel, we see an issue with tsc of performance and scope.

The performance of tsc is pretty awful, and our tests have shown it to be about 30x slower than Babel. It's not clear what we could do to improve this. There is an open issue to support multi-threading, which could help. We do know that some plugins are worse than others. The Core plugin, for example, takes about 40 seconds to build.

The scope of using tsc has also shown to be a problem. When building with tsc all dependencies need to be available, wherewith Babel, only the code is transpiled. This is particularly an issue when there are packages or plugins which are high in the dependency tree. It's quite easy to need to rebuild ALL of Kibana when changing something like src/core or @kbn/dev-utils.

We have been working to come up with alternatives before proceeding in migrating plugins to Bazel and feel like we have a solution that moves the cost of Typescript.

Proposal

I have a feeling this is going to be pretty controversial, but stick with me. All packages and plugins will have their public Typescript definitions committed for public, server, and common.

This alone should prevent the need to have the bootstrap task to build the project references. The IDE will no longer need to build anything outside of the current package or plugin being edited. This will only be possible with plugins during their migration to Bazel, as all imports across plugin boundaries will use NPM modules (example @kbn-plugin/core) instead of relative imports.

A new command or script will be created to build public types. It will use Bazel to build the types, only building what is necessary based on the changes. APIExtractor will be used to generate a single d.ts file containing only the public types defined in the entry file. These definitions will then be copied to the repository to be committed.

Moving the Typescript cost to only when changing a public interface minimizes how often a developer will be to pay it. We can reduce this further by having CI automatically commit these changes if any exist. We're not concerned with blocking CI so much as having the public type changes present on the PR. Having these as part of the PR has the benefit of showing what public interfaces have changed, which I know is something we have wanted.

@tylersmalley tylersmalley added the Team:Operations Team label for Operations Team label Aug 4, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@tylersmalley
Copy link
Contributor Author

cc @kobelb @stacey-gammon @mshustov

@spalger
Copy link
Contributor

spalger commented Aug 4, 2021

Had a good long chat with Tiago and Tyler about this and my takeaways are:

  1. Build @types/kbn__* packages into the node_modules by only depending on NPM deps, the source files of a specific package, and other @types/kbn__* packages.

    In this model making a change to a file would run two processes in parallel:

    • babel would and produce viable JS module based on the changes as quickly as possible, which would allow tools like the optimizer, jest, node, etc. to observe the changes immediately.
    • tsc would produce the @types/kbn__* package (eventually summarized to only include the public interface in the output) and this process could take as long as needed but only be necessary to consume the updated public interfaces in code outside of the package
  2. Work with iBazel to support/implement aborting an in progress build.

    With this change in place we will be able to abort the slow @types/kbn__* build when another file change is detected and start new babel&tsc builds immediately. This will make the JS packages available as quickly as possible (again, important to see the changes in the browser, jest, node, etc.) and eventually will result in up-to-date @types/kbn__* packages (just not as quickly, but eventually and without any intervention necessary).

With these changes in place we don't need to commit the build output to the filesystem, we just need to generate @types/kbn__* packages from the source and cache them in the remote Bazel cache, though it would still be ideal if the @types/kbn__* packages were as minimal as possible to reduce cascading rebuilds unless the public interface changed. Thankfully, since in this model the @types/kbn__* packages are not necessary for consuming the code in any way other than IDEs we can work on using api-extractor as a secondary phase (ideal because this project still has a lot of unknowns).

@mshustov
Copy link
Contributor

mshustov commented Aug 5, 2021

IDEs we can work on using api-extractor as a secondary phase (ideal because this project still has a lot of unknowns).

A few things I mentioned on yesterday call about api-extractor being a source of problems:

@spalger
Copy link
Contributor

spalger commented Aug 5, 2021

compiler emits inferred types inconsistently

😬

@tylersmalley tylersmalley added 1 and removed 1 labels Oct 11, 2021
@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Oct 12, 2021
@tylersmalley tylersmalley removed loe:small Small Level of Effort impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. EnableJiraSync labels Mar 16, 2022
@tylersmalley
Copy link
Contributor Author

Closing this out, since it has served it's purpose. We're working on creating a project spec for the next phase, which includes the findings here.

@matschaffer
Copy link
Contributor

@tylersmalley I'd love to hear more as it shapes up. Let me know once there are new issues to follow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Operations Team label for Operations Team
Projects
None yet
Development

No branches or pull requests

5 participants