src: extract AllocatedBuffer from env.h #33291

jasnell · 2020-05-07T20:34:04Z

Cleanup up env.h by removing things that are not
specific to Environment.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2020-05-07T20:38:58Z

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

addaleax

LGTM but I feel like it’s worth pointing out that AllocatedBuffers are tied to an Environment instance, and that with V8 8.3, the need for this is probably going to go away anyway.

src/env.cc

addaleax · 2020-05-14T17:06:49Z

Fwiw, if this lands before #33381 then #33381 would likely include a revert of this.

jasnell · 2020-05-14T17:11:49Z

Couldn't we just combine the two set of changes?

nodejs-github-bot · 2020-05-14T17:13:41Z

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

(btw, repeated ci's here are because there's a build error happening in a couple of CI environments that I'm not able to recreate locally)

addaleax · 2020-05-14T17:24:26Z

I mean, yeah, if you prefer. But ultimately the goal is for AllocatedBuffer to go away anyway.

jasnell · 2020-05-14T17:26:09Z

But ultimately the goal is for AllocatedBuffer to go away anyway.

Yep, that I know. It's partly why I wanted to separate it out from env.h in the first place.

jasnell · 2020-05-14T19:49:26Z

@addaleax... ok, I've pulled in the changes from #33381 here. Still blocked on #33321

addaleax

Still LGTM 👍

nodejs-github-bot · 2020-05-16T10:16:57Z

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

BridgeAR · 2020-05-23T17:33:23Z

Ping @jasnell

nodejs-github-bot · 2020-05-28T13:23:54Z

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

nodejs-github-bot · 2020-05-29T22:20:44Z

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

nodejs-github-bot · 2020-05-30T01:09:52Z

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

Cleanup up env.h by removing things that are not specific to `Environment`.

Alternative to nodejs#33381 that reimplements that change on top of moving AllocatedBuffer out of env.h

nodejs-github-bot · 2020-05-30T18:01:17Z

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

Cleanup up env.h by removing things that are not specific to `Environment`. PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

Alternative to #33381 that reimplements that change on top of moving AllocatedBuffer out of env.h PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

jasnell · 2020-05-30T23:22:42Z

Landed in 56ff1ee, e2cd715, 830ef81

Cleanup up env.h by removing things that are not specific to `Environment`. PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

Alternative to #33381 that reimplements that change on top of moving AllocatedBuffer out of env.h PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

Cleanup up env.h by removing things that are not specific to `Environment`. PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

Alternative to #33381 that reimplements that change on top of moving AllocatedBuffer out of env.h PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

codebytere · 2020-07-08T21:22:47Z

@jasnell should this go to v12.x? It looks like it'll need a manual backport if yes but if no just remove the label :)

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. labels May 7, 2020

jasnell requested a review from addaleax May 7, 2020 20:34

addaleax approved these changes May 7, 2020

View reviewed changes

src/env.cc Outdated Show resolved Hide resolved

devnexen approved these changes May 8, 2020

View reviewed changes

jasnell force-pushed the extract-allocated-buffer branch from 77ff8ef to 2fc6b65 Compare May 14, 2020 15:37

This comment has been minimized.

Sign in to view

jasnell force-pushed the extract-allocated-buffer branch from 5a0bd99 to 9fa9f61 Compare May 14, 2020 16:42

This comment has been minimized.

Sign in to view

jasnell force-pushed the extract-allocated-buffer branch from 9fa9f61 to 72b6542 Compare May 14, 2020 16:50

This comment has been minimized.

Sign in to view

jasnell force-pushed the extract-allocated-buffer branch from 9388529 to c971497 Compare May 14, 2020 19:10

jasnell added the blocked PRs that are blocked by other issues or PRs. label May 14, 2020

jasnell requested review from addaleax and devnexen May 14, 2020 19:53

addaleax approved these changes May 14, 2020

View reviewed changes

devnexen approved these changes May 14, 2020

View reviewed changes

addaleax mentioned this pull request May 16, 2020

src: turn AllocatedBuffer into thin wrapper around v8::BackingStore #33381

Closed

2 tasks

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels May 16, 2020

BridgeAR added the wip Issues and PRs that are still a work in progress. label May 23, 2020

BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels May 30, 2020

jasnell force-pushed the extract-allocated-buffer branch from 07f8173 to e368fc4 Compare May 30, 2020 17:55

jasnell added 3 commits May 30, 2020 10:58

src: extract AllocatedBuffer from env.h

1c61aba

Cleanup up env.h by removing things that are not specific to `Environment`.

src: turn AllocatedBuffer into thin wrapper around v8::BackingStore

9f8dc2f

Alternative to nodejs#33381 that reimplements that change on top of moving AllocatedBuffer out of env.h

src: remove superfluous inline keywords

3c49e03

jasnell force-pushed the extract-allocated-buffer branch from e368fc4 to 3c49e03 Compare May 30, 2020 17:59

jasnell added a commit that referenced this pull request May 30, 2020

src: remove superfluous inline keywords

830ef81

PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

jasnell closed this May 30, 2020

codebytere pushed a commit that referenced this pull request Jun 18, 2020

src: remove superfluous inline keywords

7725ff3

PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

codebytere mentioned this pull request Jun 28, 2020

v14.5.0 proposal #34093

Merged

addaleax mentioned this pull request Jun 30, 2020

SEGFAULT on Node Fermium #34126

Closed

codebytere pushed a commit that referenced this pull request Jun 30, 2020

src: remove superfluous inline keywords

0bd3c0a

PR-URL: #33291 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com>

codebytere added the backport-requested-v12.x label Jul 8, 2020

codebytere mentioned this pull request Jul 10, 2020

build: fix node.gyp config #33685

Closed

XadillaX mentioned this pull request Apr 21, 2021

Buffer.from(str) memory leak #38300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: extract AllocatedBuffer from env.h #33291

src: extract AllocatedBuffer from env.h #33291

jasnell commented May 7, 2020

nodejs-github-bot commented May 7, 2020

addaleax left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

addaleax commented May 14, 2020

jasnell commented May 14, 2020

nodejs-github-bot commented May 14, 2020 •

edited by jasnell

Loading

addaleax commented May 14, 2020

jasnell commented May 14, 2020

jasnell commented May 14, 2020

addaleax left a comment

nodejs-github-bot commented May 16, 2020

BridgeAR commented May 23, 2020

nodejs-github-bot commented May 28, 2020

nodejs-github-bot commented May 29, 2020

nodejs-github-bot commented May 30, 2020 •

edited by BridgeAR

Loading

nodejs-github-bot commented May 30, 2020

jasnell commented May 30, 2020

codebytere commented Jul 8, 2020

src: extract AllocatedBuffer from env.h #33291

src: extract AllocatedBuffer from env.h #33291

Conversation

jasnell commented May 7, 2020

Checklist

nodejs-github-bot commented May 7, 2020

addaleax left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

addaleax commented May 14, 2020

jasnell commented May 14, 2020

nodejs-github-bot commented May 14, 2020 • edited by jasnell Loading

addaleax commented May 14, 2020

jasnell commented May 14, 2020

jasnell commented May 14, 2020

addaleax left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 16, 2020

BridgeAR commented May 23, 2020

nodejs-github-bot commented May 28, 2020

nodejs-github-bot commented May 29, 2020

nodejs-github-bot commented May 30, 2020 • edited by BridgeAR Loading

nodejs-github-bot commented May 30, 2020

jasnell commented May 30, 2020

codebytere commented Jul 8, 2020

nodejs-github-bot commented May 14, 2020 •

edited by jasnell

Loading

nodejs-github-bot commented May 30, 2020 •

edited by BridgeAR

Loading