Skip to content

Conversation

@joyeecheung
Copy link
Member

This patch joins per_process::prog_start_time (a double)
and performance::performance_node_start (a uint64_t) into a
per_process::node_start_time (a uint64_t) which gets initialized
in node::Start().

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot 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 Feb 9, 2019
This patch joins `per_process::prog_start_time` (a double)
and `performance::performance_node_start` (a uint64_t) into a
`per_process::node_start_time` (a uint64_t) which gets initialized
in `node::Start()`.
@joyeecheung
Copy link
Member Author

Rebased and switched to constexpr: https://ci.nodejs.org/job/node-test-pull-request/20828/

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 18, 2019
@joyeecheung
Copy link
Member Author

Landed in fd0a861

joyeecheung added a commit that referenced this pull request Feb 18, 2019
This patch joins `per_process::prog_start_time` (a double)
and `performance::performance_node_start` (a uint64_t) into a
`per_process::node_start_time` (a uint64_t) which gets initialized
in `node::Start()`.

PR-URL: #26016
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 18, 2019
This patch joins `per_process::prog_start_time` (a double)
and `performance::performance_node_start` (a uint64_t) into a
`per_process::node_start_time` (a uint64_t) which gets initialized
in `node::Start()`.

PR-URL: #26016
Reviewed-By: Anna Henningsen <[email protected]>
@Trott
Copy link
Member

Trott commented Feb 19, 2019

This changed the behavior of process.uptime() in a semver-major way. The result is that pummel/test-process-uptime now fails. #26205

I'm guessing there's a a quick fix rather than requiring a revert? I'm guessing it's a matter of "oops, we had been reporting milliseconds and now we're reporting nanoseconds" or something like that.

Trott pushed a commit to joyeecheung/node that referenced this pull request Feb 19, 2019
In nodejs#26016 the result returned
by process.uptime() was mistakenly set to be based in the wrong
unit. This patch fixes the calculation and makes sure the returned
value is in seconds.

Refs: nodejs#26016
Trott pushed a commit to Trott/io.js that referenced this pull request Feb 19, 2019
In nodejs#26016 the result returned
by process.uptime() was mistakenly set to be based in the wrong
unit. This patch fixes the calculation and makes sure the returned
value is in seconds.

Refs: nodejs#26016

PR-URL: nodejs#26206
Fixes: nodejs#26205
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request Feb 19, 2019
In addition, do not make too many assumptions about the startup
time and timer latency in test-process-uptime. Instead only test
that the value is likely in the correct unit (seconds) and it should
be increasing in subsequent calls.

PR-URL: nodejs#26206
Fixes: nodejs#26205
Refs: nodejs#26016
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
In #26016 the result returned
by process.uptime() was mistakenly set to be based in the wrong
unit. This patch fixes the calculation and makes sure the returned
value is in seconds.

Refs: #26016

PR-URL: #26206
Fixes: #26205
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
In addition, do not make too many assumptions about the startup
time and timer latency in test-process-uptime. Instead only test
that the value is likely in the correct unit (seconds) and it should
be increasing in subsequent calls.

PR-URL: #26206
Fixes: #26205
Refs: #26016
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
This patch joins `per_process::prog_start_time` (a double)
and `performance::performance_node_start` (a uint64_t) into a
`per_process::node_start_time` (a uint64_t) which gets initialized
in `node::Start()`.

PR-URL: #26016
Reviewed-By: Anna Henningsen <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
In #26016 the result returned
by process.uptime() was mistakenly set to be based in the wrong
unit. This patch fixes the calculation and makes sure the returned
value is in seconds.

Refs: #26016

PR-URL: #26206
Fixes: #26205
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
In addition, do not make too many assumptions about the startup
time and timer latency in test-process-uptime. Instead only test
that the value is likely in the correct unit (seconds) and it should
be increasing in subsequent calls.

PR-URL: #26206
Fixes: #26205
Refs: #26016
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants