Skip to content
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

Jump to bottom

Fix "Maximum call stack size exceeded" bug #8636

Merged
merged 4 commits into from Jun 14, 2022
Merged

Fix "Maximum call stack size exceeded" bug #8636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be550e5
Fix potential call stack size issue
adamwathan Jun 14, 2022
e204969
Update defaultExtractor.js
adamwathan Jun 14, 2022
ef60b6f
add test to verify "Maximum call stack size exceeded" is fixed
RobinMalfait Jun 14, 2022
3d06fb5
update changelog
RobinMalfait Jun 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Ignore PostCSS nodes returned by `addVariant` ([#8608](https://github.com/tailwindlabs/tailwindcss/pull/8608))
- Fix missing spaces around arithmetic operators ([#8615](https://github.com/tailwindlabs/tailwindcss/pull/8615))
- Detect alpha value in CSS `theme()` function when using quotes ([#8625](https://github.com/tailwindlabs/tailwindcss/pull/8625))
- Fix "Maximum call stack size exceeded" bug ([#8636](https://github.com/tailwindlabs/tailwindcss/pull/8636))

## [3.1.2] - 2022-06-10

Expand Down
2 changes: 1 addition & 1 deletion src/lib/defaultExtractor.js
View file
Open in desktop
Expand Up @@ -12,7 +12,7 @@ export function defaultExtractor(context) {
let results = []

for (let pattern of patterns) {
results.push(...(content.match(pattern) ?? []))
results = [...results, ...(content.match(pattern) ?? [])]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really know the context here, but if performance is at all relevant, then you might consider doing something like this instead:

for (let pattern of patterns) {
  for (let result of (content.match(pattern) || [])) {
    results.push(result);
  }
}

(didn't test it or anything, so apologies if that isn't quite right, but the point is that there's no need to iterate through results on every iteration of patterns; same principle as in this piece: https://betterprogramming.pub/the-reduce-spread-anti-pattern-fc0c1c0b23f6)

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked into this a bit and there's very little perf difference (in a plain program at least) when given a super large content array. There's a slight difference in heap allocations but it's tiny. The perf difference is on the order of ± 0.1ms for 1000 iterations.

Though, my testing method could be a bit flawed or V8 may have hyper optimized my test case. Either way I think it's not a big deal one way or the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. I'd be surprised if there's really no significant difference, since pushing only the new items each time is O(n), whereas also iterating through all previously added items each time is O(n^2-ish). But yeah, could be that they've optimized it. Cheers.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is because array's in V8 are likely tail-allocated. This means doing something like result = [...result, ...anything] makes only a constant number of allocations independent of the number of items in result. Only the number of items in anything matters. If you flip the arrays around (result = [...anything, ...result]) it can't reuse existing memory/storage and must copy memory around. If you try this it results in times that are almost 4x slower.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you must be right. Good info!

}

return results.filter((v) => v !== undefined).map(clipAtBalancedParens)
Expand Down
6 changes: 6 additions & 0 deletions tests/default-extractor.test.js
View file
Open in desktop
Expand Up @@ -476,3 +476,9 @@ test('multi-word + arbitrary values + quotes', async () => {

expect(extractions).toContain(`grid-cols-['repeat(2)']`)
})

test('a lot of data', () => {
let extractions = defaultExtractor('underline '.repeat(2 ** 17))

expect(extractions).toContain(`underline`)
})