-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Improve performance of upgrade tool #18068
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
Changes from all commits
e5fab7b
1c52968
56b094c
7020cc4
17a7cca
2c1c31b
f1572a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import { parseCandidate } from '../../../../tailwindcss/src/candidate' | ||
import type { DesignSystem } from '../../../../tailwindcss/src/design-system' | ||
import { DefaultMap } from '../../../../tailwindcss/src/utils/default-map' | ||
import * as version from '../../utils/version' | ||
|
||
const QUOTES = ['"', "'", '`'] | ||
|
@@ -44,16 +45,23 @@ export function isSafeMigration( | |
// Whitespace before the candidate | ||
location.contents[location.start - 1]?.match(/\s/) && | ||
// A colon followed by whitespace after the candidate | ||
location.contents.slice(location.end, location.end + 2)?.match(/^:\s/) && | ||
// A `<style` block is present before the candidate | ||
location.contents.slice(0, location.start).includes('<style') && | ||
// `</style>` is present after the candidate | ||
location.contents.slice(location.end).includes('</style>') | ||
location.contents.slice(location.end, location.end + 2)?.match(/^:\s/) | ||
) { | ||
return false | ||
// Compute all `<style>` ranges once and cache it for the current files | ||
let ranges = styleBlockRanges.get(location.contents) | ||
|
||
for (let i = 0; i < ranges.length; i += 2) { | ||
let start = ranges[i] | ||
let end = ranges[i + 1] | ||
|
||
// Check if the candidate is inside a `<style>` block | ||
if (location.start >= start && location.end <= end) { | ||
return false | ||
} | ||
} | ||
} | ||
|
||
let [candidate] = Array.from(parseCandidate(rawCandidate, designSystem)) | ||
let [candidate] = parseCandidate(rawCandidate, designSystem) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small improvement, no need to convert to an array first... |
||
|
||
// If we can't parse the candidate, then it's not a candidate at all. However, | ||
// we could be dealing with legacy classes like `tw__flex` in Tailwind CSS v3 | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 But let's not do that work in the first place, yeah? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh and that's because this branch was hit about 1 041 458 times in that codebase. |
||
} | ||
|
||
// Parsed a candidate succesfully, verify if it's a valid candidate | ||
// Parsed a candidate successfully, verify if it's a valid candidate | ||
else if (candidate) { | ||
// When we have variants, we can assume that the candidate is safe to migrate | ||
// because that requires colons. | ||
|
@@ -168,3 +176,30 @@ export function isSafeMigration( | |
|
||
return true | ||
} | ||
|
||
// Assumptions: | ||
// - All `<style` tags appear before the next `</style>` tag | ||
// - All `<style` tags are closed with `</style>` | ||
// - No nested `<style>` tags | ||
const styleBlockRanges = new DefaultMap((source: string) => { | ||
let ranges: number[] = [] | ||
let offset = 0 | ||
|
||
while (true) { | ||
let startTag = source.indexOf('<style', offset) | ||
if (startTag === -1) return ranges | ||
|
||
offset = startTag + 1 | ||
|
||
// Ensure the style looks like: | ||
// - `<style>` (closed) | ||
// - `<style …>` (with attributes) | ||
if (!source[startTag + 6].match(/[>\s]/)) continue | ||
|
||
let endTag = source.indexOf('</style>', offset) | ||
if (endTag === -1) return ranges | ||
offset = endTag + 1 | ||
|
||
ranges.push(startTag, endTag) | ||
} | ||
}) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, technically yes. We could make it some LRU cache indeed 🤔
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.
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.