Defer reading of transform source file - #13419
mitchhentgesspotify wants to merge 2 commits into
Conversation
| filename: string, | ||
| options?: InternalModuleOptions, | ||
| ): string { | ||
| const source = this.readFile(filename); |
There was a problem hiding this comment.
hmm, readFile looks in cacheFS, same as the transformer. does this actually change anything? it's the same read operation cached at slightly different times?
There was a problem hiding this comment.
If I'm wrong and it's not the same cache, we should fix that
There was a problem hiding this comment.
Yep, it's the same backing cacheFS instance 👍
does this actually change anything
Err, you're absolutely correct. This needs to go another layer deeper:
_transformAndBuildScript()still wants to read the source file- The only usage of the "original code" is here, in the return value
- The only usage of the "original code" is here, in the coverage reporter
TL;DR: This PR needs more work to avoid the unnecessary readFile, as it depends on whether or not coverage reporting is going on.
Sorry about that!
There was a problem hiding this comment.
Thanks for checking! I'm super happy somebody is looking into these performance improvements 🙂 death by a thousand cuts and all that
be337f5 to
75c5a59
Compare
If test files are already compiled and in the FS cache, then their source doesn't need to be read for the purposes of transforming. Note that they're probably still read elsewhere for hashing, but this reduces some potential unnecessary reads. Signed-off-by: Mitchell Hentges <mhentges@spotify.com>
Signed-off-by: Mitchell Hentges <mhentges@spotify.com>
|
Oof, I shouldn't be working on these things while travelling, I think the premise of this PR is entirely flawed: the key of transformed files is based off of the hash of source file contents. So, the source must be read anyways. I mentioned this in my summary ("Note that they're probably still read elsewhere for hashing, but this reduces some potential unnecessary reads."), but based off of your comment and looking at this again, I think that my premise is flawed. I'm going to close this for now: it would be nice if we could avoid reading source files when we know that the transformed results are up-to-date, but I don't think that up-to-date checking is possible without, well, hashing contents. |
|
We could probably |
|
Sorry, missclick |
True, I'd be more comfortable with something like this in |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
3 participants
If test files are already compiled and in the FS cache, then their source doesn't need to be read for the purposes of transforming.
Note that they're probably still read elsewhere for hashing, but this reduces some potential unnecessary reads.
Signed-off-by: Mitchell Hentges mhentges@spotify.com
Test plan
Within
_scriptTransformer.transform(...), it already has behaviour to populatesourcewhen it's not provided, so this should be solid.The existing test suite should provide some confidence too.