This is an archive of the discontinued LLVM Phabricator instance.

[Support] Add a way to run a function on a detached thread
ClosedPublic

Authored by Dmitry.Kozhevnikov on Aug 22 2018, 7:28 AM.

Details

Summary

This roughly mimics std::thread(...).detach() except it allows to customize the stack size. Required for https://reviews.llvm.org/D50993.

I've decided against reusing the existing llvm_execute_on_thread because it's not obvious what to do with the ownership of the passed function/arguments:

  1. If we pass possibly owning functions data to llvm_execute_on_thread, we'll lose the ability to pass small non-owning non-allocating functions for the joining case (as it's used now). Is it important enough?
  2. If we use the non-owning interface in the new use case, we'll force clients to transfer ownership to the spawned thread manually, but similar code would still have to exist inside llvm_execute_on_thread(_async) anyway (as we can't just pass the same non-owning pointer to pthreads and Windows implementations, and would be forced to wrap it in some structure, and deal with its ownership.

Diff Detail

Event Timeline

zturner accepted this revision.Aug 22 2018, 8:40 AM
zturner added a subscriber: zturner.

I'm almost wondering if we should just make a replacement for std::thread that allows to set the stack size in the constructor. The approach here isn't wrong per se, but I think if we could just say llvm::thread(Func, StackSize); it would be useful in more places.

lib/Support/Threading.cpp
109 ↗(On Diff #161951)

Shouldn't the last parameter be set to true here?

This revision is now accepted and ready to land.Aug 22 2018, 8:40 AM
Dmitry.Kozhevnikov marked an inline comment as done.
Dmitry.Kozhevnikov added inline comments.
lib/Support/Threading.cpp
109 ↗(On Diff #161951)

Oops, sorry, last-minute typo.

Dmitry.Kozhevnikov marked an inline comment as done.

Added a smoke test for blocking llvm_execute_on_thread as well

jfb requested changes to this revision.Aug 22 2018, 9:10 AM
jfb added inline comments.
include/llvm/Support/Threading.h
81 ↗(On Diff #161975)

Seems like you want an optional<> for stack size. Can you also change the name so it contains the unit? i.e. Requested StackSizeInBytes.

lib/Support/Unix/Threading.inc
59 ↗(On Diff #161975)

Please change ShouldJoin to an enum class instead of a bool. Ditto on the return value.

77 ↗(On Diff #161975)

So any failure to init attr, set stack size, or create the thread just... silently fail? I know this is pre-existing, but that's terrible! Is there ever a situation when we want to execute something on a thread, or just not execute it at all? It seems like we should abort if any of these fail?

This revision now requires changes to proceed.Aug 22 2018, 9:10 AM

Addressed review comments:

  • Any failure when spawning a thread now causes a fatal error
  • Replaced a bool flag with an enum
  • Replaced unsigned RequestedStackSize with Optional<unsigned> StackSizeInBytes
jfb added inline comments.Aug 27 2018, 8:50 AM
include/llvm/Support/Threading.h
53 ↗(On Diff #162647)

Can you remove the llvm_execute_on_thread while you're here?

lib/Support/Threading.cpp
119 ↗(On Diff #162647)

llvm::make_unique instead of new.

122 ↗(On Diff #162647)

I'd replace get above with release, and not have the release here, since you're kinda "moving" ownership into a void*.

lib/Support/Unix/Threading.inc
65 ↗(On Diff #162647)

Here and below, pthread doesn't set errno: it returns the error directly. You should pass the result in and call strerror.

include/llvm/Support/Threading.h
53 ↗(On Diff #162647)

Should I? It's used twice: at CrashRecoveryContext::RunSafelyOnThread, and, which is probably worse, exposed via the libclang (tools/clang/include/clang-c/Index.h, clang_executeOnThread). My understanding was this API is meant to be stable? That was one of the reasons I haven't touched its interface at all.

Also, while looking for this, I've found the fallback implementation of llvm_execute_on_thread, in lib/Support/Threading.cpp, which just runs the callback synchronously. Sorry, I haven't updated its signature, will do. I've probably should also add the new function there, that would just raise a fatal error since it's meaningless to call it with multithreading disabled.

lib/Support/Threading.cpp
122 ↗(On Diff #162647)

I think in the current state, I can just drop unique_ptr and just pass a raw pointer returned by new directly.

lib/Support/Unix/Threading.inc
65 ↗(On Diff #162647)

Sorry, will do.

jfb added inline comments.Aug 27 2018, 9:11 AM
include/llvm/Support/Threading.h
53 ↗(On Diff #162647)

I mean the comment start. Documentation like this is the old LLVM style, and as we update code we also update the docs:

http://www.llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

include/llvm/Support/Threading.h
53 ↗(On Diff #162647)

I see, thank you for the info, will do.

Address review comments:

  • use error code returned from pthread functions to report errors
  • use naked new instead of unique_ptr (it's always released anyway)
  • tune comments
  • add matching function stubs when threading implementation is unavailable or unknown
jfb accepted this revision.Aug 27 2018, 12:47 PM

Unix stuff looks good. I didn't look at Windows.

This revision is now accepted and ready to land.Aug 27 2018, 12:47 PM
In D51103#1214852, @jfb wrote:

Unix stuff looks good. I didn't look at Windows.

Thank you!

Should I invite someone else for the review?

jfb added a comment.Aug 27 2018, 12:53 PM
In D51103#1214852, @jfb wrote:

Unix stuff looks good. I didn't look at Windows.

Thank you!

Should I invite someone else for the review?

Ain't no party like a Windows review party!

I mean, yes :)

ilya-biryukov added a subscriber: ilya-biryukov.

IIUC, someone needs to review the Windows bits here.
I volunteer to do this tomorrow unless someone else want to beat me to this :-)

jfb added a subscriber: rnk.May 9 2019, 10:38 AM

IIUC, someone needs to review the Windows bits here.
I volunteer to do this tomorrow unless someone else want to beat me to this :-)

Maybe @rnk can help.

rnk accepted this revision.May 14 2019, 4:01 PM

Windows parts look good! Sorry for the delay.

@Dmitry.Kozhevnikov, is this good to go or do you plan more changes?

Dmitry.Kozhevnikov set the repository for this revision to rG LLVM Github Monorepo.May 21 2019, 2:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2019, 2:01 AM
  • moved to monorepo, rebased
  • I've added an std::thread-based implementation of llvm_execute_on_thread_async when threading is on, but it's non-win32 non-pthread implementation (otherwise we should expose this detail for clients so they could decide if they could use it).

@Dmitry.Kozhevnikov, is this good to go or do you plan more changes?

I've added the last changes I wanted to make today.

This revision was automatically updated to reflect the committed changes.

We realized today this never got landed, and I tried to do so. Unfortunately the callers weren't updated for the unsigned -> optional change.
Stack sizes seem to get passed around through lots of functions. I can update the immediate callers only and reland.