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

Memory leak due to increasing RetainedMaps size in V8 (Fixed upstream) #57412

Closed
tunamagur0 opened this issue Mar 11, 2025 · 2 comments · Fixed by #57437
Closed

Memory leak due to increasing RetainedMaps size in V8 (Fixed upstream) #57412

tunamagur0 opened this issue Mar 11, 2025 · 2 comments · Fixed by #57437

Comments

@tunamagur0
Copy link
Contributor

Version

v22.14.0

Platform

Linux 5.15.167.4-microsoft-standard-WSL2 #1 SMP Tue Nov 5 00:21:55 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

v8

What steps will reproduce the bug?

  1. Run the below script with --expose-gc.
  2. Wait for some time.
  3. Check the automatically generated heap snapshots.
  4. Inspect the following path in the snapshots:
    (system) > system / NativeContext > retained_maps :: system / WeakArrayList
  5. Observe that its size keeps increasing over time.

Note:
The increase is gradual, so it may take several hours to see a significant difference.
However, even within a shorter period, a steady increase should still be observable.

const v8 = require('v8');
const ws = new WeakSet();

for (let i = 0; i < 1e10; i++) {
  function A() {
    this.a = 'a';
    this.b = 'b';
    this.c = 'c';
  }
  function B() {
    this.a = 'a';
    this.b = 'b';
    this.c = 'c';
  }
  function C() {
    this.a = 'a';
    this.b = 'b';
    this.c = 'c';
  }
  function D() {
    this.a = 'a';
    this.b = 'b';
    this.c = 'c';
  }
  function E() {
    this.a = 'a';
    this.b = 'b';
    this.c = 'c';
  }

  const a = new A();
  const b = new B();
  const c = new C();
  const d = new D();
  const e = new E();
  ws.add(a);
  ws.add(b);
  ws.add(c);
  ws.add(d);
  ws.add(e);
  gc();

  if (i % 10000 === 0) {
    v8.writeHeapSnapshot();
  }
}

How often does it reproduce? Is there a required condition?

It reproduces consistently.

What is the expected behavior? Why is that the expected behavior?

RetainedMaps size should remain stable over time.

What do you see instead?

  • The size of RetainedMaps (visible in heap snapshots under (system) > system / NativeContext > retained_maps :: system / WeakArrayList) keeps increasing over time.
  • This increase is gradual but persistent, leading to higher memory consumption over extended runtimes.
    Eventually, this could result in performance degradation or excessive memory usage.

Below are screenshots showing the memory usage:
v22.14.0

at startup after ~9 hours
Image Image

v22.14.0 (with the patch applied)

at startup after ~5 hours
Image Image

Additional information

@juanarbol
Copy link
Member

That's amazing! Could you please send us your patch?

This https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-V8.md may be handy

@tunamagur0
Copy link
Contributor Author

@juanarbol
I'm not entirely sure about the process, but I've created a pull request. Could you please take a look at it when you have a moment?

@aduh95 aduh95 closed this as completed in dab3f4b Mar 16, 2025
aduh95 pushed a commit that referenced this issue Mar 16, 2025
Original commit message:

    Compact retained maps array more often

    When we add maps to the retained maps array, we compacted the array if
    it's full. But, since we are now adding maps in a batch, it's unlikely
    to meet the condition. Thus, update the condition to check whether new
    size exceeds the capacity.

    Bug: 398528460
    Change-Id: I89caa47b69532c6397596edfe5caf7c7d24768cc
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6330019
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Choongwoo Han <choongwoo.han@microsoft.com>
    Cr-Commit-Position: refs/heads/main@{#99163}

Refs: v8/v8@c172ffc
PR-URL: #57437
Fixes: #57412
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Co-Authored-By: tunamagur0
  <47546832+tunamagur0@users.noreply.github.com>
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 a pull request may close this issue.

2 participants