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

process: improve process.hrtime #10764

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@joyeecheung
Member

joyeecheung commented Jan 12, 2017

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
Affected core subsystem(s)

process

This PR is split into two commits in caution of breaking change, can be squashed when identified not breaking. Non breaking:

  • Add benchmarks for diffing a previous result
  • Improvements to the documentation, including type annotation
  • Update the oudated comments in src/node.cc
  • Comment process.hrtime and improve readability

Might be breaking: check the length of argument passed into process.hrtime(). Passing a user-defined array is previously described as undefined behavior in the documentation.

Benchmark results:

                                               improvement significant   p.value
 process/bench-hrtime.js type="diff" n=1000000      0.28 %             0.8029355
 process/bench-hrtime.js type="raw" n=1000000      -0.56 %             0.6488252
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 12, 2017

Member

Unfortunately because of the change in error condition this has to be labeled as semver-major.

Member

jasnell commented Jan 12, 2017

Unfortunately because of the change in error condition this has to be labeled as semver-major.

Show outdated Hide outdated lib/internal/process.js
const nsec = hrValues[2] - ar[1];
return [nsec < 0 ? sec - 1 : sec, nsec < 0 ? nsec + 1e9 : nsec];
if (time !== undefined) {
if (Array.isArray(time) && time.length == 2) {

This comment has been minimized.

@addaleax

addaleax Jan 12, 2017

Member

nit: === for comparison

@addaleax

addaleax Jan 12, 2017

Member

nit: === for comparison

This comment has been minimized.

@joyeecheung

joyeecheung Jan 13, 2017

Member

Addressed.

@joyeecheung

joyeecheung Jan 13, 2017

Member

Addressed.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Jan 12, 2017

Member

Unfortunately because of the change in error condition this has to be labeled as semver-major.

I don’t think it has to be. As @joyeecheung mentioned in the PR description, this only adds a throw for situations that we explicitly declared to be undefined behavior in the docs. It’s probably best to consider it breaking, unless there’s a good reason not to do that.

Member

addaleax commented Jan 12, 2017

Unfortunately because of the change in error condition this has to be labeled as semver-major.

I don’t think it has to be. As @joyeecheung mentioned in the PR description, this only adds a throw for situations that we explicitly declared to be undefined behavior in the docs. It’s probably best to consider it breaking, unless there’s a good reason not to do that.

Show outdated Hide outdated benchmark/process/bench-hrtime.js
const n = conf.n >>> 0;
const n = conf.n | 0;
const hrtime = process.hrtime;
var noDead = hrtime(), i;

This comment has been minimized.

@mscdex

mscdex Jan 12, 2017

Contributor

Can you please make these separate var statements?

@mscdex

mscdex Jan 12, 2017

Contributor

Can you please make these separate var statements?

This comment has been minimized.

@joyeecheung

joyeecheung Jan 13, 2017

Member

Addressed

@joyeecheung

joyeecheung Jan 13, 2017

Member

Addressed

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Jan 14, 2017

Member

@jasnell If you still think it is semver-major, I am OK with landing this as semver-major.

Member

joyeecheung commented Jan 14, 2017

@jasnell If you still think it is semver-major, I am OK with landing this as semver-major.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 15, 2017

Member

I'm more comfortable landing it as semver-major

Member

jasnell commented Jan 15, 2017

I'm more comfortable landing it as semver-major

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Jan 15, 2017

Member

CI: https://ci.nodejs.org/job/node-test-commit/7257/ (Started this without seeing that one had already been kicked off. Sorry!)
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/525/

Member

addaleax commented Jan 15, 2017

CI: https://ci.nodejs.org/job/node-test-commit/7257/ (Started this without seeing that one had already been kicked off. Sorry!)
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/525/

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Jan 17, 2017

Member

Going to land this if there is nothing else to be addressed. @mscdex Does this LGTY?

Member

joyeecheung commented Jan 17, 2017

Going to land this if there is nothing else to be addressed. @mscdex Does this LGTY?

process.hrtime([]);
}, /^TypeError: process.hrtime\(\) only accepts an Array tuple$/);
assert.throws(() => {
process.hrtime([1]);

This comment has been minimized.

@mscdex

mscdex Jan 17, 2017

Contributor

How about adding another test that has > 2 elements?

@mscdex

mscdex Jan 17, 2017

Contributor

How about adding another test that has > 2 elements?

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jan 17, 2017

Contributor

One nit, but I agree with @jasnell that I would lean more towards semver-major to be on the safe side. Otherwise LGTM.

Contributor

mscdex commented Jan 17, 2017

One nit, but I agree with @jasnell that I would lean more towards semver-major to be on the safe side. Otherwise LGTM.

process: improve process.hrtime
* Add benchmarks for diffing a previous result
* Improvements to the documentation, including type annotation
* Update the outdated comments in src/node.cc, improve comments
  in lib/internal/process.js
* Check the argument is an Array Tuple with length 2
@joyeecheung

This comment has been minimized.

Show comment
Hide comment

joyeecheung added a commit that referenced this pull request Jan 23, 2017

process: improve process.hrtime
* Add benchmarks for diffing a previous result
* Improvements to the documentation, including type annotation
* Update the outdated comments in src/node.cc, improve comments
  in lib/internal/process.js
* Check the argument is an Array Tuple with length 2

PR-URL: #10764
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Jan 23, 2017

Member

Landed in a647d82

Member

joyeecheung commented Jan 23, 2017

Landed in a647d82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment