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

Backport 5786 to v4.x (memory growth with 'vm' module) #6871

Merged
merged 5 commits into from
May 20, 2016

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented May 19, 2016

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

@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
@mscdex mscdex added v4.x vm Issues and PRs related to the vm subsystem. labels May 19, 2016
@bnoordhuis
Copy link
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.

@MylesBorins
Copy link
Contributor

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]>
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]>
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
Copy link
Member Author

rvagg commented May 20, 2016

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?

@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
Copy link
Member Author

rvagg commented May 20, 2016

landed in v4.x-staging

MylesBorins pushed a commit that referenced this pull request May 20, 2016
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
Copy link
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

@MylesBorins MylesBorins mentioned this pull request May 20, 2016
MylesBorins pushed a commit that referenced this pull request May 23, 2016
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
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
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
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
Development

Successfully merging this pull request may close these issues.

6 participants