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

src: unify uptime base used across the code base #26016

Closed
wants to merge 1 commit into from

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 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
src/node.cc Show resolved Hide resolved
src/node.cc Show resolved Hide resolved
src/node_report.cc Outdated Show resolved Hide resolved
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 <anna@addaleax.net>
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 <anna@addaleax.net>
@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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@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 <anna@addaleax.net>
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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