Page MenuHomePhabricator

[libcxx] Threading support: externalize sleep_for()
ClosedPublic

Authored by rmaprath on Feb 7 2017, 4:38 AM.

Details

Summary

Different platforms implement the wait/sleep behaviour in very different ways. It makes sense to hoist this functionality into the threading API.

I also feel similarly about hardware_concurrecy() implementation. Any thoughts on that?

Diff Detail

Repository
rL LLVM

Event Timeline

rmaprath created this revision.Feb 7 2017, 4:38 AM
joerg added a subscriber: joerg.Feb 7 2017, 6:05 AM
joerg added inline comments.
include/__threading_support
367 ↗(On Diff #87395)

I don't think giga::num makes things any clear compared to just spelling it out.

593 ↗(On Diff #87395)

Use (ns + 999999) so that the cast rounds up.

594 ↗(On Diff #87395)

Why is ns == 0 supposed to sleep at all? In fact, the caller already ensures that can't happen?

rmaprath added inline comments.Feb 7 2017, 6:19 AM
include/__threading_support
593 ↗(On Diff #87395)

So, this code was lifted from the sources as-is. I will do this change, I think it makes sense.

594 ↗(On Diff #87395)

IIUC, this is trying to detect round-downs and then compensate for it. With the above suggestion, this should no longer be needed. Will get rid of it as well.

EricWF accepted this revision.Feb 7 2017, 7:11 PM

LGTM. @joerg has raised some legitimate concerns about the Windows implementation. However that shouldn't hold this cleanup up, since the implementation is preexisting.

include/__threading_support
354 ↗(On Diff #87395)

Reserved names are needed for s and ns ts and the like now that this is in a header.

This revision is now accepted and ready to land.Feb 7 2017, 7:11 PM
rmaprath updated this revision to Diff 87669.Feb 8 2017, 9:26 AM

Sorry for the delay, I've updated the patch with all the comments addressed.

@EricWF: Got one question before I commit: The sleep_for function in the dylib (thread.cpp) is now quite small. Is there a particular reason to keep it there? I was wondering if we should move that bit to the <thread> header altogether.

EricWF added a comment.Feb 8 2017, 9:27 AM

Sorry for the delay, I've updated the patch with all the comments addressed.

@EricWF: Got one question before I commit: The sleep_for function in the dylib (thread.cpp) is now quite small. Is there a particular reason to keep it there? I was wondering if we should move that bit to the <thread> header altogether.

Removing the definition in the dylib would be ABI breaking.

Sorry for the delay, I've updated the patch with all the comments addressed.

@EricWF: Got one question before I commit: The sleep_for function in the dylib (thread.cpp) is now quite small. Is there a particular reason to keep it there? I was wondering if we should move that bit to the <thread> header altogether.

Removing the definition in the dylib would be ABI breaking.

Ah! of course.

Thanks.

joerg accepted this revision.Feb 8 2017, 2:35 PM

One small issue left, otherwise LGTM.

include/__threading_support
187 ↗(On Diff #87669)

Drop the name here.

This revision was automatically updated to reflect the committed changes.