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

libextra: Rewrite MutexArc #11708

Closed
wants to merge 1 commit into from

Conversation

pcwalton
Copy link
Contributor

/// Called when the Arc is poisoned. This is out of line to guide
/// inlining.
#[inline(never)]
fn die_from_poison() -> ! {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have a #[cold] attribute as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Doesn't/shouldn't any function that never returns (-> !) automatically count as freezing cold?)

Copy link
Member

Choose a reason for hiding this comment

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

Did you actually see a measureable improvement by marking this inline(never)? It'll already almost never be inlined because it's not cross-crate instantiated (no type parameters like the mutex).

Recently I've seen failing code have excellent codegen.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK the fact that it's noreturn should make it be considered cold and seems to prevent inlining when I test.

@alexcrichton
Copy link
Member

This looks pretty awesome! I'm curious if we should worry about the removal of the attached condition variable, but that's probably another flavor of arc. I would expect a MutexArc to be "an arc plus a mutex" and that's it.

@alexcrichton
Copy link
Member

I remembered this morning that this does remove the cvar from inside the mutex, which is a little worrisome to me. We'd be required to write a more efficient cvar (like a mutex), but that should in theory be much easier. I think that it could be added in a backwards compatible way, I'm thinking that the wait function takes the mutex's Guard structure.

@bill-myers
Copy link
Contributor

@alexcrichton Yes, but it needs to be done carefully.

According to http://pubs.opengroup.org/onlinepubs/9699919799/:

"When a thread waits on a condition variable, having specified a particular mutex to either the pthread_cond_timedwait() or the pthread_cond_wait() operation, a dynamic binding is formed between that mutex and condition variable that remains in effect as long as at least one thread is blocked on the condition variable. During this time, the effect of an attempt by any thread to wait on that condition variable using a different mutex is undefined. Once all waiting threads have been unblocked (as by the pthread_cond_broadcast() operation), the next wait operation on that condition variable shall form a new dynamic binding with the mutex specified by that wait operation. [...]"

So unless one determines that this is not actually a problem on all implementations, then a safe interface will also need to enforce this itself.

@alexcrichton
Copy link
Member

I closed #11610 for now, so I'm going to close this one as well, sadly (needs the mutex rewrite first).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants