Skip to content

fix: Memory leak when deep cloning in babel-core#14583

Merged
JLHwung merged 7 commits intobabel:mainfrom
liuxingbaoyu:fix-choneDeep-leak
Jun 20, 2022
Merged

fix: Memory leak when deep cloning in babel-core#14583
JLHwung merged 7 commits intobabel:mainfrom
liuxingbaoyu:fix-choneDeep-leak

Conversation

@liuxingbaoyu
Copy link
Copy Markdown
Member

@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
Copy Markdown
Collaborator

babel-bot commented May 25, 2022

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

"debug": "^4.1.0",
"gensync": "^1.0.0-beta.2",
"json5": "^2.2.1",
"lodash.clonedeep": "^4.5.0",
Copy link
Copy Markdown
Member

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
Copy Markdown
Member Author

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
Copy Markdown
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
Copy Markdown
Member Author

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.

Copy link
Copy Markdown

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
Copy Markdown
Member Author

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
Copy Markdown
Member

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
Copy Markdown
Member

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
Copy Markdown
Member Author

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
Copy Markdown
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 🐛 A type of pull request used for our changelog categories 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 08:55

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

return value;
}

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

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 June 1, 2022 13:07

@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
Copy Markdown
Contributor

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
Copy Markdown
Member

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
Copy Markdown
Member Author

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
Copy Markdown
Member Author

I did a rebase to resolve the conflict.

@JLHwung JLHwung merged commit e482c76 into babel:main Jun 20, 2022
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Sep 20, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: core PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: transformFromAstSync leaks memory

5 participants