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

lib,src: support values > 4GB in heap statistics #10186

Merged
merged 1 commit into from
Dec 11, 2016

Conversation

bnoordhuis
Copy link
Member

Fixes: #10185

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. lts-watch-v6.x labels Dec 8, 2016
@bnoordhuis
Copy link
Member Author

Forgot to mention, I didn't add a test because I don't think I can make it reliable. It only works on 64 bits architectures and even there preallocating too much address space might lead to a run-in with the OOM killer.

Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were transporting the heap statistics as uint32 values to JS land but
those wrap around for values > 4 GB.  Use 64 bits floats instead, those
should last us a while.

Fixes: nodejs#10185
PR-URL: nodejs#10186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@bnoordhuis
Copy link
Member Author

Landed in aa77b76, thanks for reviewing everyone.

@bnoordhuis bnoordhuis merged commit aa77b76 into nodejs:master Dec 11, 2016
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
We were transporting the heap statistics as uint32 values to JS land but
those wrap around for values > 4 GB.  Use 64 bits floats instead, those
should last us a while.

Fixes: nodejs#10185
PR-URL: nodejs#10186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
We were transporting the heap statistics as uint32 values to JS land but
those wrap around for values > 4 GB.  Use 64 bits floats instead, those
should last us a while.

Fixes: nodejs#10185
PR-URL: nodejs#10186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
We were transporting the heap statistics as uint32 values to JS land but
those wrap around for values > 4 GB.  Use 64 bits floats instead, those
should last us a while.

Fixes: nodejs#10185
PR-URL: nodejs#10186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@tamasmahr
Copy link

Hi,

This is a crucial fix for us. When is this going to make it into an LTS release?

Thank you!

Tamas

@addaleax
Copy link
Member

@tamasmahr Unfortunately, this first has to live in a v7.x release for two weeks before it will be included in an LTS release, so it might still take a bit. :/

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
We were transporting the heap statistics as uint32 values to JS land but
those wrap around for values > 4 GB.  Use 64 bits floats instead, those
should last us a while.

Fixes: nodejs#10185
PR-URL: nodejs#10186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
@Somebi
Copy link

Somebi commented Feb 15, 2017

It seems this is still not implemented or half-implemented/tested. Record Allocation Timeline inside inspector can't be displayed properly because if this or something related to this fix.

@bnoordhuis
Copy link
Member Author

@Somebi #11401 is not related to #10185.

MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
We were transporting the heap statistics as uint32 values to JS land but
those wrap around for values > 4 GB.  Use 64 bits floats instead, those
should last us a while.

Fixes: #10185
PR-URL: #10186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@MylesBorins
Copy link
Contributor

Just landed this in v4 and v6 staging. Expect an RC with this included v. soon

MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
We were transporting the heap statistics as uint32 values to JS land but
those wrap around for values > 4 GB.  Use 64 bits floats instead, those
should last us a while.

Fixes: #10185
PR-URL: #10186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@Somebi
Copy link

Somebi commented Mar 7, 2017

Thank you. I have noticed, that CPU profiling is not working as well. I these issues are related or i should open new issue?

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
We were transporting the heap statistics as uint32 values to JS land but
those wrap around for values > 4 GB.  Use 64 bits floats instead, those
should last us a while.

Fixes: #10185
PR-URL: #10186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
We were transporting the heap statistics as uint32 values to JS land but
those wrap around for values > 4 GB.  Use 64 bits floats instead, those
should last us a while.

Fixes: #10185
PR-URL: #10186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins added a commit that referenced this pull request Mar 21, 2017
Notable changes

* performance: The performance of several APIs has been improved.
  - `Buffer.compare()` is up to 35% faster on average. (Brian White)
    #10927
  - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
    #10895
  - `fs.*statSync()` functions are now up to 9.3% faster on average.
    (Brian White) #11522
  - `os.loadavg` is up to 151% faster. (Brian White)
    #11516
  - `process.memoryUsage()` is up to 34% faster. (Brian White)
    #11497
  - `querystring.unescape()` for `Buffer`s is 15% faster on average.
    (Brian White) #10837
  - `querystring.stringify()` is up to 7.8% faster on average.
    (Brian White) #10852
  - `querystring.parse()` is up to 21% faster on average. (Brian White)
    #10874

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* child_process:
  - `spawnSync` now returns a null `status` when child is terminated by
    a signal. (cjihrig) #11288
  - This fixes the behavior to act like `spawn()` does.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923
  - Debug messages have been added for cases when headers contain
    invalid values. (Evan Lucas)
    #9195

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186

* timers:
  - Timer callbacks now always maintain order when interacting with
    domain error handling. (John Barboza)
    #10522

PR-URL: #11759
MylesBorins added a commit that referenced this pull request Mar 21, 2017
Notable Changes:

* buffer:
  - The performance of `.toJSON()` is now up to 2859% faster on average
    (Brian White) #10895

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186
MylesBorins added a commit that referenced this pull request Mar 21, 2017
Notable Changes:

* buffer:
  - The performance of `.toJSON()` is now up to 2859% faster on average
    (Brian White) #10895

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186

PR-URL: #11760
MylesBorins added a commit that referenced this pull request Mar 21, 2017
Notable changes

* performance: The performance of several APIs has been improved.
  - `Buffer.compare()` is up to 35% faster on average. (Brian White)
    #10927
  - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
    #10895
  - `fs.*statSync()` functions are now up to 9.3% faster on average.
    (Brian White) #11522
  - `os.loadavg` is up to 151% faster. (Brian White)
    #11516
  - `process.memoryUsage()` is up to 34% faster. (Brian White)
    #11497
  - `querystring.unescape()` for `Buffer`s is 15% faster on average.
    (Brian White) #10837
  - `querystring.stringify()` is up to 7.8% faster on average.
    (Brian White) #10852
  - `querystring.parse()` is up to 21% faster on average. (Brian White)
    #10874

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* child_process:
  - `spawnSync` now returns a null `status` when child is terminated by
    a signal. (cjihrig) #11288
  - This fixes the behavior to act like `spawn()` does.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923
  - Debug messages have been added for cases when headers contain
    invalid values. (Evan Lucas)
    #9195

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186

* timers:
  - Timer callbacks now always maintain order when interacting with
    domain error handling. (John Barboza)
    #10522

PR-URL: #11759
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    Notable Changes:

    * buffer:
      - The performance of `.toJSON()` is now up to 2859% faster on average
        (Brian White) nodejs/node#10895

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    Notable changes

    * performance: The performance of several APIs has been improved.
      - `Buffer.compare()` is up to 35% faster on average. (Brian White)
        nodejs/node#10927
      - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
        nodejs/node#10895
      - `fs.*statSync()` functions are now up to 9.3% faster on average.
        (Brian White) nodejs/node#11522
      - `os.loadavg` is up to 151% faster. (Brian White)
        nodejs/node#11516
      - `process.memoryUsage()` is up to 34% faster. (Brian White)
        nodejs/node#11497
      - `querystring.unescape()` for `Buffer`s is 15% faster on average.
        (Brian White) nodejs/node#10837
      - `querystring.stringify()` is up to 7.8% faster on average.
        (Brian White) nodejs/node#10852
      - `querystring.parse()` is up to 21% faster on average. (Brian White)
        nodejs/node#10874

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * child_process:
      - `spawnSync` now returns a null `status` when child is terminated by
        a signal. (cjihrig) nodejs/node#11288
      - This fixes the behavior to act like `spawn()` does.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923
      - Debug messages have been added for cases when headers contain
        invalid values. (Evan Lucas)
        nodejs/node#9195

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

    * timers:
      - Timer callbacks now always maintain order when interacting with
        domain error handling. (John Barboza)
        nodejs/node#10522

    PR-URL: nodejs/node#11759

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    Notable Changes:

    * buffer:
      - The performance of `.toJSON()` is now up to 2859% faster on average
        (Brian White) nodejs/node#10895

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    Notable changes

    * performance: The performance of several APIs has been improved.
      - `Buffer.compare()` is up to 35% faster on average. (Brian White)
        nodejs/node#10927
      - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
        nodejs/node#10895
      - `fs.*statSync()` functions are now up to 9.3% faster on average.
        (Brian White) nodejs/node#11522
      - `os.loadavg` is up to 151% faster. (Brian White)
        nodejs/node#11516
      - `process.memoryUsage()` is up to 34% faster. (Brian White)
        nodejs/node#11497
      - `querystring.unescape()` for `Buffer`s is 15% faster on average.
        (Brian White) nodejs/node#10837
      - `querystring.stringify()` is up to 7.8% faster on average.
        (Brian White) nodejs/node#10852
      - `querystring.parse()` is up to 21% faster on average. (Brian White)
        nodejs/node#10874

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * child_process:
      - `spawnSync` now returns a null `status` when child is terminated by
        a signal. (cjihrig) nodejs/node#11288
      - This fixes the behavior to act like `spawn()` does.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923
      - Debug messages have been added for cases when headers contain
        invalid values. (Evan Lucas)
        nodejs/node#9195

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

    * timers:
      - Timer callbacks now always maintain order when interacting with
        domain error handling. (John Barboza)
        nodejs/node#10522

    PR-URL: nodejs/node#11759

Signed-off-by: Ilkka Myller <[email protected]>
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++. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants