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

Refactor build -- 13x faster typescript bundling #1558

Merged
merged 13 commits into from
Apr 14, 2021

Conversation

jhsware
Copy link
Contributor

@jhsware jhsware commented Apr 13, 2021

UPDATE: This PR eventually made TS bundling 13x faster.

I refactored the build system for speed and readability. There was some over-engineering going on in the scripts which made them more daunting than necessary.

Build time went from ~9mins to ~4.5mins with reduced load. Will probably be faster with more cores.

  • now using lerna exec to invoke builds for each package in parallel
  • the different outputs for each package are spawned by node allowing more efficient use of JS Engine and memory
  • removed invocation of tsc to generate types because rollup will provide these anyway
  • runs inferno-shared first because it is required by other scripts, everything else appears to run fine in parallel
  • updated the quick-test-scripts

I did a diff on generated output in [package-name]/dist and there is no difference.

My hope is that this will make it easier if one wants to improve the build system further.

@Havunen
Copy link
Member

Havunen commented Apr 13, 2021

Very nice, do you know where the bottleneck lies after this change? I wonder if it's doing typescript compilation again for each package and each output?

@Havunen
Copy link
Member

Havunen commented Apr 13, 2021

This is slightly related also to this ticket: #1487

return true
})

await targets.forEach(async (options) => {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the alias plugin is needed anymore? Also would it be possible to build all packages using rollup inputs rather than spawning new process for each package using lerna?

Copy link
Contributor Author

@jhsware jhsware Apr 13, 2021

Choose a reason for hiding this comment

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

Interesting! I didn't investigate the alias plugin due to lack of time. I ran into an issue that I now realise might have been related to that plugin but it solved itself. If we can remove it we should because it makes the build process harder to understand.

I am not sure that I understand what you mean by "using rollup inputs". I chose to let lerna exec take care of parallel execution because I wanted to simplify everything to allow a lower barrier of entry for further optimisation. If esbuild solves const enums I believe the esm and esmnext parts of the transforms could be done using esbuild without making the build process more complex, that would cut build time to 2.5mins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alias.js appears to be important to the build workflow. It allows packages to use transpiled output during bundling.

@jhsware
Copy link
Contributor Author

jhsware commented Apr 13, 2021

Very nice, do you know where the bottleneck lies after this change? I wonder if it's doing typescript compilation again for each package and each output?

Yes, I do believe it does the TS-compilation multiple times. And worse, it generates type files for all the packages for every package so that is for sure room for improvement. In the esbuild branch I just generated the type defs once for the entire repos. Perhaps one could to something similar for the esnext-transpile, a single transpile but split to multiple outputs, one for each package, and then use those for the rest of the transpilations.

Another way could be to use the alias and choose more carefully the order of transpilation to avoid TS for imports, starting with packages without TS deps.

There was a discussion here microsoft/TypeScript#17611 but I am not sure if it really results in an optimisation. Looked like it was more about convenience, but I only browsed the discussion.

@jhsware
Copy link
Contributor Author

jhsware commented Apr 13, 2021

Actually TS project references looks promising according to this https://devblogs.microsoft.com/typescript/announcing-typescript-3-0/#project-references

@Havunen
Copy link
Member

Havunen commented Apr 14, 2021

I believe its this line from the repository root config which makes it to build all packages always:
https://github.com/infernojs/inferno/blob/master/tsconfig.json#L91-L93

We could try overriding this value when building

@jhsware
Copy link
Contributor Author

jhsware commented Apr 14, 2021

Awesome! I'll check it out and see what happens.

@jhsware
Copy link
Contributor Author

jhsware commented Apr 14, 2021

@Havunen So I have got the build time down to 43secs by removing typescript transpilation from bundling. This is a diff of generated output.

  • The esnext files actually look more correct than with the previous build.
  • The type defs appear to only have swapped order of alternative types.
  • Some other files only have changes in whitespace

jhsware/inferno-build-dist@5794f97

@Havunen
Copy link
Member

Havunen commented Apr 14, 2021

Amazing work! much appreciated!

@jhsware
Copy link
Contributor Author

jhsware commented Apr 14, 2021

I removed the quick-test scripts. They are broken and for them to be useful they should be made even faster.

@jhsware jhsware changed the title Refactor build Refactor build -- 13x faster builds :) Apr 14, 2021
@jhsware jhsware changed the title Refactor build -- 13x faster builds :) Refactor build -- 13x faster typescript bundling Apr 14, 2021
@Havunen
Copy link
Member

Havunen commented Apr 14, 2021

Can you pull the latest from the master branch before merging this?
Is there anything left here still? Would be nice to merge this

@jhsware
Copy link
Contributor Author

jhsware commented Apr 14, 2021

Can you pull the latest from the master branch before merging this?
Is there anything left here still? Would be nice to merge this

FIXED with a rebase and force-push.

I don't think there is anything left to be done at this moment. The tests are passing and docs can be built.

Merge away and let me know if something blows up.

@Havunen Havunen merged commit 550550a into infernojs:master Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants