8000 vibe optimization by quantizor · Pull Request #547 · dcastil/tailwind-merge · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

vibe optimization #547

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

vibe optimization #547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
275 changes: 188 additions & 87 deletions src/lib/class-group-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,46 +8,97 @@ import {
ThemeGetter,
ThemeObject,
} from './types'
import { createLruCache } from './lru-cache'

export interface ClassPartObject {
nextPart: Map<string, ClassPartObject>
validators: ClassValidatorObject[]
classGroupId?: AnyClassGroupIds
classGroupId: AnyClassGroupIds | undefined // Always define optional props for consistent shape
}

interface ClassValidatorObject {
classGroupId: AnyClassGroupIds
validator: ClassValidator
}

// Factory function ensures consistent object shapes
const createClassValidatorObject = (
classGroupId: AnyClassGroupIds,
validator: ClassValidator,
): ClassValidatorObject => ({
classGroupId,
validator,
})

// Factory ensures consistent ClassPartObject shape
const createClassPartObject = (
nextPart: Map<string, ClassPartObject> = new Map(),
validators: ClassValidatorObject[] = [],
classGroupId?: AnyClassGroupIds,
): ClassPartObject => ({
nextPart,
validators,
classGroupId,
})

const CLASS_PART_SEPARATOR = '-'
const EMPTY_VALIDATORS: readonly ClassValidatorObject[] = []
const EMPTY_CONFLICTS: readonly AnyClassGroupIds[] = []
const ARBITRARY_PROPERTY_PREFIX = 'arbitrary..'
const ARBITRARY_PROPERTY_REGEX = /^\[(.+)\]$/

export const createClassGroupUtils = (config: AnyConfig) => {
const classMap = createClassMap(config)
const { conflictingClassGroups, conflictingClassGroupModifiers } = config

// Use the existing LRU cache implementation
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this comment, it's clear from the code that you're using the existing implementation

const classGroupCache = createLruCache<string, AnyClassGroupIds | undefined>(config.cacheSize)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the addition of the cache to a separate PR? I'm not sure about this though and would want to test this.

Some things I'm thinking of:

  1. I'm not sure how the size of this cache would correlate with the main cache size. Could it be that this cache is much smaller/bigger than the main one?
  2. How much speed do we gain with this cache alone (this is why I want to isolate it into its own PR)? We're paying for it with increased memory consumption and I don't yet know whether it would be worth it.


const getClassGroupId = (className: string) => {
const classParts = className.split(CLASS_PART_SEPARATOR)
// Check cache first for efficiency
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this comment, the code is self-explanatory

const cachedResult = classGroupCache.get(className)
if (cachedResult !== undefined) {
return cachedResult
}

// Classes like `-inset-1` produce an empty string as first classPart. We assume that classes for negative values are used correctly and remove it from classParts.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure that comments like this one stay in the codebase after your changes. Same with other instances.

if (classParts[0] === '' && classParts.length !== 1) {
classParts.shift()
let result: AnyClassGroupIds | undefined

if (className.startsWith('[')) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a good opportunity to also check for className.endsWith(']') here and then inside getGroupIdForArbitraryProperty we can assume that the clas is an arbitrary property and skip the regex matching entirely by just removing the first and last character from the string.

result = getGroupIdForArbitraryProperty(className)
} else {
const classParts = className.split(CLASS_PART_SEPARATOR)
if (classParts[0] === '' && classParts.length > 1) {
classParts.shift()
}
result = getGroupRecursive(classParts, classMap)
}

return getGroupRecursive(classParts, classMap) || getGroupIdForArbitraryProperty(className)
// Cache result to avoid repeated computation
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment can be removed, too

classGroupCache.set(className, result)
return result
}

const getConflictingClassGroupIds = (
classGroupId: AnyClassGroupIds,
hasPostfixModifier: boolean,
) => {
const conflicts = conflictingClassGroups[classGroupId] || []

if (hasPostfixModifier && conflictingClassGroupModifiers[classGroupId]) {
return [...conflicts, ...conflictingClassGroupModifiers[classGroupId]!]
): readonly AnyClassGroupIds[] => {
const conflicts = conflictingClassGroups[classGroupId]
if (!hasPostfixModifier || !conflictingClassGroupModifiers[classGroupId]) {
return conflicts || EMPTY_CONFLICTS
}

return conflicts
const modifierConflicts = conflictingClassGroupModifiers[classGroupId]!
if (!conflicts) return modifierConflicts

// Pre-allocate for better V8 optimization
const result = new Array(conflicts.length + modifierConflicts.length)
for (let i = 0; i < conflicts.length; i++) {
result[i] = conflicts[i]
}
for (let i = 0; i < modifierConflicts.length; i++) {
result[conflicts.length + i] = modifierConflicts[i]
}
Comment on lines +93 to +100
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting optimization. I expected V8 to pre-allocate in the spread operation as well since it knows the sizes of all involved arrays. But your code is indeed consistently 2-3x faster in my testing.

The code I tested with:

const num1 = []
const num2 = []

for (let i = 0; i < 100; i++) {
    num1.push(i)
    num2.push(i)
}

const arrs = []
const iterationsCount = 10_000

console.time('array spread')
for (let i = 0; i < iterationsCount; i++) {
    arrs.push([...num1, ...num2])
}
console.timeEnd('array spread')

console.time('preallocated array')
for (let i = 0; i < iterationsCount; i++) {
    const result = new Array(num1.length + num2.length)
    for (let i = 0; i < num1.length; i++) {
        result[i] = num1[i]
    }
    for (let i = 0; i < num2.length; i++) {
        result[num1.length + i] = num2[i]
    }
    arrs.push(result)
}
console.timeEnd('preallocated array')

return result
}

return {
Expand All @@ -60,58 +111,65 @@ const getGroupRecursive = (
classParts: string[],
classPartObject: ClassPartObject,
): AnyClassGroupIds | undefined => {
if (classParts.length === 0) {
const len = classParts.length
if (len === 0) {
Comment on lines +114 to +115
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to use a more descriptive name like classPartsLength since there is another length-related variable in the function.

return classPartObject.classGroupId
}

const currentClassPart = classParts[0]!
const nextClassPartObject = classPartObject.nextPart.get(currentClassPart)
const classGroupFromNextClassPart = nextClassPartObject
? getGroupRecursive(classParts.slice(1), nextClassPartObject)
: undefined

if (classGroupFromNextClassPart) {
return classGroupFromNextClassPart
if (nextClassPartObject) {
const result = getGroupRecursive(classParts.slice(1), nextClassPartObject)
if (result) return result
}

if (classPartObject.validators.length === 0) {
const validators = classPartObject.validators
if (validators.length === 0) {
return undefined
}

const classRest = classParts.join(CLASS_PART_SEPARATOR)
const len2 = validators.length
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also use a more descriptive name here, e.g. validatorsLength or validatorsCount.


return classPartObject.validators.find(({ validator }) => validator(classRest))?.classGroupId
for (let i = 0; i < len2; i++) {
const validatorObj = validators[i]!
if (validatorObj.validator(classRest)) {
return validatorObj.classGroupId
}
}

return undefined
}

const arbitraryPropertyRegex = /^\[(.+)\]$/
const getGroupIdForArbitraryProperty = (className: string): AnyClassGroupIds | undefined => {
const match = ARBITRARY_PROPERTY_REGEX.exec(className)
if (!match?.[1]) return undefined

const getGroupIdForArbitraryProperty = (className: string) => {
if (arbitraryPropertyRegex.test(className)) {
const arbitraryPropertyClassName = arbitraryPropertyRegex.exec(className)![1]
const property = arbitraryPropertyClassName?.substring(
0,
arbitraryPropertyClassName.indexOf(':'),
)
const colonIndex = match[1].indexOf(':')
if (colonIndex === -1) return undefined

if (property) {
// I use two dots here because one dot is used as prefix for class groups in plugins
return 'arbitrary..' + property
}
}
const property = match[1].slice(0, colonIndex)
return property ? ARBITRARY_PROPERTY_PREFIX + property : undefined
}

/**
* Exported for testing only
*/
export const createClassMap = (config: Config<AnyClassGroupIds, AnyThemeGroupIds>) => {
const { theme, classGroups } = config
const classMap: ClassPartObject = {
nextPart: new Map<string, ClassPartObject>(),
validators: [],
}
return processClassGroups(classGroups, theme)
}

// Split into separate functions to maintain monomorphic call sites
const processClassGroups = (
classGroups: Record<AnyClassGroupIds, ClassGroup<AnyThemeGroupIds>>,
theme: ThemeObject<AnyThemeGroupIds>,
): ClassPartObject => {
const classMap = createClassPartObject()

for (const classGroupId in classGroups) {
processClassesRecursively(classGroups[classGroupId]!, classMap, classGroupId, theme)
const group = classGroups[classGroupId]
if (group) {
processClassesRecursively(group, classMap, classGroupId, theme)
}
Comment on lines +169 to +172
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just add a non-null assertion here and skip the if statement at runtime since group should never be falsy.

Suggested change
const group = classGroups[classGroupId]
if (group) {
processClassesRecursively(group, classMap, classGroupId, theme)
}
const group = classGroups[classGroupId]!
processClassesRecursively(group, classMap, classGroupId, theme)

}

return classMap
Expand All @@ -123,60 +181,103 @@ const processClassesRecursively = (
classGroupId: AnyClassGroupIds,
theme: ThemeObject<AnyThemeGroupIds>,
) => {
classGroup.forEach((classDefinition) => {
if (typeof classDefinition === 'string') {
const classPartObjectToEdit =
classDefinition === '' ? classPartObject : getPart(classPartObject, classDefinition)
classPartObjectToEdit.classGroupId = classGroupId
return
}
const len = classGroup.length
for (let i = 0; i < len; i++) {
const classDefinition = classGroup[i]!
processClassDefinition(classDefinition, classPartObject, classGroupId, theme)
}
}

if (typeof classDefinition === 'function') {
if (isThemeGetter(classDefinition)) {
processClassesRecursively(
classDefinition(theme),
classPartObject,
classGroupId,
theme,
)
return
}
// Split into separate functions for each type to maintain monomorphic call sites
const processClassDefinition = (
classDefinition: ClassGroup<AnyThemeGroupIds>[number],
classPartObject: ClassPartObject,
classGroupId: AnyClassGroupIds,
theme: ThemeObject<AnyThemeGroupIds>,
) => {
if (typeof classDefinition === 'string') {
processStringDefinition(classDefinition, classPartObject, classGroupId)
return
}

classPartObject.validators.push({
validator: classDefinition,
classGroupId,
})
if (typeof classDefinition === 'function') {
processFunctionDefinition(classDefinition, classPartObject, classGroupId, theme)
return
}

return
}
if (classDefinition) {
processObjectDefinition(
classDefinition as Record<string, ClassGroup<AnyThemeGroupIds>>,
classPartObject,
classGroupId,
theme,
)
}
Comment on lines +208 to +215
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to check for truthiness here since there wasn't a check in the previous implementation either

Suggested change
if (classDefinition) {
processObjectDefinition(
classDefinition as Record<string, ClassGroup<AnyThemeGroupIds>>,
classPartObject,
classGroupId,
theme,
)
}
processObjectDefinition(
classDefinition as Record<string, ClassGroup<AnyThemeGroupIds>>,
classPartObject,
classGroupId,
theme,
)

}

const processStringDefinition = (
classDefinition: string,
classPartObject: ClassPartObject,
classGroupId: AnyClassGroupIds,
) => {
const classPartObjectToEdit =
classDefinition === '' ? classPartObject : getPart(classPartObject, classDefinition)
classPartObjectToEdit.classGroupId = classGroupId
}

const processFunctionDefinition = (
classDefinition: Function,
classPartObject: ClassPartObject,
classGroupId: AnyClassGroupIds,
theme: ThemeObject<AnyThemeGroupIds>,
) => {
if (isThemeGetter(classDefinition)) {
processClassesRecursively(classDefinition(theme), classPartObject, classGroupId, theme)
return
}

Object.entries(classDefinition).forEach(([key, classGroup]) => {
processClassesRecursively(
classGroup,
getPart(classPartObject, key),
classGroupId,
theme,
)
})
})
if (classPartObject.validators === EMPTY_VALIDATORS) {
classPartObject.validators = []
}
Comment on lines +239 to +241
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I think this could be quite the footgun. We can skip the empty arrays entirely and just make validators undefined when there are no validators present. Then in getGroupRecursive we can do

- if (validators.length === 0) {
+ if (validators === undefined) {

classPartObject.validators.push(
createClassValidatorObject(classGroupId, classDefinition as ClassValidator),
)
}

const processObjectDefinition = (
classDefinition: Record<string, ClassGroup<AnyThemeGroupIds>>,
classPartObject: ClassPartObject,
classGroupId: AnyClassGroupIds,
theme: ThemeObject<AnyThemeGroupIds>,
) => {
const entries = Object.entries(classDefinition)
const len = entries.length
for (let i = 0; i < len; i++) {
const [key, value] = entries[i]!
processClassesRecursively(value, getPart(classPartObject, key), classGroupId, theme)
}
}

const getPart = (classPartObject: ClassPartObject, path: string) => {
let currentClassPartObject = classPartObject
const getPart = (classPartObject: ClassPartObject, path: string): ClassPartObject => {
let current = classPartObject
const parts = path.split(CLASS_PART_SEPARATOR)
const len = parts.length

path.split(CLASS_PART_SEPARATOR).forEach((pathPart) => {
if (!currentClassPartObject.nextPart.has(pathPart)) {
currentClassPartObject.nextPart.set(pathPart, {
nextPart: new Map(),
validators: [],
})
}
for (let i = 0; i < len; i++) {
const part = parts[i]
if (!part) continue
Comment on lines +267 to +268
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use a non-null assertion here and skip the if statement at runtime.

Suggested change
const part = parts[i]
if (!part) continue
const part = parts[i]!


currentClassPartObject = currentClassPartObject.nextPart.get(pathPart)!
})
let next = current.nextPart.get(part)
if (!next) {
next = createClassPartObject()
current.nextPart.set(part, next)
}
current = next
}

return currentClassPartObject
return current
}

const isThemeGetter = (func: ClassValidator | ThemeGetter): func is ThemeGetter =>
(func as ThemeGetter).isThemeGetter
// Type guard maintains monomorphic check
const isThemeGetter = (func: Function): func is ThemeGetter =>
'isThemeGetter' in func && (func as ThemeGetter).isThemeGetter === true
2 changes: 1 addition & 1 deletion src/lib/default-config.ts
View file
Open in desktop
C94A
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ export const getDefaultConfig = () => {
const scaleTranslate = () => [isFraction, 'full', ...scaleUnambiguousSpacing()] as const

return {
cacheSize: 500,
cacheSize: 20000,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this to a separate PR? I agree that 500 is probably quite small, but would be interesting to see other values make sense. I'd prefer to keep the value as small as possible but big enough for a reasonably sized project to not get over the cache size too quickly.

I'm hesitant to increase it to 20k because that would increase the memory footprint of tailwind-merge quite a bit. Maybe 2k also works?

theme: {
animate: ['spin', 'ping', 'pulse', 'bounce'],
aspect: ['video'],
Expand Down
8 changes: 6 additions & 2 deletions src/lib/from-theme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import { DefaultThemeGroupIds, NoInfer, ThemeGetter, ThemeObject } from './types'

const fallbackThemeArr: ThemeObject<DefaultThemeGroupIds>[DefaultThemeGroupIds] = []

export const fromTheme = <
AdditionalThemeGroupIds extends string = never,
DefaultThemeGroupIdsInner extends string = DefaultThemeGroupIds,
>(key: NoInfer<DefaultThemeGroupIdsInner | AdditionalThemeGroupIds>): ThemeGetter => {
>(
key: NoInfer<DefaultThemeGroupIdsInner | AdditionalThemeGroupIds>,
): ThemeGetter => {
const themeGetter = (theme: ThemeObject<DefaultThemeGroupIdsInner | AdditionalThemeGroupIds>) =>
theme[key] || []
theme[key] || fallbackThemeArr

themeGetter.isThemeGetter = true as const

Expand Down
Loading
0