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

Added cache for some FS operations while compile #829

Merged
merged 12 commits into from
Sep 9, 2018
Merged

Added cache for some FS operations while compile #829

merged 12 commits into from
Sep 9, 2018

Conversation

timocov
Copy link
Contributor

@timocov timocov commented Aug 29, 2018

Fixes #825.

Open questions:

  • Should it be under option?
  • If so, how this option should be named? Should it be something like "experimental" flag?
  • Do we need to add logs? Is so, what kind of logs (clearing caches or something else)?
  • Should we add the same caches to makeWatchHost too?
  • I'm not sure how to write tests for this - possible somebody has thoughts about it.

@johnnyreilly
Copy link
Member

Thanks for the PR! I'm away from home right now with only my phone and dodgy connectivity and so will struggle to review this until in front of a proper screen.

To your questions:

Should it be under option? If so, how this option should be named? Should it be something like "experimental" flag?

Yes please; I like the idea of this going out under a flag called something like experimentalFileCaching. If it works well and no problems are reported we can look to promote this to the default behaviour in future.

Do we need to add logs? Is so, what kind of logs (clearing caches or something else)?

Don't feel strongly. I'd say probably not for now.

Should we add the same caches to makeWatchHost too?

I'm not sure; I think not for now.

I'm not sure how to write tests for this - possible somebody has thoughts about it.

The encouraging thing is that tests pass before you've put it behind an option. But yeah - need to think about this

@johnnyreilly
Copy link
Member

Also, please could you experiment with this on your own projects and document the performance improvements you're seeing here please?

I'd like the benefits to be easily discoverable for people who want learn about this. Thanks!

@timocov
Copy link
Contributor Author

timocov commented Aug 29, 2018

Yes please; I like the idea of this going out under a flag called something like experimentalFileCaching. If it works well and no problems are reported we can look to promote this to the default behaviour in future.

Ok, I'll do it.

Also, please could you experiment with this on your own projects and document the performance improvements you're seeing here please?

Sure. But currently I am able to check this fix against 3.5.0 version only (I have fix for this version too - see https://github.com/timocov/ts-loader/tree/fix825_v3) because we are in progress to upgrade to webpack 4 for now. But I'll try to check with v4 too.

@johnnyreilly
Copy link
Member

Sure. But currently I am able to check this fix against 3.5.0 version only

I'm guessing you know that ts-loader has supported only webpack 4+ since version 4?

@timocov
Copy link
Contributor Author

timocov commented Aug 29, 2018

I'm guessing you know that ts-loader has supported only webpack 4+ since version 4?

Yep 🙂

@timocov
Copy link
Contributor Author

timocov commented Aug 29, 2018

Here are cold-compilation results for our project.

Env:

  • PC: AMD FX-4300 Quad-Core 4x3.8GHz, 16GB RAM, HDD, Windows 10
  • NodeJS version: 10.3.0
  • TypeScript version: 3.0.1
  • Approx. modules count: 3000-4700 (from webpack stats)

webpack@3.10.0 runs:

ts-loader version compilation time (avg of three runs)
2.3.3 554s (546s, 553s, 564s)
3.5.0 828s (850s, 810s, 823s)
3.5.0 (with cache) 303s (296s, 312s, 302s)

webpack@4.16.0 runs:

ts-loader version compilation time (avg of two runs)
4.5.0 419s (413s, 425s)
4.5.0 (with cache) 315s (307s, 324s)

I think that we cannot compare results between webpack 3 and webpack 4 because there are some changes in our project for webpack 4 and I cannot guaratnee that the build has exactly the same input (but inside one webpack version it should be the same).

@timocov
Copy link
Contributor Author

timocov commented Aug 29, 2018

I'm guessing you know that ts-loader has supported only webpack 4+ since version 4?

If this PR will be merged, is there any chance to make the similar PR for 3 version (I'm ready to provide necessary changes for 3.5 version)?

@johnnyreilly
Copy link
Member

If this PR will be merged, is there any chance to make the similar PR for 3 version (I'm ready to provide necessary changes for 3.5 version)?

You mean a PR for this branch: https://github.com/TypeStrong/ts-loader/tree/ts-loader-3?files=1

Which I'd then publish as 3.6?

Honestly I'm not keen since I've not been supporting webpack 3 since 4 shipped and I only have a certain amount of time I can devote to open source. This would take away part of that time and given people are moving away from webpack 3 it doesn't feel like time well spent.

Will that be super painful for you?

@timocov
Copy link
Contributor Author

timocov commented Aug 29, 2018

given people are moving away from webpack 3

Hm, good point 👍

Will that be super painful for you?

I guess in this case we can just publish it to local npm until we'll upgrade to webpack 4.

@timocov
Copy link
Contributor Author

timocov commented Sep 3, 2018

@johnnyreilly do I need to do something else in the PR? It is possible that you have any confused/ambiguously thoughts about it (I guess except tests)?

@johnnyreilly
Copy link
Member

Sorry - I hadn't spotted that you'd added further commits. I'm back from my travels now; I'll try and take a look soon. Please bear with me!

src/instances.ts Outdated Show resolved Hide resolved
src/instances.ts Outdated
"You may be using an old version of webpack; please check you're using at least version 4"
);
if (cachedServicesHost.clearCache !== null) {
loader._compiler.hooks.watchRun.tap(
Copy link
Member

Choose a reason for hiding this comment

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

I just wanted to check; is watchRun definitely sync not async?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Is there any advantage to using tapAsync instead of tap? I've an idea it doesn't really matter but I thought I'd check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that it depends on what are you doing in your handler. In this case we need to use sync because we need to just clear cache without any async operation. But if you find something interesting about it - please let me know 🙂

src/servicesHost.ts Outdated Show resolved Hide resolved
src/servicesHost.ts Outdated Show resolved Hide resolved
src/servicesHost.ts Outdated Show resolved Hide resolved
- Added type Action
- CachedServicesHost renamed to ServiceHostWhichMayBeCacheable
- cachedServicesHost variable renamed back to servicesHost
src/servicesHost.ts Outdated Show resolved Hide resolved
@timocov
Copy link
Contributor Author

timocov commented Sep 5, 2018

@johnnyreilly can you please take a look?

@johnnyreilly
Copy link
Member

Thanks @timocov - I will!

@johnnyreilly
Copy link
Member

Really sorry About the delay @timocov - very very busy right now. I haven't forgotten about this - just not had time

@timocov
Copy link
Contributor Author

timocov commented Sep 6, 2018

No problem 🙂

fileExists: compiler.sys.fileExists,
readFile: compiler.sys.readFile,
fileExists: moduleResolutionHost.fileExists,
readFile: moduleResolutionHost.readFile,
readDirectory: compiler.sys.readDirectory,
Copy link
Member

Choose a reason for hiding this comment

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

was there a reason that readDirectory wasn't included in the caching functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. I didn't see this function in the profiler before and I don't think that it can reduce perf, but if you want - I can check and provide the stats.

Copy link
Member

Choose a reason for hiding this comment

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

If you could check that'd be awesome. I'm currently working on tweaking the test packs so they can be made to work against a variety of experimental... flags - the idea being that while the cache functionality lies behind a flag we can still make sure it is tested by the existing test pack. Watch this space...

#834

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you could check that'd be awesome

Unfortunately for now I'm away from work and cannot check it. I'll do it in next couple of days and will provide here results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnnyreilly I just checked readDirectory - it seems that this function is never called while compilation (at least in our case).


useCaseSensitiveFileNames: () => compiler.sys.useCaseSensitiveFileNames,

realpath: moduleResolutionHost.realpath,
Copy link
Member

Choose a reason for hiding this comment

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

We didn't have realpath before I think; I'm just curious; what is it used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To resolve path to original one. For example, if you have symlink.

Copy link
Member

Choose a reason for hiding this comment

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

Ah - we've had since we added symlink support here: https://github.com/TypeStrong/ts-loader/pull/774/files

We just didn't supply it to the LanguageServiceHost until now though. I'm kind of surprised we didn't get a compilation error previously 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, interesting. Perhaps microsoft/TypeScript#12020 (comment) (and the whole PR) is related to this.

1. Made variable names more descriptive
2. `cacheableFunctions` is now declared once and re-used (micro-optimisation TBH)
@johnnyreilly
Copy link
Member

Hey @timocov,

I've been beavering away on the test pack and I think I've now got the comparison test pack set up so it does 2 passes; the existing pass and then again but with the experimentalFileCaching flag set. I'm just waiting for CI to pass. Assuming it does I think we're nearly ready to merge. 😄

@johnnyreilly johnnyreilly merged commit 078fab7 into TypeStrong:master Sep 9, 2018
@johnnyreilly
Copy link
Member

Released with v5.1.0 https://github.com/TypeStrong/ts-loader/releases/tag/v5.1.0

Thanks for all your work!

@timocov timocov deleted the fix825 branch September 9, 2018 20:35
@timocov
Copy link
Contributor Author

timocov commented Sep 9, 2018

Wow, thank you @johnnyreilly!

@johnnyreilly
Copy link
Member

My pleasure - you've been an awesome contributor! ❤️ 🌻

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.

2 participants