Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fs: refactoring declaratively with the Array.fromAsync function #54644
fs: refactoring declaratively with the Array.fromAsync function #54644
Changes from 6 commits
07d075a
e2d8a69
dde434c
f4025e5
1a9ae15
db16e20
43ef1e4
cb2591a
1ff400e
c976446
7dbb93f
defe492
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO
n
should be a single (higher) value. Something like3e4
(but that might be too high)?100 is way to low.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: There seem to be too many cases. Suggest reducing a few options if possible. Please consider if it's possible to obtain sufficient results with fewer iterations and cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the review!
If it's the review you mentioned, can I ask if it would be a good idea to try reducing the number of
dir
andpattern
types?I thought about it this way:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think the configs would be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! There were definitely a variety of cases..! I fixed from defe492!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't seem to use boolean in benchmark (error on CI), so I fixed that in this commit!
Many Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to count the output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this statement affect benchmarking speeds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RedYetiDev
Thank you for the review!
I think wrapping a Promise with each iteration affects benchmarking, So I fixed it.
After benchmarking again, there seems to be a decrease in performance..🥲
So, I'm trying to close the current PR and upload a PR that only adds benchmarking code. Is that okay?
mode: async, recursive: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RedYetiDev, Sorry for the frequent comments.
I look forward to hearing from you.
Many thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there is a benchmark for this, but this should probably be benchmarked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RedYetiDev Thanks for the review!
There are currently no benchmarks, so I'll add them!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RedYetiDev
Sorry for being late!
I added the fs glob benchmark f4025e5.
The benchmark results are as follows:
Performance is significantly improved when asynchronous compared to synchronous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RedYetiDev
Please confirm when you have time. Many Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that benchmark is accurate. See my review comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RedYetiDev
Many Thanks, we were able to improve the benchmark code.
Here are the results of the improved benchmark!
The results of
recursive=true useAsync=true
are:The results of
recursive=true useAsync=false
are:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Looks like a bit of improvement