-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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: implement process.hrtime.bigint() #21256
Conversation
84cd2cd
to
f7ba146
Compare
message: 'The "time" argument must be of type bigint. Received type object' | ||
}); | ||
|
||
// The default behavior, return an Array "tuple" of numbers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, copy-paste error..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it still a perf boost to use the ArrayBuffer when it's just one value?
@devsnek I don't think it makes much difference, probably can save an allocation depending on the situation though. |
doc/api/process.md
Outdated
|
||
setTimeout(() => { | ||
const diff = process.hrtime.bigint(time); | ||
// 1552n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this supposed to be 1000000552n?
doc/api/process.md
Outdated
|
||
If `time` is specified, it must be the result of a previous | ||
`process.hrtime.bigint()` call. The returned result will be the difference | ||
between the current time and the specified time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While useful for the array-based method, I don’t think this feature is that useful now one can just subtract the times directly…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyGu Yeah come to think of it, it does seem unnecessary. I'll remove it.
src/node.cc
Outdated
void HrtimeBigInt(const FunctionCallbackInfo<Value>& args) { | ||
Local<ArrayBuffer> ab = args[0].As<BigUint64Array>()->Buffer(); | ||
uint64_t* fields = static_cast<uint64_t*>(ab->GetContents().Data()); | ||
fields[0] = uv_hrtime(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d also appreciate some benchmarking showing that the ArrayBuffer
-based approach is faster than just returning the bigint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyGu There are no APIs for constructing a BigInt out of a uint64_t at the moment, it seems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could add one for uint64_t, but there is a discussion going on about the general API design of BigInt: https://bugs.chromium.org/p/v8/issues/detail?id=7712 A simple overload would be ambiguous anyway.
doc/api/process.md
Outdated
|
||
Unlike [`process.hrtime()`][], it does not support an additional `time` | ||
argument since the difference can just be computed directly | ||
by substraction of the two `bigint`s. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subtraction
doc/api/process.md
Outdated
// 191052633396993n | ||
|
||
console.log(`Benchmark took ${end - start} nanoseconds`); | ||
// benchmark took 1154389282 nanoseconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
benchmark -> Benchmark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm +1 on the addition but I'm not a fan of the scoping. Has there been some discussion around this? Personally I'm not a huge fan of putting bigint
functions on top of existing functions. The naming ends up being pretty non-obvious plus it's an extra property lookup. Even hrtimeBigInt
is more appealing to me, personally.
@apapirovski The naming is following the convention of The extra lookup can be avoided if the user assign the function to an identifier first (also if they are already using Another idea would be to expose a symbol similar to |
For reference: there is also whatwg/webidl#525 and w3c/IndexedDB#231 cc @littledan do you have any suggestion regarding how to expose variants of existing APIs that support BigInt? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the doc nit.
src/node.cc
Outdated
@@ -1580,6 +1581,9 @@ static void Kill(const FunctionCallbackInfo<Value>& args) { | |||
#define NANOS_PER_SEC 1000000000 | |||
|
|||
// Hrtime exposes libuv's uv_hrtime() high-resolution timer. | |||
|
|||
// This is the legacy version of hrtime before BigInt was introduced in | |||
// JavaScript. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, seeing this we probably want a note in the documentation for process.hrtime()
that the BigInt equivalent is preferred.
1ff4825
to
b4d45ba
Compare
Rebased and addressed nits: https://ci.nodejs.org/job/node-test-pull-request/15494/ |
CI is green. @apapirovski is your review in #21256 (review) blocking? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joyeecheung nope, it wasn't. :)
Landed in 1d8a231, thanks! |
PR-URL: #21256 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Depends on #21105 to land on v10.x-staging. |
PR-URL: #21256 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Notable changes: * console: * The `console.timeLog()` method has been implemented. (#21312) * deps: * Upgrade to libuv 1.22.0. (#21731) * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728) * http: * Added support for passing both `timeout` and `agent` options to `http.request`. (#21204) * napi: * Added experimental support for functions dealing with bigint numbers. (#21226) * process: * The `process.hrtime.bigint()` method has been implemented. (#21256) * Added the `--title` command line argument to set the process title on startup. (#21477) * trace_events: * Added process_name metadata. (#21477)
Notable changes: * console: * The `console.timeLog()` method has been implemented. (#21312) * deps: * Upgrade to libuv 1.22.0. (#21731) * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728) * http: * Added support for passing both `timeout` and `agent` options to `http.request`. (#21204) * napi: * Added experimental support for functions dealing with bigint numbers. (#21226) * process: * The `process.hrtime.bigint()` method has been implemented. (#21256) * Added the `--title` command line argument to set the process title on startup. (#21477) * trace_events: * Added process_name metadata. (#21477) * Added new collaborators * codebytere - Shelley Vohr PR-URL: #21851
Notable changes: * console: * The `console.timeLog()` method has been implemented. (#21312) * deps: * Upgrade to libuv 1.22.0. (#21731) * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728) * http: * Added support for passing both `timeout` and `agent` options to `http.request`. (#21204) * inspector: * Expose the original console API in `require('inspector').console`. (#21659) * napi: * Added experimental support for functions dealing with bigint numbers. (#21226) * process: * The `process.hrtime.bigint()` method has been implemented. (#21256) * Added the `--title` command line argument to set the process title on startup. (#21477) * trace_events: * Added process_name metadata. (#21477) * Added new collaborators * codebytere - Shelley Vohr PR-URL: #21851
Notable changes: * console: * The `console.timeLog()` method has been implemented. (#21312) * deps: * Upgrade to libuv 1.22.0. (#21731) * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728) * http: * Added support for passing both `timeout` and `agent` options to `http.request`. (#21204) * inspector: * Expose the original console API in `require('inspector').console`. (#21659) * napi: * Added experimental support for functions dealing with bigint numbers. (#21226) * process: * The `process.hrtime.bigint()` method has been implemented. (#21256) * Added the `--title` command line argument to set the process title on startup. (#21477) * trace_events: * Added process_name metadata. (#21477) * Added new collaborators * codebytere - Shelley Vohr PR-URL: #21851
The first commit is from #21255
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes