src: fix memory leak for v8.serialize #42695

liuxingbaoyu · 2022-04-11T14:50:06Z

When Buffer::New passes in existing data, it cannot be garbage collected in js synchronous execution. Fixes: nodejs#40828 Refs: nodejs#38300

addaleax

#40828 (comment) is the right comment here here: This is currently not working because the garbage collector does not get a chance to run, including in the test case here.

This change here is not a good solution. It adds an unnecessary buffer copy without real benefit.

If you do want to fix this in a better way, you’ll need to modify Buffer::New(Environment*,char*,size_t) to not call into Buffer::New(Environment*,char*,size_t,FreeCallback,void); instead.

The best way to fix this is to keep reminding users that Buffer GC may need to perform some cleanup asynchronously, and Buffer instances may be kept alive until synchronous code stops running.

liuxingbaoyu · 2022-04-13T10:21:34Z

@addaleax

Thanks for your review!

This change here is not a good solution. It adds an unnecessary buffer copy without real benefit.

Although an extra memory copy is added, the test becomes faster, perhaps due to the reduced use of CleanupHook.

And it takes less memory and is more stable.

In some cases, such as babel, it can reduce memory by 300~500mb.

If you do want to fix this in a better way, you’ll need to modify Buffer::New(Environment*,char*,size_t) to not call into Buffer::New(Environment*,char*,size_t,FreeCallback,void); instead.

Can you give me more help? I'd love to use this better method!

I have tried modifying the CallbackInfo to release the buffer synchronously, but that requires a lock on the CleanupHook.

And using CleanupHook also seems to have an impact on performance.

The best way to fix this is to keep reminding users that Buffer GC may need to perform some cleanup asynchronously, and Buffer instances may be kept alive until synchronous code stops running.

But this problem leads to bad performance and even crashes, and there is no good way to avoid it other than changing everything to async.

addaleax · 2022-04-13T10:43:29Z

Can you give me more help? I'd love to use this better method!

The goal here would be to avoid CallbackInfo/CleanupHook entirely, and instead let V8 handle the allocation release. Passing a free callback to V8 is more efficient than passing one to the 5-argument Buffer::New() version, because the V8 callback makes fewer API guarantees (for example, it can run on any thread and can be called as part of GC; on the other hand, the Buffer::New() callback guarantees that JS can be executed from inside the callback, for legacy compatibility reasons).

So – the idea here is, instead of using Buffer::New(Environment*, char*, size_t, FreeCallback, void*) inside of Buffer::New(Environment*, char*, size_t):

Creating a v8::BackingStore with a release callback using v8::ArrayBuffer::NewBackingStore(char*, size_t, DeleterCallback, void*)
Creating an v8::ArrayBuffer from it using v8::ArrayBuffer::New(Isolate*, std::shared_ptr<BackingStore>)
Create a Node.js buffer from that using Buffer::New(Environment*, Local<ArrayBuffer>, size_t, size_t)

The really nice thing about this approach would be that it takes care of this problem in all situations in which Buffer::New(Environment*, char*, size_t) is used, including addons which use it.

Although an extra memory copy is added, the test becomes faster, perhaps due to the reduced use of CleanupHook.

I assume that this would depend a bit on how much memory is allocated – longer buffers take longer to copy, but (typically) a constant amount of time to release.

In some cases, such as babel, it can reduce memory by 300~500mb.

Nice! To be clear, I would absolutely love to see this :)

liuxingbaoyu · 2022-04-13T10:51:09Z

Thank you very much for your detailed help, I will try it!

addaleax

Awesome, thank you!

nodejs-github-bot · 2022-04-13T12:00:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43471/

liuxingbaoyu · 2022-04-13T12:45:32Z

It seems that the test is so strict that it fails on jenkins, I changed it from less than 10mb increments to less than double.

nodejs-github-bot · 2022-04-15T14:35:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43516/

nodejs-github-bot · 2022-04-26T23:55:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43717/

nodejs-github-bot · 2022-04-28T18:38:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43747/

nodejs-github-bot · 2022-05-02T17:49:47Z

Commit Queue failed

- Loading data for nodejs/node/pull/42695 ✔ Done loading data for nodejs/node/pull/42695 ----------------------------------- PR info ------------------------------------ Title src: fix memory leak for v8.serialize (#42695) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch liuxingbaoyu:fix-buffer-callback -> nodejs:master Labels c++, author ready Commits 3 - src: fix memory leak for v8.serialize - use a better way - fix test Committers 1 - liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com> PR-URL: https://github.com/nodejs/node/pull/42695 Fixes: https://github.com/nodejs/node/issues/40828 Refs: https://github.com/nodejs/node/issues/38300 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Minwoo Jung ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/42695 Fixes: https://github.com/nodejs/node/issues/40828 Refs: https://github.com/nodejs/node/issues/38300 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Minwoo Jung -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 11 Apr 2022 14:50:06 GMT ✔ Approvals: 3 ✔ - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/42695#pullrequestreview-940855691 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/42695#pullrequestreview-941134072 ✔ - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/42695#pullrequestreview-941762487 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-04-28T18:38:01Z: https://ci.nodejs.org/job/node-test-pull-request/43747/ - Querying data for job/node-test-pull-request/43747/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 42695 From https://github.com/nodejs/node * branch refs/pull/42695/merge -> FETCH_HEAD ✔ Fetched commits as 73a8a0ee3ceb..74a59561b032 -------------------------------------------------------------------------------- [master ece4132a09] src: fix memory leak for v8.serialize Author: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com> Date: Tue Apr 12 02:16:57 2022 +0800 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-v8-serialize-leak.js [master 840fa5c1df] use a better way Author: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com> Date: Wed Apr 13 19:37:11 2022 +0800 2 files changed, 15 insertions(+), 4 deletions(-) [master f5d4840fed] fix test Author: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com> Date: Wed Apr 13 20:45:13 2022 +0800 1 file changed, 2 insertions(+), 2 deletions(-) ✔ Patches applied There are 3 commits in the PR. Attempting autorebase. Rebasing (2/6)

Executing: git node land --amend --yes
⚠ Found Fixes: #40828, skipping..
⚠ Found Refs: #38300, skipping..
--------------------------------- New Message ----------------------------------
src: fix memory leak for v8.serialize

When Buffer::New passes in existing data,
it cannot be garbage collected in js synchronous execution.

Fixes: #40828
Refs: #38300

PR-URL: #42695
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

[detached HEAD 59541eb482] src: fix memory leak for v8.serialize
Author: liuxingbaoyu 30521560+liuxingbaoyu@users.noreply.github.com
Date: Tue Apr 12 02:16:57 2022 +0800
2 files changed, 24 insertions(+), 1 deletion(-)
create mode 100644 test/parallel/test-v8-serialize-leak.js
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
use a better way

PR-URL: #42695
Fixes: #40828
Refs: #38300
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

[detached HEAD 86bd2acfe9] use a better way
Author: liuxingbaoyu 30521560+liuxingbaoyu@users.noreply.github.com
Date: Wed Apr 13 19:37:11 2022 +0800
2 files changed, 15 insertions(+), 4 deletions(-)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix test

PR-URL: #42695
Fixes: #40828
Refs: #38300
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

[detached HEAD 0bbde95aef] fix test
Author: liuxingbaoyu 30521560+liuxingbaoyu@users.noreply.github.com
Date: Wed Apr 13 20:45:13 2022 +0800
1 file changed, 2 insertions(+), 2 deletions(-)

Successfully rebased and updated refs/heads/master.

ℹ Use --fixupAll option, squash the PR manually or land the PR from the command line.

https://github.com/nodejs/node/actions/runs/2259461302

nodejs-github-bot · 2022-05-02T19:45:49Z

Landed in ce29d28

When Buffer::New passes in existing data, it cannot be garbage collected in js synchronous execution. Fixes: #40828 Refs: #38300 PR-URL: #42695 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

nodejs-github-bot added c++ needs-ci labels Apr 11, 2022

src: fix memory leak for v8.serialize

809386a

When Buffer::New passes in existing data, it cannot be garbage collected in js synchronous execution. Fixes: nodejs#40828 Refs: nodejs#38300

liuxingbaoyu force-pushed the fix-buffer-callback branch from 24c5304 to 809386a Compare Apr 11, 2022

addaleax requested changes Apr 12, 2022

View changes

use a better way

806687b

addaleax approved these changes Apr 13, 2022

View changes

addaleax added request-ci and removed needs-ci labels Apr 13, 2022

github-actions bot removed the request-ci label Apr 13, 2022

fix test

74a5956

jasnell approved these changes Apr 13, 2022

View changes

github-actions bot mentioned this pull request Apr 14, 2022

CI Reliability 2022-04-14 nodejs/reliability#251

Open

24 tasks

JungMinu approved these changes Apr 14, 2022

View changes

github-actions bot mentioned this pull request Apr 15, 2022

CI Reliability 2022-04-15 nodejs/reliability#252

Open

16 tasks

addaleax added author ready request-ci labels Apr 15, 2022

github-actions bot removed the request-ci label Apr 15, 2022

github-actions bot mentioned this pull request Apr 16, 2022

CI Reliability 2022-04-16 nodejs/reliability#253

Open

16 tasks

github-actions bot mentioned this pull request Apr 17, 2022

CI Reliability 2022-04-17 nodejs/reliability#254

Open

15 tasks

github-actions bot mentioned this pull request Apr 18, 2022

CI Reliability 2022-04-18 nodejs/reliability#255

Open

14 tasks

github-actions bot mentioned this pull request Apr 19, 2022

CI Reliability 2022-04-19 nodejs/reliability#256

Open

16 tasks

github-actions bot mentioned this pull request Apr 20, 2022

CI Reliability 2022-04-20 nodejs/reliability#257

Open

16 tasks

github-actions bot mentioned this pull request Apr 21, 2022

CI Reliability 2022-04-21 nodejs/reliability#258

Open

22 tasks

github-actions bot mentioned this pull request Apr 28, 2022

CI Reliability 2022-04-28 nodejs/reliability#265

Open

18 tasks

github-actions bot mentioned this pull request Apr 29, 2022

CI Reliability 2022-04-29 nodejs/reliability#266

Open

21 tasks

github-actions bot mentioned this pull request Apr 30, 2022

CI Reliability 2022-04-30 nodejs/reliability#267

Open

27 tasks

github-actions bot mentioned this pull request May 1, 2022

CI Reliability 2022-05-01 nodejs/reliability#268

Open

29 tasks

github-actions bot mentioned this pull request May 2, 2022

CI Reliability 2022-05-02 nodejs/reliability#269

Open

30 tasks

aduh95 added the commit-queue label May 2, 2022

nodejs-github-bot added commit-queue-failed and removed commit-queue labels May 2, 2022

aduh95 added commit-queue commit-queue-squash and removed commit-queue-failed labels May 2, 2022

nodejs-github-bot removed the commit-queue label May 2, 2022

nodejs-github-bot merged commit ce29d28 into nodejs:master May 2, 2022
70 checks passed

liuxingbaoyu mentioned this pull request May 7, 2022

TextDecoder is slow. #39879

Open

RafaelGSS mentioned this pull request May 10, 2022

v18.2.0 proposal #43036

Merged

nodejs / node Public

src: fix memory leak for v8.serialize #42695

src: fix memory leak for v8.serialize #42695

liuxingbaoyu commented Apr 11, 2022

addaleax left a comment

liuxingbaoyu commented Apr 13, 2022 •

edited

addaleax commented Apr 13, 2022

liuxingbaoyu commented Apr 13, 2022

addaleax left a comment

nodejs-github-bot commented Apr 13, 2022

liuxingbaoyu commented Apr 13, 2022

nodejs-github-bot commented Apr 15, 2022

nodejs-github-bot commented Apr 26, 2022

nodejs-github-bot commented Apr 28, 2022

nodejs-github-bot commented May 2, 2022

PR-URL: #42695
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

PR-URL: #42695
Fixes: #40828
Refs: #38300
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

PR-URL: #42695
Fixes: #40828
Refs: #38300
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

Successfully rebased and updated refs/heads/master.

nodejs-github-bot commented May 2, 2022

Apr	MAY	Jun
	17
2021	2022	2023

nodejs / node Public

src: fix memory leak for v8.serialize #42695

src: fix memory leak for v8.serialize #42695

Conversation

liuxingbaoyu commented Apr 11, 2022

addaleax left a comment

liuxingbaoyu commented Apr 13, 2022 • edited

addaleax commented Apr 13, 2022

liuxingbaoyu commented Apr 13, 2022

addaleax left a comment

nodejs-github-bot commented Apr 13, 2022

liuxingbaoyu commented Apr 13, 2022

nodejs-github-bot commented Apr 15, 2022

nodejs-github-bot commented Apr 26, 2022

nodejs-github-bot commented Apr 28, 2022

nodejs-github-bot commented May 2, 2022

PR-URL: #42695 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Minwoo Jung nodecorelab@gmail.com

PR-URL: #42695 Fixes: #40828 Refs: #38300 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Minwoo Jung nodecorelab@gmail.com

PR-URL: #42695 Fixes: #40828 Refs: #38300 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Minwoo Jung nodecorelab@gmail.com

Successfully rebased and updated refs/heads/master.

nodejs-github-bot commented May 2, 2022

liuxingbaoyu commented Apr 13, 2022 •

edited

PR-URL: #42695
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

PR-URL: #42695
Fixes: #40828
Refs: #38300
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

PR-URL: #42695
Fixes: #40828
Refs: #38300
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com