osx: thread id and signal stack support#1373
Conversation
dnoe
left a comment
dnoe
left a comment
There was a problem hiding this comment.
Mostly looks good, one question about the thread ID.
| return syscall(SYS_gettid); | ||
| #elif defined(__APPLE__) | ||
| int ret = mach_thread_self(); | ||
| mach_port_deallocate(mach_task_self(), ret); |
There was a problem hiding this comment.
Sorry I didn't catch this the first time. I found some info while searching about mach_thread_self/mach_port_deallocate:
https://codereview.chromium.org/276043002/
Did you try pthread_mach_thread_np(pthread_self())?
| #ifdef __linux__ | ||
| return syscall(SYS_gettid); | ||
| #elif defined(__APPLE__) | ||
| return pthread_mach_thread_np(pthread_self()); |
There was a problem hiding this comment.
Any reason for not using pthread_threadid_np() here?
uint64_t tid;
pthread_threadid_np(NULL, &tid);
return tid;
There was a problem hiding this comment.
The current method has the advantage of returning 32 bit ints. After some experimentation the values from the current method end up being repeatable across executions. Given that currentThreadId is used as part of a seed for a random number generator in source/common/runtime/runtime_impl.h, I'll switch to returning the low order bits of pthread_threadid_np's thread id.
There was a problem hiding this comment.
What's the advantage of returning 32 bits, exactly? Why can't we return full 64 bits?
And yes, kernel thread numbers (returned by pthread_mach_thread_np()) are quite aggressively recycled, AFAIK.
There was a problem hiding this comment.
There's no advantage to 32 bits beyond keeping this change local to this file.
| return syscall(SYS_gettid); | ||
| #elif defined(__APPLE__) | ||
| uint64_t tid; | ||
| pthread_threadid_np(NULL, &tid); |
There was a problem hiding this comment.
Can we just use this on all platforms now and lose ifdef? feel free to return uint64_t and lose the static cast if you want.
There was a problem hiding this comment.
There is no pthread_threadid_np() on Linux.
There was a problem hiding this comment.
That function doesn't exist on Linux.
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM. @dnoe @PiotrSikora ?
|
|
||
| int32_t Thread::currentThreadId() { return syscall(SYS_gettid); } | ||
| int32_t Thread::currentThreadId() { | ||
| #ifdef __linux__ |
There was a problem hiding this comment.
Nit: could you either add #else case or use Linux as the fallback? i.e.
#if defined(__linux__)
return syscall(SYS_gettid);
#elif defined(__APPLE__)
uint64_t tid;
pthread_threadid_np(NULL, &tid);
return tid;
#else
#error "OS not supported."
#endif
or
#ifdef __APPLE__
uint64_t tid;
pthread_threadid_np(NULL, &tid);
return tid;
#else
return syscall(SYS_gettid);
#endif
otherwise we're going to end with code that doesn't have return value on other OSes.
Either one is fine with me, assuming @mattklein123 doesn't have any objections.
There was a problem hiding this comment.
Yeah I thought about this. Agree this would be a bit nicer. Either way the compile will fail so not sure it matters that much. No preference from me about either of these or leaving what we have.
There was a problem hiding this comment.
Added an #error directive.
Description: fixes two leaks in the platform filter chain. One in the common code, and one specific to iOS Risk Level: low Testing: ran with Lyft's Driver app under xcode's memory profiler. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: fixes two leaks in the platform filter chain. One in the common code, and one specific to iOS Risk Level: low Testing: ran with Lyft's Driver app under xcode's memory profiler. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
…1373) **Description** Despite the documentation about "messages" operation, the anthropic messages processor was wrongly using the chat completions metrics implementation hence the metrics produced in /messages endpoint were mixed up with the ones in /chat/completions metrics. This fixes it and make it in line with the documentation. --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
4 participants
Modifies
source/common/common/thread.ccto retrieve a thread id on OS X.Modifies
source/exe/signal_action.ccto retrieve RIP in OS X. Increases alt stack size used for signal handlers to meet the OS minimum values. Handles an OS X bug where disabling the alt signal stack fails because the size field in the original stack_t is less than MINSIGSTKSZ.(Split out from #1348, in support of #128.)