Improve performance of upgrade tool #18068
Conversation
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.
| } | ||
|
|
||
| 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.
| // Ensure the style looks like: | ||
| // - `<style>` (closed) | ||
| // - `<style …>` (with attributes) | ||
| if (!source[startTag + 6].match(/[>\s]/)) return ranges | ||
|
|
||
| offset = startTag + 1 |
There was a problem hiding this comment.
This will stop scanning as soon as it sees a tag that starts with <style but isn't <style>. e.g. <styleless>. How about this?
| // Ensure the style looks like: | |
| // - `<style>` (closed) | |
| // - `<style …>` (with attributes) | |
| if (!source[startTag + 6].match(/[>\s]/)) return ranges | |
| offset = startTag + 1 | |
| offset = startTag + 1 | |
| // Ensure the style looks like: | |
| // - `<style>` (closed) | |
| // - `<style …>` (with attributes) | |
| if (!source[startTag + 6].match(/[>\s]/)) continue |
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!