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

Fast exceptions no sources #1097

Merged
merged 2 commits into from
Nov 5, 2018
Merged

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Oct 26, 2018

This PR builds on top of @ry branch to try to fix #1087.

The .map for the bundle goes from about 9.3Mb to 1.7Mb on my machine. The only problem I have notices is that stack traces that go back to typescript.js don't have the line numbers anymore and are reported as <anonymous>. I can live with that personally.

$ ./out/debug/deno tests/error_004_missing_module.ts
Compiling /Users/kkelly/github/deno/tests/error_004_missing_module.ts
NotFound: Cannot resolve module "bad-module.ts" from "/Users/kkelly/github/deno/tests/error_004_missing_module.ts"
    at maybeError (deno/js/errors.ts:38:12)
    at maybeThrowError (deno/js/errors.ts:26:15)
    at sendSync (deno/js/dispatch.ts:70:5)
    at Object.codeFetch (deno/js/os.ts:40:19)
    at DenoCompiler.resolveModule (deno/js/compiler.ts:518:38)
    at moduleNames.map.name (deno/js/compiler.ts:666:31)
    at Array.map (<anonymous>)
    at DenoCompiler.resolveModuleNames (deno/js/compiler.ts:658:24)
    at Object.compilerHost.resolveModuleNames (<anonymous>)
    at resolveModuleNamesWorker (<anonymous>)

Requires denoland/deno_third_party#17.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Nice work! I think this about solves the problem completely.

This still increases incremental build time (40s -> 52s) and snapshot_deno.bin (39M -> 48M) but these are much much more reasonable than what I had.

It seems your rollup patch includes other updates. I'd like to be able to cleanly see your patch in the git log - so can you split it into two commits - one where you upgrade and another where you apply your patch? Is it possible to have package.json load from third_party/rollup instead of third_party/node_modules/rollup ? We need to be careful to separate it. Have you sent your patch upstream? Please add a comment to package.json that links to the upstream PR so we know when get back onto the mainline.

@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 26, 2018

so can you split it into two commits - one where you upgrade and another where you apply your patch?

I can try. There are other commits in master that are unreleased yet. So either do a build off master, add that in, then do a build off my branch and add that in, or go to the nearest released and then do mine, which will contain some other stuff.

Is it possible to have package.json load from third_party/rollup instead of third_party/node_modules/rollup.

I don't think, but I might be wrong, that you can break out of typical module resolution that way. You can have the package.json load not from npm (which is what is there now, but something outside of the scope of the current directly. What I can do is take the tarball that I generated of my build and place it in third_party and point that package.json there, so it is full reproducible if needed. I should have done that in the first place.

Have you sent your patch upstream?

Yes. rollup/rollup#2531. The only comments that are allowed with yarn are duplicate key comments. I will do that.

@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 26, 2018

@ry I updated this, but I am seeing errors in CI that just don't seem to be related to the changes here.

@ry
Copy link
Member

ry commented Oct 26, 2018

@kitsonk Can you try rebasing?

FYI we had a discussion about this patch today and have some concerns about the number of objects created in the snapshot - and if that will have performance impacts on GC. I think we won't land in time for the imminent v0.1.10 release. I need to research the possibility of doing the decoding in Rust ... i'll update later on.

@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 26, 2018

@ry will rebase, no problem, saw part of the chat, could be worth investigating. It is worth dumping the sources in the map no matter what, but maybe can wait until accepted upstream.

@ry
Copy link
Member

ry commented Nov 4, 2018

@kitsonk Do you know if rollup made a release with your patch in it? I'll upgrade it if so.

I think we should land this patch and see how it effects benchmark numbers. If it's notably negative we'll revert and invest in Rust sourcemap decoding. What say you @piscisaureus ?

@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 4, 2018

@ry it just released 11 hours ago with my PR merged: https://github.com/rollup/rollup/releases/tag/v0.67.0

I have time to refresh this with the released version of rollup.

@kitsonk kitsonk force-pushed the fast_exceptions_no_sources branch 2 times, most recently from 484f0f9 to 0843f14 Compare November 5, 2018 00:26
@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 5, 2018

@ry CI seems to be acting up... 403 Forbidden on accessing the cache on appveyor!? 🤷‍♂️

ry and others added 2 commits November 5, 2018 13:07
Pro:
time ./out/debug/deno tests/error_001.ts  3.0s -> 0.4s

Con:
time ./tool/build.py snapshot              33s -> 1m52s
out/debug/gen/snapshot_deno.bin            39M -> 121M
@ry ry force-pushed the fast_exceptions_no_sources branch 2 times, most recently from 8beebff to 7c76a56 Compare November 5, 2018 20:19
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks!

@ry ry merged commit 5c51cff into denoland:master Nov 5, 2018
@hayd
Copy link
Contributor

hayd commented Nov 8, 2018

This seems to have affected performance in start time, throughput and syscalls.

@ry
Copy link
Member

ry commented Nov 8, 2018

@hayd Yes.

  • Execution time: the main goal of this patch was to reduce 001_error from 2.0s to 0.1. This was good. However warm startup time increased from 0.05 to 0.13 - this is bad.
  • Throughput: Because the cat programs are using pipe, they throw an exception at the end of their execution. A stack trace is being swallowed - which is ok. Thus, like, 001_error these got massive speedups.
  • HTTP seems unaffected.
  • Executable size increased because the snapshot size increased. The snapshot size increased because the source map is fully decoded now in the snapshot. main.js.map decreased because of @kitsonk's work in this PR.
  • Syscalls increased with more mmap calls - these are from V8 - and probably caused by increased GC/memory thrashing.

Overall I think this PR has been a win. However it's clear that decoding the source maps in the snapshot has negatively effected us. @kitsonk is now looking into decoding in Rust - which I think is the necessary future.

@kitsonk kitsonk deleted the fast_exceptions_no_sources branch August 2, 2022 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception throw is VERY slow in Deno
3 participants