Improve performance of upgrade tool #18068
Conversation
| } | ||
|
|
||
| let [candidate] = Array.from(parseCandidate(rawCandidate, designSystem)) | ||
| let [candidate] = parseCandidate(rawCandidate, designSystem) |
There was a problem hiding this comment.
Small improvement, no need to convert to an array first...
| @@ -62,10 +70,10 @@ export function isSafeMigration( | |||
| // So let's only skip if we couldn't parse and we are not in Tailwind CSS v3. | |||
| // | |||
| if (!candidate && version.isGreaterThan(3)) { | |||
| return true | |||
| return false | |||
There was a problem hiding this comment.
I'm a bit of a dumb dumb here. When we know that a candidate doesn't parse, and we know that we are in v4 and up, then we know that we can't migrate the candidate anyway so we can bail.
That makes it an unsafe migration (which skips all the migrations for this candidate). We were returning true which means that we were migrating the candidate. This still worked because eventually the migrations were no-ops because they all bailed.
But let's not do that work in the first place, yeah?
There was a problem hiding this comment.
This change alone makes the upgrade tool on Tailwind UI's product folder go from ~12s to ~9s
There was a problem hiding this comment.
Oh and that's because this branch was hit about 1 041 458 times in that codebase.
packages/@tailwindcss-upgrade/src/codemods/template/is-safe-migration.ts
Outdated
Show resolved
Hide resolved
| ) { | ||
| return false | ||
| // Compute all `<style>` ranges once and cache it for the current files | ||
| let ranges = styleBlockRanges.get(location.contents) |
There was a problem hiding this comment.
Will this map end up having the content of all your repo in there by the end of it? We've already seen people having memory issues with the upgrade tool (#17228) so I wonder if we should do something a bit smarter here. E.g. it's probably enough to only retain a list of up to NUM_CORES files EVER since files are always completed before we start a new one or so?
There was a problem hiding this comment.
Hmm, technically yes. We could make it some LRU cache indeed 🤔
There was a problem hiding this comment.
For future reference:
Tested this on our Tailwind UI codebase, and it indeed results in ~82mb of memory usage. However, the very last file we scan is an 650k line JSON file so even with an LRU cache in place it's still ~71mb of memory usage in our case.
I believe the biggest bottleneck is if you are scanning too many files (aka, node_modules is scanned for some reason) and that that's the root cause instead of this pre-computed cache.
Going to keep it as-is for now, but if people run into memory issues like this, then we know where to look.
We already know we are in v4 and up and it didn't compile. All migrations will fail but will still run, so now we can bail early.
This allows us to compute all the positions once and cache it per file contents. This is useful because otherwise we have to check the exact same file over and over and over again, but just for different positions. This also just computes the ranges of start and end positions. This will allow us to simply check if the candidate is between any of 2 pair points to know if we are in a `<style>…</style>` or not.
There is no need to convert the iterator to an array first. We can just destructure the first value immediately.
…gration.ts Co-authored-by: Jordan Pittman <jordan@cryptica.me>
RobinMalfait
force-pushed
the
feat/upgrade-performance
branch
from
4d62bf8 to
f1572a4
Compare
This PR improves the performance of the upgrade tool due to a regression introduced by #18057
Essentially, we had to make sure that we are not in
<style>…</style>tags because we don't want to migrate declarations in there such asflex-shrink: 0;The issue with this approach is that we checked before the candidate if a
<stylecold be found and if we found an</style>tag after the candidate.We would basically do this check for every candidate that matches.
Running this on our Tailwind UI codebase, this resulted in a bit of a slowdown:
... quite the difference.
This is because we have a snapshot file that contains ~650k lines of code. Looking for
<style>and</style>tags in a file that large is expensive, especially if we do it a lot.I ran some numbers and that file contains ~1.8 million candidates.
Anyway, this PR fixes that by doing a few things:
<style>and</style>tag positions only once per file and cache it. This allows us to re-use this work for every candidate that needs it.Running the numbers now gets us to:
Much better!