Skip to content

Revert "<chrono>: Cache QueryPerformanceFrequency() and divide just o… #972

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
StephanTLavavej merged 3 commits into from
Jul 3, 2020
Merged

Revert "<chrono>: Cache QueryPerformanceFrequency() and divide just o… #972

StephanTLavavej merged 3 commits into from
Jul 3, 2020

Conversation

@AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Jul 2, 2020 •
edited by StephanTLavavej
Loading

…nce (#653)"

This reverts commit ad1a26a.

Fixes #971.

@AlexGuteniev
Revert "<chrono>: Cache QueryPerformanceFrequency() and divide just o…
8c360df 
…nce (microsoft#653)"

This reverts commit ad1a26a.

# Conflicts:
#	stl/inc/chrono
#	stl/inc/xatomic.h
@AlexGuteniev
reverted too much
c446067
@AlexGuteniev AlexGuteniev marked this pull request as ready for review July 2, 2020 13:20
@AlexGuteniev AlexGuteniev requested a review from a team as a code owner July 2, 2020 13:20
@AlexGuteniev AlexGuteniev mentioned this pull request Jul 2, 2020
Closed
@BillyONeal
Copy link
Member

BillyONeal commented Jul 2, 2020

Why do you want to revert this?

@BillyONeal
Copy link
Member

BillyONeal commented Jul 2, 2020

Oh I see the linked issue

@CaseyCarter CaseyCarter linked an issue Jul 2, 2020 that may be closed by this pull request
<chrono>: std::chrono::steady_clock::now() overflow #971
Closed
@CaseyCarter CaseyCarter added the bug Something isn't working label Jul 2, 2020
BillyONeal
BillyONeal approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

:'(

@cbezault
Copy link
Contributor

cbezault commented Jul 2, 2020

Why not just fix the calculation?

@AlexGuteniev
Copy link
Contributor Author

AlexGuteniev commented Jul 2, 2020

The proposed fix in #971 introduces precision loss if den does not divide by frequency (observed, although rarely).

There are still ways to optimize. _div128 is one of options. Another is to special case 10'000'000 frequency, as it is common. But I think the first thing to do is to fix the bug. And then probably don't touch this anymore.

@StephanTLavavej StephanTLavavej added the high priority Important! label Jul 2, 2020
StephanTLavavej
StephanTLavavej requested changes Jul 2, 2020
View reviewed changes
@StephanTLavavej StephanTLavavej self-assigned this Jul 2, 2020
@StephanTLavavej StephanTLavavej merged commit 5be7d49 into microsoft:master Jul 3, 2020
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jul 3, 2020

Thanks again for fixing this so quickly! 😺

@AlexGuteniev AlexGuteniev deleted the undo_chrono_changes branch July 3, 2020 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+2 more reviewers

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@BillyONeal BillyONeal BillyONeal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

bug Something isn't working high priority Important!

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<chrono>: std::chrono::steady_clock::now() overflow

5 participants

@AlexGuteniev @BillyONeal @cbezault @StephanTLavavej @CaseyCarter