fix: Memory leak when deep cloning in babel-core#14583
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52203/ |
packages/babel-core/package.json
Outdated
| "debug": "^4.1.0", | ||
| "gensync": "^1.0.0-beta.2", | ||
| "json5": "^2.2.1", | ||
| "lodash.clonedeep": "^4.5.0", |
There was a problem hiding this comment.
Could we use
instead? Lo dash is annoying because it always has security bugs that don't affect us but that scare our users.There was a problem hiding this comment.
Oh no. It doesn't seem to support circular references.
jonschlinkert/clone-deep#23
There was a problem hiding this comment.
Well, the old JSON-based solution doesn't either. It's used to clone an AST, so it's expected to not have cycles.
There was a problem hiding this comment.
You are right!
However I suspect clone-deep will corrupt the same reference.
lodash/lodash#2725
Also regarding the failure in ci, clone-deep only works on PlainObject, our ast object returns false in isPlainObject, so nothing changes.
There was a problem hiding this comment.
Would it be possible for babel itself to use the structuredClone polyfill from core-js in versions of Node.js without native support (it's a recent addition)?
There was a problem hiding this comment.
Theoretically yes, but I haven't tried it.
There was a problem hiding this comment.
We don't need something as advanced as structuredClone, a simpler argoritm for trees is enough.
There was a problem hiding this comment.
Uhm, the clone-deep package is simple enough that we can just copy it's code removing the isPlainObject check.
There was a problem hiding this comment.
v8=require("v8")
a={}
b={x:a,y:a}
c=v8.deserialize(v8.serialize(a))
console.log(c.x==c.y,c.x==b.x)
true false
Our comments need to keep the same object reference.
There was a problem hiding this comment.
Good catch, comments. Something like this? Our AST is a really simple structure, we don't need a complex cloning function
function deepClone(value, cache = new WeakMap) {
if (typeof value === "object" && value !== null) {
if (cache.has(value)) return cache.get(value);
let cloned;
if (Array.isArray(value)) {
cloned = Array.from(value, elem => deepClone(elem, cache));
} else {
cloned = {};
Object.keys(value).forEach(key => cloned[key] = deepClone(value[key], cache));
}
cache.set(value, cloned);
return cloned;
}
return value;
}(dropping my review since deep-clone doesn't work for us)
f3d49e0 to
405c399
Compare
| return value; | ||
| } | ||
|
|
||
| export default function <T = types.Node>(value: T): T { |
There was a problem hiding this comment.
This doesn't seem to be necessary:
| export default function <T = types.Node>(value: T): T { | |
| export default function <T>(value: T): T { |
@JLHwung I dismissed your review because this PR has been almost completely rewritten since your approval
3fe03ba to
745293f
Compare
| if (v8.deserialize && v8.serialize) { | ||
| return v8.deserialize(v8.serialize(value)); | ||
| //https://github.com/babel/babel/pull/14583#discussion_r882828856 | ||
| function deepClone(value: any, cache: Map<any, any>): any { |
There was a problem hiding this comment.
| function deepClone(value: any, cache: Map<any, any>): any { | |
| function deepClone<T extends object>(value: T, cache: Map<T, T>): T |
There was a problem hiding this comment.
The problem is that T must be the union of all the types of the nested objects of value (since the cache Map potentially contains all of them), so "this function returns T" isn't much useful.
There was a problem hiding this comment.
Yes, this is an internal implementation that is not exported.
745293f to
9de45fc
Compare
|
I did a rebase to resolve the conflict. |
lodash.clonedeepAlthough a new dependency is added, this avoids high memory usage and in some cases greatly improves performance.