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: use uv_gettimeofday() #27029

Merged
merged 2 commits into from
Apr 22, 2019
Merged

src: use uv_gettimeofday() #27029

merged 2 commits into from
Apr 22, 2019

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 31, 2019

Labeled as a WIP until the next libuv update (ignore the first commit here).

The next version of libuv will provide uv_gettimeofday() as a cross platform alternative to gettimeofday().

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 31, 2019
src/node_perf.cc Outdated Show resolved Hide resolved
src/node_perf.cc Outdated Show resolved Hide resolved
uv_timeval_t ts;
if (uv_gettimeofday(&ts) == 0) {
writer.json_keyvalue("dumpEventTimeStamp",
std::to_string(ts.tv_sec * 1000 + ts.tv_usec / 1000));
Copy link
Member

@bnoordhuis bnoordhuis Apr 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might overflow when tv_sec is a 32 bits long.

(I kind of regret that we didn't use int64_t for uv_timeval_t.tv_sec...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there's still time to change uv_gettimeofday to uint64_t uv_gettimeofday() (or unsigned long long int) and always return microseconds? Seems like uv_timeval_t is a little "legacy" for this API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not a bad idea. Since uv_gettimeofday() hasn't been released yet, there is time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can discuss at libuv/libuv#2243.

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 15, 2019

@bnoordhuis I think I've addressed your nits with the new libuv release.

@cjihrig cjihrig marked this pull request as ready for review April 16, 2019 14:35
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 22, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/22640/

EDIT(cjihrig): CI was green.

dumpEventTimeStamp was not implemented on Windows, and did not
include any error checking. This commit adds Windows support
and error checking.

PR-URL: nodejs#27029
Reviewed-By: Refael Ackermann <refack@gmail.com>
Use uv_gettimeofday() in GetCurrentTimeInMicroseconds() to
remove the need for #ifdef logic.

PR-URL: nodejs#27029
Reviewed-By: Refael Ackermann <refack@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 22, 2019

Landed in 90cf2d5...8e1e994.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants