fix(watchman): Parallelize Watchman calls in crawler again#5640
fix(watchman): Parallelize Watchman calls in crawler again#5640
Conversation
This is a follow up to #5615 where it made all async watchman commands serialized when converting them from promises to `async` functions. Existing Watchman crawler tests should pass, maybe slightly faster.
|
/cc @cpojer @jeanlauliac |
| } else { | ||
| // Make the filter directories an empty array to signal that this root | ||
| // was already seen and needs to be watched for all files/directories | ||
| watchmanRoots.set(response.watch, []); |
There was a problem hiding this comment.
Slightly concerned about race conditions here but it should be fine since the main thread is memory-safe, right?
There was a problem hiding this comment.
yes, this is completely fine
| shouldReset = shouldReset || response.is_fresh_instance; | ||
| watchmanFileResults.set(root, response); | ||
| } | ||
| const response = await cmd('query', root, query); |
There was a problem hiding this comment.
Instead of what I did above, I can also just return [root, cmd('query', root, query)] here and then do
const watchmanFileResults = new Map(await Promise.all(...));
That said I still need to loop through responses and determine if shouldReset needs to be set to true, which would probably be
const watcmanRawResults = await Promise.all(...);
const watchmanFileResults = new Map(watcmanRawResults);
const shouldReset = watcmanRawResults.some(([_root, response]) => response.is_fresh_instance);
I'm kind of liking this new version but what do you think?
There was a problem hiding this comment.
I think this is fine tbh
davidaurelio
left a comment
davidaurelio
left a comment
There was a problem hiding this comment.
Thanks, this is nicer
| } else { | ||
| // Make the filter directories an empty array to signal that this root | ||
| // was already seen and needs to be watched for all files/directories | ||
| watchmanRoots.set(response.watch, []); |
There was a problem hiding this comment.
yes, this is completely fine
| shouldReset = shouldReset || response.is_fresh_instance; | ||
| watchmanFileResults.set(root, response); | ||
| } | ||
| const response = await cmd('query', root, query); |
There was a problem hiding this comment.
I think this is fine tbh
| // A root can only be filtered if it was never seen with a relative_path before | ||
| const canBeFiltered = !existing || existing.length > 0; | ||
| await Promise.all( | ||
| roots.map(async root => { |
There was a problem hiding this comment.
a function that you could hoist out :-)
| let files = data.files; | ||
|
|
||
| try { | ||
| async function getWatcmanRoots(roots) { |
There was a problem hiding this comment.
typo, you are missing a “h”
Codecov Report
@@ Coverage Diff @@
## master #5640 +/- ##
==========================================
+ Coverage 62.28% 62.31% +0.03%
==========================================
Files 216 216
Lines 7898 7903 +5
Branches 3 4 +1
==========================================
+ Hits 4919 4925 +6
+ Misses 2978 2977 -1
Partials 1 1
Continue to review full report at Codecov.
|
|
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. |
|
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. |
5 participants
Summary
This is a follow up to #5615 where it made all async watchman
commands serialized when converting them from promises to
asyncfunctions.
Test plan
Existing Watchman crawler tests should pass, maybe slightly faster.