GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Backport 5786 to v4.x (memory growth with 'vm' module) by rvagg · Pull Request #6871 · nodejs/node

Conversation

@rvagg

@rvagg rvagg commented May 19, 2016

Copy link
Copy Markdown
Member

Original issue: #3113

Backports #5786 & #5786 together to v4.x-staging. This combination was tested by users with a v4.x build @ #3113 (comment), we said we'd backport it properly but were waiting for a solid proving time in v5.x because of some problems we had (the bug that #5786 fixed).

Backported to 5.x in #5800 and was released in v5.9.1 / 2016-03-23 with no additional problems beyond #5786. IMO this is ready to roll into the next v4.x.

/ @nodejs/lts

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
indieisaconcept reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 19, 2016
@rvagg rvagg mentioned this pull request May 19, 2016
VM: Extreme memory growth #3113
Closed
@mscdex mscdex added v4.x vm Issues and PRs related to the vm subsystem. labels May 19, 2016
@bnoordhuis

bnoordhuis commented May 19, 2016

Copy link
Copy Markdown
Member

LGTM but can you fix up the Reviewed-By tags? Also, the tags are missing in the last commit; the original (a53b2ac) has them.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis bnoordhuis mentioned this pull request May 19, 2016
test,tools: limit lint tolerance of gc global #6324
Closed
2 tasks done
@MylesBorins

MylesBorins commented May 19, 2016

Copy link
Copy Markdown
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/2708/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ofrobots added 5 commits May 20, 2016 12:30
@ofrobots @rvagg
contextify: cleanup weak ref for global proxy
faa94ff 
Cleanup how node_contextify keeps weak references in order to prepare
for new style phantom weakness API. We didn't need to keep a weak
reference to the context's global proxy, as the context holds it.

PR-URL: nodejs#5392
Reviewed-By: Ben Noordhuis <[email protected]>
@ofrobots @rvagg
contextify: cleanup weak ref for sandbox
3d56671 
Simplify how node_contextify was keeping a weak reference to the
sandbox object in order to prepare for new style phantom weakness V8
API. It is simpler (and more robust) for the context to hold a
reference to the sandbox in an embedder data field. Doing otherwise
meant that the sandbox could become weak while the context was still
alive. This wasn't a problem because we would make the reference
strong at that point.

Since the sandbox must live at least as long as the context, it
would be better for the context to hold onto the sandbox.

PR-URL: nodejs#5392
Reviewed-By: Ben Noordhuis <[email protected]>
@ofrobots @rvagg
contextify: replace deprecated SetWeak usage
ab00cf5 
PR-URL: nodejs#5392
Reviewed-By: Ben Noordhuis <[email protected]>
@ofrobots @rvagg
contextify: cache sandbox and context in locals
4d48400 
PR-URL: nodejs#5392
Reviewed-By: Ben Noordhuis <[email protected]>
@ofrobots @rvagg
contextify: tie lifetimes of context & sandbox
c6db822 
When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

This is a backport of nodejs#5786 to v5.x.

Ref: nodejs#5786
PR-URL: nodejs#5800
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@rvagg
rvagg force-pushed the 5392-backport branch from 9c31bbb to c6db822 Compare May 20, 2016 04:44
@rvagg

rvagg commented May 20, 2016

Copy link
Copy Markdown
Member Author

Thanks @bnoordhuis, all fixed now.

CI is all green except for test-debug-port-cluster on FreeBSD which is apparently fixed on master by #6769 which I have flagged for lts-watch.

@thealphanerd what's the chance of this getting in to the next v4.x? Are we running too close to make it in?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rvagg
rvagg merged commit c6db822 into nodejs:v4.x-staging May 20, 2016
@rvagg
rvagg deleted the 5392-backport branch May 20, 2016 05:44
@rvagg

rvagg commented May 20, 2016

Copy link
Copy Markdown
Member Author

landed in v4.x-staging
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins pushed a commit that referenced this pull request May 20, 2016
Myles Borins
2016-05-20, Version 4.4.5 'Argon' (LTS)
5986356 
Notable changes:

* **buffer**:
  * Buffer no longer errors if you call lastIndexOf with a search term
    longer than the buffer (Anna Henningsen)
    #6511

* contextify:
  * Context objects are now properly garbage collected, this solves a
    problem some individuals were experiencing with extreme memory
    growth (Ali Ijaz Sheikh)
    #6871

* deps:
  * update npm to 2.15.5 (Rebecca Turner)
    #6663

* http:
  * Invalid status codes can no longer be sent. Limited to 3 digit
    numbers between 100 - 999 (Brian White)
    #6291
@MylesBorins

MylesBorins commented May 20, 2016

Copy link
Copy Markdown
Contributor

@rvagg considering the amount of testing these changes received I see no problem with including it in the next release. I've added them to the proposal branch and am just running tests / cutting a new RC right now
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins mentioned this pull request May 20, 2016
v4.4.5 proposal #6824
Merged
MylesBorins pushed a commit that referenced this pull request May 23, 2016
Myles Borins
2016-05-23, Version 4.4.5 'Argon' (LTS)
c6d033c 
Notable changes:

* **buffer**:
  * Buffer no longer errors if you call lastIndexOf with a search term
    longer than the buffer (Anna Henningsen)
    #6511

* contextify:
  * Context objects are now properly garbage collected, this solves a
    problem some individuals were experiencing with extreme memory
    growth (Ali Ijaz Sheikh)
    #6871

* deps:
  * update npm to 2.15.5 (Rebecca Turner)
    #6663

* http:
  * Invalid status codes can no longer be sent. Limited to 3 digit
    numbers between 100 - 999 (Brian White)
    #6291
MylesBorins pushed a commit that referenced this pull request May 24, 2016
Myles Borins
2016-05-24, Version 4.4.5 'Argon' (LTS)
6330f48 
Notable changes:

* **buffer**:
  * Buffer no longer errors if you call lastIndexOf with a search term
    longer than the buffer (Anna Henningsen)
    #6511

* contextify:
  * Context objects are now properly garbage collected, this solves a
    problem some individuals were experiencing with extreme memory
    growth (Ali Ijaz Sheikh)
    #6871

* deps:
  * update npm to 2.15.5 (Rebecca Turner)
    #6663

* http:
  * Invalid status codes can no longer be sent. Limited to 3 digit
    numbers between 100 - 999 (Brian White)
    #6291
MylesBorins pushed a commit that referenced this pull request May 24, 2016
Myles Borins
2016-05-24, Version 4.4.5 'Argon' (LTS)
e03c326 
Notable changes:

* **buffer**:
  * Buffer no longer errors if you call lastIndexOf with a search term
    longer than the buffer (Anna Henningsen)
    #6511

* contextify:
  * Context objects are now properly garbage collected, this solves a
    problem some individuals were experiencing with extreme memory
    growth (Ali Ijaz Sheikh)
    #6871

* deps:
  * update npm to 2.15.5 (Rebecca Turner)
    #6663

* http:
  * Invalid status codes can no longer be sent. Limited to 3 digit
    numbers between 100 - 999 (Brian White)
    #6291
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@rvagg @bnoordhuis @MylesBorins @mscdex @nodejs-github-bot @ofrobots
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.