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

Remove ts_library_builder, maintain lib.deno_runtime.d.ts by hand #2827

Merged
merged 5 commits into from Aug 30, 2019

Conversation

@ry
Copy link
Collaborator

commented Aug 28, 2019

I want to temporarily rip out ts_library_builder - I'm basically ripping out all of our automated build processes towards the goal of getting "cargo build" to be our main build system. First was flatbuffers, now I'm doing snapshots - which is outlined in the https://github.com/ry/deno_typescript repo. This will involve removing rollup and changing our import syntax in //js to use file extensions. This breaks ts_library_builder... so want to just check in lib.deno_runtime.d.ts for now until the dust settles. Once build.py is gone, I will try to hook up the build to be more automated again. Eventually I want to rewrite ts_library_builder to take as input the d.ts file produced by outFile, rather than the whole tree.

Towards #2496, #2608, #2711

@ry

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 28, 2019

This patch also removes js/assets.ts, which needed to be removed anyway to get off rollup. Assets are now fetched via an op.... this may introduce a performance regression for compilation speed.

@ry ry force-pushed the ry:rm_ts_library_builder branch from 0e72004 to e4f0b80 Aug 28, 2019

@kitsonk
Copy link
Contributor

left a comment

LGTM

Worry a little bit about moving the assets to Rust wholly as TypeScript tends to request these a lot. I think the way we cache module resolution in the compiler would make that efficient, but I suspect introspecting the debug log for something will show if there is any redunant ops to fetch stuff repeatedly that could be cached.

@ry

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2019

@kitsonk I've done some benchmarking. We will see about a 1.15x slow down on cold rebuilds due to fetching the assets. On the other hand, the executable becomes 2mb smaller.

This is simpler and the perf regression isn't too bad.

I am trying to think about how to do "!string" style assets without rollup, but I think that can wait.

"lib.esnext.asynciterable.d.ts" => inc!("lib.esnext.asynciterable.d.ts"),
"lib.esnext.bigint.d.ts" => inc!("lib.esnext.bigint.d.ts"),
"lib.esnext.intl.d.ts" => inc!("lib.esnext.intl.d.ts"),
"lib.esnext.symbol.d.ts" => inc!("lib.esnext.symbol.d.ts"),

This comment has been minimized.

Copy link
@ry

ry Aug 29, 2019

Author Collaborator

I'm sure someone well-versed in rust macros could dry this up further. But I don't particularly care about elegance right now as I'm trudging through this build system refactor.

@ry ry requested a review from piscisaureus Aug 29, 2019

@ry ry changed the title WIP Remove ts_library_builder, maintain lib.deno_runtime.d.ts by hand Remove ts_library_builder, maintain lib.deno_runtime.d.ts by hand Aug 29, 2019

@ry ry referenced this pull request Aug 29, 2019
@piscisaureus

This comment has been minimized.

Copy link
Collaborator

commented Aug 29, 2019

So you don't want to keep ts_library_builder in the repo so it can be run manually?
I'm not changing too much .ts files but it seems like a pain to manually update the .d.ts file; might be easier to just run some script instead, even if it's not part of the build process.

@ry

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

@piscisaureus due to the file extension change it won’t be runnable

ry added 2 commits Aug 30, 2019

@ry ry merged commit c370f74 into denoland:master Aug 30, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@ry

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

The main performance impact of this patch was to the memory usage of cold run typescript programs. This is due to the removal of assets from the bundle - they are now fetched from Rust - the memory gets duplicated.

Screen Shot 2019-08-30 at 2 26 59 PM

Otherwise this massive change looks good software. It seems tests fail if lib.deno_runtime.d.ts gets out of sync with the code, so far.

@ry ry deleted the ry:rm_ts_library_builder branch Aug 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.