perf: hunt down more serial awaits#2350
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: c5c8d30 The changes in this PR will be included in the next version bump. This PR includes changesets to release 25 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| "@flint.fyi/cli": patch | ||
| --- | ||
|
|
||
| Improve performance by a factor of 2. |
There was a problem hiding this comment.
It went from 2m to 1m on my machine, but it went from 1:25 to 1:22 in CI so... :)
auvred
left a comment
auvred
left a comment
There was a problem hiding this comment.
Again, we will probabaly have to refactor this file from the ground up due to #1253 (comment); however I'm not against merging this!
| flint.presets.stylistic, | ||
| node.presets.logicalStrict, | ||
| node.presets.stylisticStrict, | ||
| performance.presets.logical, |
There was a problem hiding this comment.
Looks like this is unrelated to the PR?
There was a problem hiding this comment.
Oh, I was using the loopAwaits to track these down. Now that you say it though, I figure I can drop it from this PR b/c I didn't feel like fixing the loopFunctions violations.
Aside: Now that I say that, why isn't loopFunctions in the typescript plugin? That's a logic issue right?
There was a problem hiding this comment.
Cross-linking for posterity, #2355 - because it's really annoying when you aren't optimizing for performance. Maybe we'd want to enable it for just core & cli, or something like that?
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Agreed, this is great - and can definitely land before the big project-aware refactor(s).
| // Why read file many time when few do trick? | ||
| // Or, at least it should all be virtual... | ||
| // https://github.com/flint-fyi/flint/issues/73 | ||
| // flint-disable-next-line performance/loopAwaits |
There was a problem hiding this comment.
This is what I mean by the annoyance of the loopAwaits rule. It's such a nitpicky thing!
There was a problem hiding this comment.
Huh, I feel the opposite. To me it feels like it pointed out something we should be parallelizing to be good to our users, but I'm lazy and just skipped it (oh, we should implement baselines!.. someday).
There was a problem hiding this comment.
Hah, fair. I suppose this case is a good example of it being useful then 😄
| if (!fileResults.reports.length) { | ||
| return; | ||
| } | ||
| const sourceFileText = await fs.readFile(filePath, "utf-8"); |
There was a problem hiding this comment.
Just noting https://github.com/flint-fyi/flint/pull/2200/changes#diff-48fb99ed9bf971f8e02835a414bd4b3c49221bfea27c822b3bc6c23139a9e023: that'd be nice too, and maybe would eliminate part of the asynchronous-yness here?
There was a problem hiding this comment.
Yeah I'm cleaning up #2294 and I think that should help with some of this (not this one specifically I think but there were some relevant conflicts)
JoshuaKGoldberg
added
the
1 approval
lishaduck
removed
the
status: blocked
auvred
added
the
ready to merge
3 participants
PR Checklist
status: accepting prsOverview
#2343 but more of it and automated and failing CI rn.