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.

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

Fix "Maximum call stack size exceeded" bug #8636

Merged
merged 4 commits into from
Jun 14, 2022
Merged

Conversation

adamwathan
Copy link
Member

@adamwathan adamwathan commented Jun 14, 2022

For performance reasons we already read the content line by line, this allows us to cache individual lines if we already saw them. This also allows us to do the necessary work on relatively "small" content chunks because not a lot of developers use very long lines in the source files.

However...

Some people either misconfigure the content paths to include everything (even node_modules) or in a more realistic scenario link to a 3rd party dependency that uses Tailwind inside the node_modules folder. Those files are typically minified, all on one line. The stack size issue occurs when we try to push a lot of matches into an array. We used result.push(...x) which is limited by the length of x because functions have a maximum amount of parameters they can receive.

Thanks to @nam-tran-tego for the wonderful reproduction and fix for this!

Fixes: #8582

@RobinMalfait RobinMalfait changed the title Fix potential call stack size issue Fix "Maximum call stack size exceeded" bug Jun 14, 2022
@RobinMalfait RobinMalfait merged commit 22eaad1 into master Jun 14, 2022
@RobinMalfait RobinMalfait deleted the adamwathan-patch-1 branch June 14, 2022 12:17
@@ -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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maximum call stack size exceeded
4 participants