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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

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
Copy link
Collaborator

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2553/pipeline

@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
addaleax
addaleax approved these changes Feb 9, 2019
View reviewed changes
src/node.cc Show resolved Hide resolved
bnoordhuis
bnoordhuis reviewed Feb 9, 2019
View reviewed changes
src/node.cc Show resolved Hide resolved
src/node_report.cc Outdated Show resolved Hide resolved
@joyeecheung
src: unify uptime base used across the code base
f6912ca 
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
@joyeecheung
src: unify uptime base used across the code base
fd0a861 
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
@joyeecheung @addaleax
src: unify uptime base used across the code base
4f0b77c 
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.

@joyeecheung joyeecheung mentioned this pull request Feb 19, 2019
Closed
3 tasks
Trott pushed a commit to joyeecheung/node that referenced this pull request Feb 19, 2019
@joyeecheung @Trott
process: fix calculation in process.uptime()
bd09f7f 
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
@joyeecheung @Trott
process: fix calculation in process.uptime()
a32c574 
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
@joyeecheung @Trott
process: move test-process-uptime to parallel
5c9b37b 
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
@joyeecheung @addaleax
process: fix calculation in process.uptime()
14b8eb5 
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
@joyeecheung @addaleax
process: move test-process-uptime to parallel
356923f 
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
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@joyeecheung @rvagg
src: unify uptime base used across the code base
82df851 
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
@joyeecheung @rvagg
process: fix calculation in process.uptime()
fde4011 
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
@joyeecheung @rvagg
process: move test-process-uptime to parallel
cd10e25 
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>
This was referenced Jul 24, 2019
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@bnoordhuis bnoordhuis bnoordhuis left review comments

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joyeecheung @nodejs-github-bot @Trott @bnoordhuis @addaleax @targos