Skip to content

util: Use steady clock instead of system clock to measure durations #27405

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
Merged
merged 5 commits into from
May 6, 2023
Merged

util: Use steady clock instead of system clock to measure durations #27405

merged 5 commits into from
May 6, 2023

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Apr 3, 2023

GetTimeMillis has multiple issues:

  • It doesn't denote the underlying clock type
  • It isn't type-safe
  • It is used incorrectly in places that should use a steady clock

Fix all issues here.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 3, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK willcl-ark, martinus
Concept ACK dergoegge, fanquake, RandyMcMillan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@dergoegge
Copy link
Member

Concept ACK

@maflcko maflcko changed the title Use steady clock instead of system clock to measure durations util: Use steady clock instead of system clock to measure durations Apr 3, 2023
@maflcko maflcko force-pushed the 2304-steady-over-millis- branch from fa069d8 to ffff99d Compare April 3, 2023 12:59
martinus
martinus reviewed Apr 3, 2023
View reviewed changes
Copy link
Contributor

@martinus martinus left a comment

Choose a reason for hiding this comment

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

Concept ACK

@fanquake
Copy link
Member

fanquake commented Apr 4, 2023

Concept ACK

@maflcko maflcko force-pushed the 2304-steady-over-millis- branch from ffff99d to fa83fb3 Compare April 4, 2023 10:34
@DrahtBot DrahtBot mentioned this pull request Apr 26, 2023
Closed
3 tasks
@fanquake fanquake requested a review from willcl-ark May 4, 2023 12:49
@willcl-ark
Copy link
Member

ACK fa83fb3

This is a nice cleanup to the clocks.

There are a few time values left as other types in the codebase, but it doesn't make sense to change them as they are being used as those types by other components (mainly libevent it seems).

@DrahtBot DrahtBot removed the request for review from willcl-ark May 5, 2023 09:42
@RandyMcMillan
Copy link
Contributor

Concept ACK

@fanquake fanquake requested a review from martinus May 5, 2023 16:16
@DrahtBot DrahtBot removed the request for review from martinus May 5, 2023 18:31
martinus
martinus approved these changes May 5, 2023
View reviewed changes
Copy link
Contributor

@martinus martinus left a comment

Choose a reason for hiding this comment

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

Code review ACK fa83fb3, also ran all tests. All usages of the steady_clock are just for duration measurements, so the change to a different epoch is ok.

@fanquake fanquake merged commit e460c0a into bitcoin:master May 6, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 7, 2023
@fanquake
Merge bitcoin#27405: util: Use steady clock instead of system clock t…
6f8d674 
…o measu…
@maflcko maflcko deleted the 2304-steady-over-millis- branch May 8, 2023 07:04
kwvg added a commit to kwvg/dash that referenced this pull request Jul 19, 2023
@kwvg
partial bitcoin#27405: Use steady clock instead of system clock to me…
8d26315 
…asure durations

includes:
- fa1d804
kwvg added a commit to kwvg/dash that referenced this pull request Jul 19, 2023
@kwvg
partial bitcoin#27405: Use steady clock instead of system clock to me…
8e02949 
…asure durations

includes:
- fa1d804
kwvg added a commit to kwvg/dash that referenced this pull request Jul 23, 2023
@kwvg
partial bitcoin#27405: Use steady clock instead of system clock to me…
01511fb 
…asure durations

includes:
- fa1d804
kwvg added a commit to kwvg/dash that referenced this pull request Jul 23, 2023
@kwvg
partial bitcoin#27405: Use steady clock instead of system clock to me…
fbe7679 
…asure durations

includes:
- fa1d804
kwvg added a commit to kwvg/dash that referenced this pull request Jul 24, 2023
@kwvg
partial bitcoin#27405: Use steady clock instead of system clock to me…
bca200e 
…asure durations

includes:
- fa1d804
kwvg added a commit to kwvg/dash that referenced this pull request Jul 28, 2023
@kwvg
partial bitcoin#27405: Use steady clock instead of system clock to me…
d1d82b9 
…asure durations

includes:
- fa1d804
kwvg added a commit to kwvg/dash that referenced this pull request Jul 28, 2023
@kwvg
partial bitcoin#27405: Use steady clock instead of system clock to me…
4652f91 
…asure durations

includes:
- fa1d804
kwvg added a commit to kwvg/dash that referenced this pull request Jul 30, 2023
@kwvg
partial bitcoin#27405: Use steady clock instead of system clock to me…
c336e4a 
…asure durations

includes:
- fa1d804
knst pushed a commit to knst/dash that referenced this pull request Aug 1, 2023
@kwvg @knst
partial bitcoin#27405: Use steady clock instead of system clock to me…
6bf586a 
…asure durations

includes:
- fa1d804
knst pushed a commit to knst/dash that referenced this pull request Aug 1, 2023
@kwvg @knst
partial bitcoin#27405: Use steady clock instead of system clock to me…
560400b 
…asure durations

includes:
- fa1d804
kwvg added a commit to kwvg/dash that referenced this pull request Aug 1, 2023
@kwvg
partial bitcoin#27405: Use steady clock instead of system clock to me…
d06ce9d 
…asure durations

includes:
- fa1d804
kwvg added a commit to kwvg/dash that referenced this pull request Aug 1, 2023
@kwvg
partial bitcoin#27405: Use steady clock instead of system clock to me…
5c4c4af 
…asure durations

includes:
- fa1d804
kwvg added a commit to kwvg/dash that referenced this pull request Aug 1, 2023
@kwvg
partial bitcoin#27405: Use steady clock instead of system clock to me…
b5dd43b 
…asure durations

includes:
- fa1d804
PastaPastaPasta pushed a commit to kwvg/dash that referenced this pull request Aug 2, 2023
@kwvg @PastaPastaPasta
partial bitcoin#27405: Use steady clock instead of system clock to me…
a9e3b9f 
…asure durations

includes:
- fa1d804
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Aug 2, 2023
@PastaPastaPasta
Merge pull request #5501 from kwvg/assumeutxo5
0b8e501 
backport: merge bitcoin#19521, bitcoin#22047, bitcoin#21796, bitcoin#21767, bitcoin#24133, bitcoin#24921, bitcoin#27988, partial bitcoin#24117, bitcoin#24138, bitcoin#27405 (assumeutxo: part 5)
@bitcoin bitcoin locked and limited conversation to collaborators May 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@martinus martinus martinus approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@maflcko @DrahtBot @dergoegge @fanquake @willcl-ark @RandyMcMillan @martinus