Add threadid to AsyncStackRootHolder #644
Conversation
facebook-github-bot
added
the
CLA Signed
source/async_stack.cpp
Outdated
| #elif defined(_WIN32) | ||
| return uint64_t(GetCurrentThreadId()); | ||
| #else | ||
| return uint64_t(syscall(UNIFEX_SYS_gettid)); |
There was a problem hiding this comment.
Can we avoid defining UNIFEX_SYS_gettid by just switching here on whether SYS_gettid is defined?
Also, I'm a bit anxious about the cost of a syscall in each AsyncStackRoot constructor since we construct so many of them. Is this a cheap syscall implemented in user space? If so then it doesn't matter, but if it's a typical syscall that transits into kernel space then we should probably revisit capturing this value in the AsyncStackRootHolder since we'll have one of those per thread and the per-thread value of this is constant.
There was a problem hiding this comment.
So I updated this to use the glibc wrapper of gettid because it implements caching (D49174906)
I also spoke to David and his response is that it's difficult to say because it depends on the implementation and we need to profile the code in order to know. This will be used a lot on android and his response is below:
I see. The overhead of the syscall is mostly dependent on what speculative execution mitigations are turned on e.g. is the kernel hardened. I don't know how we configure our Android kernels.
The more mitigations the costlier the overhead of a user to kernel transition
The only real way to know is to profile the code. But as a hunch thread creation should be off the hot path so I don't imagine adding a gettid in there will be problematic
There was a problem hiding this comment.
Synced with @ispeters offline and decided to move threadid to async stack root holder for getting the threadid per thread only
jesswong
changed the title
source/async_stack.cpp
Outdated
| namespace utils { | ||
| static std::uint64_t get_os_thread_id() { | ||
| # if defined(__APPLE__) | ||
| std::uint64_t tid; | ||
| pthread_threadid_np(nullptr, &tid); | ||
| return tid; | ||
| # elif defined(_WIN32) | ||
| return std::uint64_t(GetCurrentThreadId()); | ||
| # else | ||
| return std::uint64_t(gettid()); | ||
| # endif | ||
| } | ||
| #endif // UNIFEX_ASYNC_STACK_ROOT_USE_PTHREAD == 0 | ||
| } // namespace utils |
There was a problem hiding this comment.
Since get_os_thread_id is static and inside the unifex namespace, I don't think you don't need the utils namespace.
This diff adds the threadid to the ASR in order to unambiguously correlate the ASR's symbol to the thread.
This is loosely based off folly's implementation of getOSThreadID: https://github.com/facebook/folly/blob/main/folly/system/ThreadId.cpp#L41
The main difference is that folly supports EMSCRIPTEN and BSD, which I do not include.
We expect to only need this for ios, android, and linux applications.