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

deno fmt is slow #2490

Closed
ry opened this issue Jun 10, 2019 · 24 comments · Fixed by #2928
Closed

deno fmt is slow #2490

ry opened this issue Jun 10, 2019 · 24 comments · Fixed by #2928

Comments

@ry
Copy link
Member

ry commented Jun 10, 2019

No description provided.

@ry ry mentioned this issue Jun 10, 2019
43 tasks
@hayd
Copy link
Contributor

hayd commented Jun 11, 2019

Is it preferable/faster to do something ourselves rather than shell out to prettier?

I was wondering if this is feasible to port:
https://github.com/vvakame/typescript-formatter/blob/master/lib/formatter.ts

@ry
Copy link
Member Author

ry commented Jun 11, 2019

It would be great to use the built-in TS formatter (as that one you linked to does) but it doesn't support wrapping

https://github.com/denoland/deno_third_party/blob/92159ba506cf2fb87f4779bebeb07fd3463d0063/node_modules/typescript/lib/typescript.d.ts#L5136-L5172

@kitsonk
Copy link
Contributor

kitsonk commented Jun 11, 2019

I looked into adding wrapping before, in addition to the TypeScript formatter, and it gets a bit ugly, and essentially you need to do an AST parse to make sure you are wrapping properly. It is non-trivial. I am not aware of something that effectively does that without a full AST parse.

@kt3k
Copy link
Member

kt3k commented Jun 11, 2019

#2473 (comment)

The other option is to include prettier in the compiler snapshot - which would make it run very fast.

If we take this path, can we still depend on deno_std to use glob and walk functions? Or should we perform deno fmt without any download?

@ry
Copy link
Member Author

ry commented Jun 11, 2019

If we take this path, can we still depends on deno_std to use glob and walk functions? Or should we perform deno fmt without any download?

Importing deno_std stuff is a problem... We should solve that somehow.

I wonder if we can use "deno bundle" for the main bundle now?

@kitsonk
Copy link
Contributor

kitsonk commented Jun 11, 2019

I wonder if we can use "deno bundle" for the main bundle now?

Not sure. We would need to embed the loader in there, and we don't have source map support yet, so errors thrown in the bundle would be obscure. The main bundle would be easier than the compiler, with having to include TypeScript. It is worth trying to get to a point where we would be happy. Having even part of our bundles be build with Deno itself would be a good thing.

@MarkTiedemann
Copy link
Contributor

Or should we perform deno fmt without any download?

I would advocate for that. This would allow people to format code in flaky or no network situations.

For example, I downloaded v0.8.0 yesterday on a 100 Mbps connection. Today, on a flaky 2G hotspot, I was able to recompile my code, but I wasn't able to format it: deno fmt failed twice after roughly 10 minutes with an uncaught HttpOther error, probably due to the flaky connection. This was kind of unexpected.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 12, 2019

@catastrop not very constructive feedback to be honest. Building a JavaScript/TypeScript formatter from scratch in Rust is a lot of work (with little value?). Unless you are aware of one that exists, than the suggestion isn't very practical.

@axetroy
Copy link
Contributor

axetroy commented Jun 12, 2019

Consider using deno bundle to package deno_std/prettier/main.ts into a single js file.

Then this js file is released with deno.

Deno fmt is executing this js file

@ry
Copy link
Member Author

ry commented Jun 12, 2019

Consider using deno bundle to package deno_std/prettier/main.ts into a single js file.

We have a faster and established way of doing this with V8 snapshots.

There are two issues

  1. We want to ensure that all deno programs are fast, not just ones included in the binary, maybe we should take this opportunity to investigate the root causes of the slowness. (2MB download is not much - I think there is some bug happening.)
  2. If we include it in the snapshot, we can't import the code directly from deno_std since snapshotted code uses a different import syntax (extensionless) and runs under rollup, whereas deno_std is real user code.

@luvies
Copy link
Contributor

luvies commented Jun 12, 2019

I feel like working on 1 first, and falling back to 2 if it simply doesn't match up to what is wanted, is probably a good way to move forward, since even if it doesn't work 100%, it would improve all deno code in general in terms of speed.

Personally, it also feels a little odd to embed the formatter in the main deno executable, since it's a bit of a mixing of concerns (deno being the main runtime, and the formatter being purely a dev tool, not reliant on any of the internals).

@MarkTiedemann
Copy link
Contributor

a mixing of concerns (deno being the main runtime, and the formatter being purely a dev tool

Personally, I like that Deno has common dev tooling out-of-the-box (like fmt, bundle, info, etc.) Separate concerns as subcommands makes it easy to distribute this tooling for Deno and easy to discover and use the tooling for users.

@MarkTiedemann
Copy link
Contributor

Speaking of formatting being slow... Last year, I implemented a small wrapper around Prettier called prettier-if-modified which made formatting incremental (only reformat files that were changed). This was implemented using extended file attributes (#2415 would be a prerequisite) and sped up the formatting of a codebase with a few thousand TypeScript files from 30s to a few ms (provided you only touched a few files).

@luvies
Copy link
Contributor

luvies commented Jun 13, 2019

Personally, I like that Deno has common dev tooling out-of-the-box (like fmt, bundle, info, etc.) Separate concerns as subcommands makes it easy to distribute this tooling for Deno and easy to discover and use the tooling for users.

I'm not against having the subcommand (it's nice to have a common standard, although I'm not a massive fan of the formatting options :p), just embedding the tool directly in deno. The go CLI has go fmt, but that calls the separate tool gofmt internally, it's not embedded in the CLI.

@ry
Copy link
Member Author

ry commented Jun 13, 2019

If we were able to easily include prettier in the compiler snapshot without increasing the executable size massively, I'd totally go for it. I suspect when we try to do that experiment, that it might add 10MB ... that would be unfortunate.

@ryanatkn
Copy link

ryanatkn commented Jun 15, 2019

For the curious, here's the current size of prettier's dist:

file kB uncompressed kB gzipped
bin-prettier.js 1388 312
doc.js 59 15
index.js 1329 297
parser-angular.js 56 13
parser-babylon.js 218 55
parser-flow.js 1430 188
parser-glimmer.js 138 43
parser-graphql.js 43 11
parser-html.js 79 22
parser-markdown.js 127 36
parser-postcss.js 913 184
parser-typescript.js 2261 591
parser-yaml.js 135 36
standalone.js 1000 225
third-party.js 140 33
total w/ readme/license/etc 9314 2096

@hayd
Copy link
Contributor

hayd commented Jun 15, 2019

Are these gzipped over the wire? Is deno using Accept-Encoding="gzip"?

@kitsonk
Copy link
Contributor

kitsonk commented Jun 16, 2019

No, actually that is an interesting point, something we should consider when fetching remote modules, supporting both gzip and brotli for module transfer. Most of the servers support it and it would speed up fetching remote modules. I will open another issue for it.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 16, 2019

Actually I already did in #1533. Doh! I think we should still do that.

@hayd
Copy link
Contributor

hayd commented Jun 16, 2019

I thought maybe #2477 would fix this, but it seems not. On investigating I think it's not the downloads which are slow(est): it's compilation. You can see this when passing -D.

One thought is bundling but atm bundle doesn't support args (I don't think). The bundle is 12.4MB.

It'd be great to see some flamegraphs/something to see what is taking the time here...

@ry
Copy link
Member Author

ry commented Sep 10, 2019

I did some investigation into the slowness of "deno fmt" on first run. It appears to hang during TypeScript parsing of std/prettier/vendor/parser_typescript.js which is a 2.2 MB javascript file.

Ideally we'd be able to opt-out of TypeScript trying to parse that file... Maybe we should turn off checkJs ?

@bartlomieju
Copy link
Member

@ry I believe this file is imported from other .ts file (prettier/main.ts? I'm on a phone can't check tho) so changing checkJs won't change anything as JS imports are still loaded by TS compiler during getSourceFile

@MarkTiedemann
Copy link
Contributor

Would providing a .d.ts for parser_typescript.js make tsc skip checking the JS in this case?

@bartlomieju
Copy link
Member

bartlomieju commented Sep 10, 2019

Would providing a .d.ts for parser_typescript.js make tsc skip checking the JS in this case?

I guess it should, I'm experimenting with it right now, but I found 2 nasty bugs along the way. Will get back

EDIT: Yep, it's super fast when using type definitions. I'll write up issues and starting writing fixes for the bugs to get this working.

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 a pull request may close this issue.

9 participants