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

deps: cherry-pick 1ef7487b from v8 upstream #6218

Closed
wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Apr 15, 2016

  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

deps/v8

Description of change

Original commit message:

We'd like this in 6.x to help with diagnosing customer problems.
It provides a better message on OOM so that it is easier to
be able to tell whether the OOM was due to heap exhaustion
or running out of native memory. Is already landed in google
master.

 Improved diagnostic message for JS heap out of memory

 This patch replaces the unused 'take_snapshot' parameter on
 FatalProcessOutOfMemory() with a 'is_heap_oom' parameter.
 The parameter is set to true on error paths where the
 JS heap is out of memory, as distinct from a malloc()
 failure i.e. process out of memory.  The message output to
 stderr or passed to embedding applications via FatalErrorCallback
 is 'Javascript heap out of memory' rather than
 'process out of memory'.

 BUG=

 R=jochen@chromium.org, verwaest@chromium.org, michael_dawson@ca.ibm.com

 Review URL: https://codereview.chromium.org/1873443002

 Cr-Commit-Position: refs/heads/master@{#35431}

@mhdawson
Copy link
Member Author

Runs completed before PR sumitted:

@bnoordhuis
Copy link
Member

bnoordhuis commented Apr 15, 2016

You have a typo in the commit hash, it should be v8/v8@1ef7487b (EDIT: that's 1ef7487b). LGTM apart from that.

@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Apr 15, 2016
@jasnell
Copy link
Member

jasnell commented Apr 15, 2016

LGTM

Original commit message:

     Improved diagnostic message for JS heap out of memory

     This patch replaces the unused 'take_snapshot' parameter on
     FatalProcessOutOfMemory() with a 'is_heap_oom' parameter.
     The parameter is set to true on error paths where the
     JS heap is out of memory, as distinct from a malloc()
     failure i.e. process out of memory.  The message output to
     stderr or passed to embedding applications via FatalErrorCallback
     is 'Javascript heap out of memory' rather than
     'process out of memory'.

     BUG=

     R=jochen@chromium.org, verwaest@chromium.org, michael_dawson@ca.ibm.com

     Review URL: https://codereview.chromium.org/1873443002

     Cr-Commit-Position: refs/heads/master@{nodejs#35431}

We'd like this in 6.x to help with diagnosing customer problems.
It provides a better message on OOM so that it is easier to
be able to tell whether the OOM was due to heap exhaustion
or running out of native memory.
@mhdawson mhdawson changed the title deps: cherry-pick 1ef74871 from v8 upstream deps: cherry-pick 1ef7487b from v8 upstream Apr 18, 2016
@mhdawson
Copy link
Member Author

mhdawson commented Apr 18, 2016

Fixed hash, final CI run before landing:
https://ci.nodejs.org/job/node-test-pull-request/2299/

@mhdawson mhdawson self-assigned this Apr 18, 2016
@mhdawson
Copy link
Member Author

CI all green except for 2 arm failures that look like timeout related issues which I believe are unrelated to the change being made (arm seems to have regular timeout failures in different tests). Will land now.

mhdawson added a commit that referenced this pull request Apr 18, 2016
Original commit message:

     Improved diagnostic message for JS heap out of memory

     This patch replaces the unused 'take_snapshot' parameter on
     FatalProcessOutOfMemory() with a 'is_heap_oom' parameter.
     The parameter is set to true on error paths where the
     JS heap is out of memory, as distinct from a malloc()
     failure i.e. process out of memory.  The message output to
     stderr or passed to embedding applications via FatalErrorCallback
     is 'Javascript heap out of memory' rather than
     'process out of memory'.

     BUG=

     R=jochen@chromium.org, verwaest@chromium.org, michael_dawson@ca.ibm.com

     Review URL: https://codereview.chromium.org/1873443002

     Cr-Commit-Position: refs/heads/master@{#35431}

We'd like this in 6.x to help with diagnosing customer problems.
It provides a better message on OOM so that it is easier to
be able to tell whether the OOM was due to heap exhaustion
or running out of native memory.

PR-URL: #6218
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mhdawson
Copy link
Member Author

Landed as af4a380

@MylesBorins
Copy link
Contributor

adding don't land for v4 and v5 assuming this is specific to v8 5

@mhdawson please remove flag and let me know if this is wrong

@mhdawson
Copy link
Member Author

I could possibly be back ported, but I think leaving it as a value in in 6.x is fine.

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
Original commit message:

     Improved diagnostic message for JS heap out of memory

     This patch replaces the unused 'take_snapshot' parameter on
     FatalProcessOutOfMemory() with a 'is_heap_oom' parameter.
     The parameter is set to true on error paths where the
     JS heap is out of memory, as distinct from a malloc()
     failure i.e. process out of memory.  The message output to
     stderr or passed to embedding applications via FatalErrorCallback
     is 'Javascript heap out of memory' rather than
     'process out of memory'.

     BUG=

     R=jochen@chromium.org, verwaest@chromium.org, michael_dawson@ca.ibm.com

     Review URL: https://codereview.chromium.org/1873443002

     Cr-Commit-Position: refs/heads/master@{nodejs#35431}

We'd like this in 6.x to help with diagnosing customer problems.
It provides a better message on OOM so that it is easier to
be able to tell whether the OOM was due to heap exhaustion
or running out of native memory.

PR-URL: nodejs#6218
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
Original commit message:

     Improved diagnostic message for JS heap out of memory

     This patch replaces the unused 'take_snapshot' parameter on
     FatalProcessOutOfMemory() with a 'is_heap_oom' parameter.
     The parameter is set to true on error paths where the
     JS heap is out of memory, as distinct from a malloc()
     failure i.e. process out of memory.  The message output to
     stderr or passed to embedding applications via FatalErrorCallback
     is 'Javascript heap out of memory' rather than
     'process out of memory'.

     BUG=

     R=jochen@chromium.org, verwaest@chromium.org, michael_dawson@ca.ibm.com

     Review URL: https://codereview.chromium.org/1873443002

     Cr-Commit-Position: refs/heads/master@{#35431}

We'd like this in 6.x to help with diagnosing customer problems.
It provides a better message on OOM so that it is easier to
be able to tell whether the OOM was due to heap exhaustion
or running out of native memory.

PR-URL: #6218
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Jun 3, 2016
Original commit message:

     Improved diagnostic message for JS heap out of memory

     This patch replaces the unused 'take_snapshot' parameter on
     FatalProcessOutOfMemory() with a 'is_heap_oom' parameter.
     The parameter is set to true on error paths where the
     JS heap is out of memory, as distinct from a malloc()
     failure i.e. process out of memory.  The message output to
     stderr or passed to embedding applications via FatalErrorCallback
     is 'Javascript heap out of memory' rather than
     'process out of memory'.

     BUG=

     R=jochen@chromium.org, verwaest@chromium.org, michael_dawson@ca.ibm.com

     Review URL: https://codereview.chromium.org/1873443002

     Cr-Commit-Position: refs/heads/master@{nodejs#35431}

We'd like this in 6.x to help with diagnosing customer problems.
It provides a better message on OOM so that it is easier to
be able to tell whether the OOM was due to heap exhaustion
or running out of native memory.

PR-URL: nodejs#6218
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Jun 29, 2016
Original commit message:

     Improved diagnostic message for JS heap out of memory

     This patch replaces the unused 'take_snapshot' parameter on
     FatalProcessOutOfMemory() with a 'is_heap_oom' parameter.
     The parameter is set to true on error paths where the
     JS heap is out of memory, as distinct from a malloc()
     failure i.e. process out of memory.  The message output to
     stderr or passed to embedding applications via FatalErrorCallback
     is 'Javascript heap out of memory' rather than
     'process out of memory'.

     BUG=

     R=jochen@chromium.org, verwaest@chromium.org, michael_dawson@ca.ibm.com

     Review URL: https://codereview.chromium.org/1873443002

     Cr-Commit-Position: refs/heads/master@{nodejs#35431}

We'd like this in 6.x to help with diagnosing customer problems.
It provides a better message on OOM so that it is easier to
be able to tell whether the OOM was due to heap exhaustion
or running out of native memory.

PR-URL: nodejs#6218
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
Original commit message:

     Improved diagnostic message for JS heap out of memory

     This patch replaces the unused 'take_snapshot' parameter on
     FatalProcessOutOfMemory() with a 'is_heap_oom' parameter.
     The parameter is set to true on error paths where the
     JS heap is out of memory, as distinct from a malloc()
     failure i.e. process out of memory.  The message output to
     stderr or passed to embedding applications via FatalErrorCallback
     is 'Javascript heap out of memory' rather than
     'process out of memory'.

     BUG=

     R=jochen@chromium.org, verwaest@chromium.org, michael_dawson@ca.ibm.com

     Review URL: https://codereview.chromium.org/1873443002

     Cr-Commit-Position: refs/heads/master@{nodejs#35431}

We'd like this in 6.x to help with diagnosing customer problems.
It provides a better message on OOM so that it is easier to
be able to tell whether the OOM was due to heap exhaustion
or running out of native memory.

PR-URL: nodejs#6218
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mhdawson mhdawson deleted the ras branch March 15, 2017 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants