fix: "Map maximum size exceeded" in deepClone#17142
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/58787 |
| try { | ||
| return deepClone(value, new Map(), true); | ||
| } catch (_) { | ||
| return structuredClone(value); |
There was a problem hiding this comment.
This only works in node v17+, but v8.serialize doesn't work in browsers and other runtimes.
There was a problem hiding this comment.
Can we use structuredClone in Babel 8?
There was a problem hiding this comment.
#16967 (comment)
Unfortunately it is slow, less than half the performance of current implementations.🤦♂️
| function deepClone( | ||
| value: any, | ||
| cache: Map<any, any>, | ||
| allowCircle: boolean, |
There was a problem hiding this comment.
The allowCircle is false but the object actually contains a cycle, this function is now going to cause an infinite loop, right? Can we throw in that case instead? We'd only need to keep track of the current "stack", rather than of all nodes in the map.
| try { | ||
| return deepClone(value, new Map(), true); | ||
| } catch (_) { | ||
| return structuredClone(value); |
There was a problem hiding this comment.
Can we use structuredClone in Babel 8?
| circleSet.clear(); | ||
| throw new Error("Babel-deepClone: Cycles are not allowed in AST"); | ||
| } | ||
| circleSet.add(value); |
There was a problem hiding this comment.
How big is the performance impact of using this set compared to:
- the old behavior with the map
- the behavior with no protection against infinite loops
If it's significantly expensive, we could keep track of the depth we are at in the cloning and only start using the cycle detection logic if it's, for example, >100. It means that we'll be slower in error cases, but faster in the (common) successful case.
There was a problem hiding this comment.
current: 263 ops/sec ±1.13% (3.809ms)
circle-check: 255 ops/sec ±0.9% (3.924ms)
no-circle-check: 357 ops/sec ±1.55% (2.799ms)
circle-check-100-depth: 355 ops/sec ±0.7% (2.814ms)
I'll add depth detection. :)
| if (cache.has(value)) return cache.get(value); | ||
| if (allowCircle) { | ||
| if (cache.has(value)) return cache.get(value); | ||
| } else if (++depth > 100) { |
There was a problem hiding this comment.
I was trying to see what are reasonable depth values: both @babel/standalone/babel.js and @babel/standalone/babel.min.js are around 200.
Let's increase this to 200 at least, maybe 250 to be safe? It's unlikely that there are files deeper than that.
(Or maybe we could also just completely remove the cycle-detection logic and just throw an error if depth > 1000 😬)
There was a problem hiding this comment.
Thanks for your investigation!
I'd be inclined to set it to 250, since there are probably a few hundred variables being concatenated(auto-generated code) :)
Changes since the last version: babel/babel#17142 babel/babel#17127 babel/babel#17052
Changes since the last version: babel/babel#17142 babel/babel#17127 babel/babel#17052
Changes since the last version: babel/babel#17142 babel/babel#17127 babel/babel#17052
In Babel 7, only a workaround was applied, and in Babel 8, a complete fix was made.