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: Memory leak when deep cloning in babel-core #14583

Merged
merged 7 commits into from Jun 20, 2022

Conversation

liuxingbaoyu
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu commented May 25, 2022

Q                       A
Fixed Issues? Fixes #13970
Patch: Bug Fix?
Major: Breaking Change? ×
Minor: New Feature? ×
Tests Added + Pass? ×
Documentation PR Link
Any Dependency Changes? add lodash.clonedeep
License MIT

Although a new dependency is added, this avoids high memory usage and in some cases greatly improves performance.

@babel-bot
Copy link
Collaborator

babel-bot commented May 25, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52203/

@@ -62,6 +62,7 @@
"debug": "^4.1.0",
"gensync": "^1.0.0-beta.2",
"json5": "^2.2.1",
"lodash.clonedeep": "^4.5.0",
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 25, 2022

Choose a reason for hiding this comment

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

Could we use

"clone-deep": "^4.0.1",
instead? Lo dash is annoying because it always has security bugs that don't affect us but that scare our users.

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu May 25, 2022

Choose a reason for hiding this comment

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

Oh no. It doesn't seem to support circular references.
jonschlinkert/clone-deep#23

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 25, 2022

Choose a reason for hiding this comment

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

Well, the old JSON-based solution doesn't either. It's used to clone an AST, so it's expected to not have cycles.

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu May 25, 2022

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu May 26, 2022

Choose a reason for hiding this comment

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

Theoretically yes, but I haven't tried it.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 26, 2022

Choose a reason for hiding this comment

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

We don't need something as advanced as structuredClone, a simpler argoritm for trees is enough.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 26, 2022

Choose a reason for hiding this comment

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

Uhm, the clone-deep package is simple enough that we can just copy it's code removing the isPlainObject check.

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu May 26, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 26, 2022

Choose a reason for hiding this comment

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

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;
}

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 pkg: core labels May 25, 2022
nicolo-ribaudo
nicolo-ribaudo previously approved these changes May 25, 2022
JLHwung
JLHwung previously approved these changes May 26, 2022
@nicolo-ribaudo nicolo-ribaudo dismissed their stale review May 26, 2022

(dropping my review since deep-clone doesn't work for us)

return value;
}

export default function <T = types.Node>(value: T): T {
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jun 1, 2022

Choose a reason for hiding this comment

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

This doesn't seem to be necessary:

Suggested change
export default function <T = types.Node>(value: T): T {
export default function <T>(value: T): T {

@nicolo-ribaudo nicolo-ribaudo dismissed JLHwung’s stale review Jun 1, 2022

@JLHwung I dismissed your review because this PR has been almost completely rewritten since your approval

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 {
Copy link
Contributor

@JLHwung JLHwung Jun 10, 2022

Choose a reason for hiding this comment

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

Suggested change
function deepClone(value: any, cache: Map<any, any>): any {
function deepClone<T extends object>(value: T, cache: Map<T, T>): T

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jun 10, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu Jun 11, 2022

Choose a reason for hiding this comment

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

Yes, this is an internal implementation that is not exported.

@liuxingbaoyu
Copy link
Contributor Author

liuxingbaoyu commented Jun 11, 2022

I did a rebase to resolve the conflict.

@JLHwung JLHwung merged commit e482c76 into babel:main Jun 20, 2022
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: core PR: Bug Fix 🐛
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants