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

[v12.x] backport Buffer cleanup changes #31355

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Backport #30475 and the remaining commit from #30551.

All conflicts were neighbouring-line conflicts.

More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: nodejs#30339 (comment)

PR-URL: nodejs#30475
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Invoke the free callback for a given `Buffer` if it was created
with one, and mark the underlying `ArrayBuffer` as detached.

This makes sure that the memory is released e.g. when addons inside
Workers create such `Buffer`s.

PR-URL: nodejs#30551
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Jan 14, 2020
@targos
Copy link
Member

targos commented Jan 14, 2020

Landed on v12.x-staging, thanks!

@targos targos closed this Jan 14, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: #30339 (comment)

PR-URL: #30475
Backport-PR-URL: #31355
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
Invoke the free callback for a given `Buffer` if it was created
with one, and mark the underlying `ArrayBuffer` as detached.

This makes sure that the memory is released e.g. when addons inside
Workers create such `Buffer`s.

PR-URL: #30551
Backport-PR-URL: #31355
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: #30339 (comment)

PR-URL: #30475
Backport-PR-URL: #31355
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
Invoke the free callback for a given `Buffer` if it was created
with one, and mark the underlying `ArrayBuffer` as detached.

This makes sure that the memory is released e.g. when addons inside
Workers create such `Buffer`s.

PR-URL: #30551
Backport-PR-URL: #31355
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@addaleax addaleax deleted the backport-30551 branch January 14, 2020 17:50
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: #30339 (comment)

PR-URL: #30475
Backport-PR-URL: #31355
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Invoke the free callback for a given `Buffer` if it was created
with one, and mark the underlying `ArrayBuffer` as detached.

This makes sure that the memory is released e.g. when addons inside
Workers create such `Buffer`s.

PR-URL: #30551
Backport-PR-URL: #31355
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants