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

std: Avoid usage of `Once` in `Instant` #59676

Merged
merged 1 commit into from Apr 4, 2019

Conversation

Projects
None yet
6 participants
@alexcrichton
Copy link
Member

commented Apr 3, 2019

This commit removes usage of Once from the internal implementation of
time utilities on OSX and Windows. It turns out that we accidentally hit
a deadlock today (#59020) via events that look like:

  • A thread invokes park_timeout
  • Internally, only on OSX, park_timeout calls Instant::elapsed
  • Inside of Instant::elapsed on OSX we enter a Once to initialize
    global timer data
  • Inside of Once, it attempts to park

This means on the same stack frame, when there's contention, we're
calling park from inside park_timeout, causing a deadlock!

The solution implemented in this commit was to remove usage of Once
and instead just do a small dance with atomics. There's no real need we
need to guarantee that the global information is only learned once, only
that it's only stored once. This implementation may have multiple
threads invoke mach_timebase_info, but only one will store the global
information which will amortize the cost for all other threads.

A similar fix has been applied to windows to be uniform across our
implementations, but looking at the code on Windows no deadlock was
possible. This is purely just a consistency update for Windows and in
theory a slightly leaner implementation.

Closes #59020

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2019

r? @rkruppe

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@rust-highfive rust-highfive assigned sfackler and unassigned rkruppe Apr 3, 2019

@sfackler

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

📌 Commit 4ef32a4 has been approved by sfackler

Centril added a commit to Centril/rust that referenced this pull request Apr 3, 2019

Rollup merge of rust-lang#59676 - alexcrichton:osx-deadlock, r=sfackler
std: Avoid usage of `Once` in `Instant`

This commit removes usage of `Once` from the internal implementation of
time utilities on OSX and Windows. It turns out that we accidentally hit
a deadlock today (rust-lang#59020) via events that look like:

* A thread invokes `park_timeout`
* Internally, only on OSX, `park_timeout` calls `Instant::elapsed`
* Inside of `Instant::elapsed` on OSX we enter a `Once` to initialize
  global timer data
* Inside of `Once`, it attempts to `park`

This means on the same stack frame, when there's contention, we're
calling `park` from inside `park_timeout`, causing a deadlock!

The solution implemented in this commit was to remove usage of `Once`
and instead just do a small dance with atomics. There's no real need we
need to guarantee that the global information is only learned once, only
that it's only *stored* once. This implementation may have multiple
threads invoke `mach_timebase_info`, but only one will store the global
information which will amortize the cost for all other threads.

A similar fix has been applied to windows to be uniform across our
implementations, but looking at the code on Windows no deadlock was
possible. This is purely just a consistency update for Windows and in
theory a slightly leaner implementation.

Closes rust-lang#59020

@Centril Centril referenced this pull request Apr 3, 2019

Closed

Rollup of 8 pull requests #59682

bors added a commit that referenced this pull request Apr 3, 2019

Auto merge of #59682 - Centril:rollup-ncm5ojo, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #59316 (Internal lints take 2)
 - #59663 (Be more direct about borrow contract)
 - #59664 (Updated the documentation of spin_loop and spin_loop_hint)
 - #59666 (Updated the environment description in rustc.)
 - #59669 (Reduce repetition in librustc(_lint) wrt. impl LintPass by using macros)
 - #59672 (Revert rust-lld place changes)
 - #59676 (std: Avoid usage of `Once` in `Instant`)
 - #59677 (rustfix coverage: Skip UI tests with non-json error-format)

Failed merges:

r? @ghost
@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

Failed in #59682 (comment), @bors r-

@Centril Centril closed this Apr 3, 2019

@Centril Centril reopened this Apr 3, 2019

std: Avoid usage of `Once` in `Instant`
This commit removes usage of `Once` from the internal implementation of
time utilities on OSX and Windows. It turns out that we accidentally hit
a deadlock today (#59020) via events that look like:

* A thread invokes `park_timeout`
* Internally, only on OSX, `park_timeout` calls `Instant::elapsed`
* Inside of `Instant::elapsed` on OSX we enter a `Once` to initialize
  global timer data
* Inside of `Once`, it attempts to `park`

This means on the same stack frame, when there's contention, we're
calling `park` from inside `park_timeout`, causing a deadlock!

The solution implemented in this commit was to remove usage of `Once`
and instead just do a small dance with atomics. There's no real need we
need to guarantee that the global information is only learned once, only
that it's only *stored* once. This implementation may have multiple
threads invoke `mach_timebase_info`, but only one will store the global
information which will amortize the cost for all other threads.

A similar fix has been applied to windows to be uniform across our
implementations, but looking at the code on Windows no deadlock was
possible. This is purely just a consistency update for Windows and in
theory a slightly leaner implementation.

Closes #59020

@alexcrichton alexcrichton force-pushed the alexcrichton:osx-deadlock branch from 4ef32a4 to cb57484 Apr 4, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@bors: r=sfackler

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

📌 Commit cb57484 has been approved by sfackler

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

⌛️ Testing commit cb57484 with merge 53f2165...

bors added a commit that referenced this pull request Apr 4, 2019

Auto merge of #59676 - alexcrichton:osx-deadlock, r=sfackler
std: Avoid usage of `Once` in `Instant`

This commit removes usage of `Once` from the internal implementation of
time utilities on OSX and Windows. It turns out that we accidentally hit
a deadlock today (#59020) via events that look like:

* A thread invokes `park_timeout`
* Internally, only on OSX, `park_timeout` calls `Instant::elapsed`
* Inside of `Instant::elapsed` on OSX we enter a `Once` to initialize
  global timer data
* Inside of `Once`, it attempts to `park`

This means on the same stack frame, when there's contention, we're
calling `park` from inside `park_timeout`, causing a deadlock!

The solution implemented in this commit was to remove usage of `Once`
and instead just do a small dance with atomics. There's no real need we
need to guarantee that the global information is only learned once, only
that it's only *stored* once. This implementation may have multiple
threads invoke `mach_timebase_info`, but only one will store the global
information which will amortize the cost for all other threads.

A similar fix has been applied to windows to be uniform across our
implementations, but looking at the code on Windows no deadlock was
possible. This is purely just a consistency update for Windows and in
theory a slightly leaner implementation.

Closes #59020
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: sfackler
Pushing 53f2165 to master...

@bors bors added the merged-by-bors label Apr 4, 2019

@bors bors merged commit cb57484 into rust-lang:master Apr 4, 2019

1 check passed

homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.