ReactiveX / rxjs

A reactive programming library for JavaScript
https://rxjs.dev
Apache License 2.0
30.7k stars 3k forks source link

Should we need to consider the compile speed in a product code with type definitions of rx v5? #1356

Closed tetsuharuohzeki closed 8 years ago

tetsuharuohzeki commented 8 years ago

Notice

As an additional information, I tried to upgrade to rx@5.0.0-beta.2 from rx@4.0.7 for the private code of my day job.

This is the comparison (sorry, I forgot diagnostic options...):

This upgrading needs some migration to my code thus this is not simply comparison. But this indicates that the type definitions of rxjs v5 is more complex than v4.

By this result, I feel we might to need to consider the compile speed in a product code with type definitions of rx v5. Of course, I strongly hope TypeScript team would fix this problem in the compiler.

How should we resolve this?

kwonoj commented 8 years ago

Build time should be one of topics may need to track down further, up until now it was mainly focused on build time of library itself instead of any consumer perspective. longer than 2h of build time seems really serious, is there way to replicate it with some simple code snippet can be shared?

As an initial thought (rough guess though) I think this is somewhat related with type inference by references inside of codebases, not by complex type signatures. RxJS4's type definition already have complex signature more than what current RxJS5 does have. Still need further isolation to see what's causing this.

kwonoj commented 8 years ago

/cc @david-driscoll also to share, he might be interested as well.

david-driscoll commented 8 years ago

@saneyuki do you have multiple different project references to RxJS (nested modules, etc?). I noticed a similar issue (see https://github.com/Microsoft/TypeScript/issues/5695) this may be the problem you're seeing, but I'm not entirely certain. I haven't gotten back to it since my last comment there, but it sounds there is a possible workaround (less than ideal though)

tetsuharuohzeki commented 8 years ago

@kwonoj

is there way to replicate it with some simple code snippet can be shared?

Ugh, I still have not found the simple test case for 2hr~ worst case....

By the way, in other my personal project (which is different from the above 2hr~ project), this commit improves the compile time dramatically about ~18x improvement. (see more details: https://github.com/karen-irc/karen/pull/574#issuecomment-182933596)

As an initial thought (rough guess though) I think this is somewhat related with type inference by references inside of codebases, not by complex type signatures. RxJS4's type definition already have complex signature more than what current RxJS5 does have. Still need further isolation to see what's causing this.

I agree. We need to continue to tracking this.


@david-driscoll

do you have multiple different project references to RxJS (nested modules, etc?).

No. I just working to upgrade to v5 from v4, then I switched to v5 completely.

gytisgreitai commented 8 years ago

I've just tried to introduce RxJS v5 to my small nodejs project And compile time increase is impressive with just couple of lines of RxJS code (bindNodeCallback, throw, of, flatMap in a single class). :)

#Typescript Version 1.9.0-dev.20160315 without @reactivex/rxjs
tsc --p tsconfig.json --outDir ../generated/ -d --diagnostics
Files:             107
Lines:           44517
Nodes:          161511
Identifiers:     61623
Symbols:       3838799
Types:           11879
Memory used:    92948K
I/O read:        0.01s
I/O write:       0.03s
Parse time:      0.60s
Bind time:       0.46s
Check time:      1.51s
Emit time:       0.28s
Total time:      2.85s

#Typescript Version 1.9.0-dev.20160315 with @reactivex/rxjs
tsc --p tsconfig.json --outDir ../generated/ -d --diagnostics
Files:             165
Lines:           45897
Nodes:          176476
Identifiers:     67587
Symbols:       7426360
Types:          661767
Memory used:   938877K
I/O read:        0.07s
I/O write:       0.08s
Parse time:      0.58s
Bind time:       0.38s
Check time:     14.43s
Emit time:       1.03s
Total time:     16.42s

#Typescript Version 1.8.7 without @reactivex/rxjs
Files:             107
Lines:           44426
Nodes:          160944
Identifiers:     61447
Symbols:       3827377
Types:           11844
Memory used:    93924K
I/O read:        0.07s
I/O write:       0.04s
Parse time:      0.64s
Bind time:       0.47s
Check time:      1.61s
Emit time:       0.29s
Total time:      3.01s

#Typescript Version 1.8.7 with @reactivex/rxjs
Files:             165
Lines:           45806
Nodes:          175909
Identifiers:     67411
Symbols:       7388831
Types:          649879
Memory used:   933061K
I/O read:        0.07s
I/O write:       0.08s
Parse time:      0.75s
Bind time:       0.52s
Check time:     17.63s
Emit time:       0.90s
Total time:     19.80s

MacBook Pro (Retina, 13-inch, Late 2013), 2,4 GHz Intel Core i5, 8 GB

benlesh commented 8 years ago

I've been talking to the TypeScript folks, and we have a few fixes that will drastically improve this.

kwonoj commented 8 years ago

I assume latest PR https://github.com/ReactiveX/RxJS/pull/1475 is addressing this issue. @saneyuki , would you able to confirm if latest changes helps? Still there may be continuous improvement in further, I think immediate improvement will help to solve worst case scenarios.

tetsuharuohzeki commented 8 years ago

I confirmed it's a time that we can close this issue. Thank you @vladima and all!

lock[bot] commented 6 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.