[RemoteMirror][swift-inspect] Decode locks in priority-escalation concurrency runtime. #41855
Conversation
|
|
||
| PriorityMask = 0xFF00, | ||
| PriorityShift = 0x8, | ||
| }; |
There was a problem hiding this comment.
Could we hoist the enum out of the function scope? Having the symbolic names for the flags is a general convenience (and may be useful outside of the function scope).
There was a problem hiding this comment.
This feels like it ought to be shared with ActiveActorStatus in some way. Similarly for the flags for the task status.
| @@ -248,6 +248,9 @@ typedef struct swift_async_task_info { | |||
| uint8_t IsRunning; | |||
| uint8_t IsEnqueued; | |||
|
|
|||
| uint8_t HasThreadPort; | |||
There was a problem hiding this comment.
I assume that the only reason that we need HasThreadPort is because ThreadPort is 32-bit and mach_port_t is uintptr_t sized, and thus we cannot use the traditional invalid port specifier?
There was a problem hiding this comment.
No, it's cause on platforms where we don't do anything useful with the thread port for priority escalation, we don't even bother tracking that value in the active state.
| uint64_t Flags; | ||
| uint8_t State; | ||
| uint8_t IsDistributedRemote; | ||
| uint8_t IsPriorityEscalated; |
There was a problem hiding this comment.
Should these 2 really be uint8_t rather than bit flags (or _Bool)?
There was a problem hiding this comment.
Space isn't a concern, but using a boolean would be smart. C++ doesn't like _Bool but bool plus stdbool.h does the job.
stdlib/public/Concurrency/Actor.cpp
Outdated
| @@ -608,6 +608,8 @@ class JobRef { | |||
| /// DefaultActorImpl. The additional alignment requirements are | |||
| /// enforced by static asserts below. | |||
| class alignas(sizeof(void *) * 2) ActiveActorStatus { | |||
| // These flags are decoded in ReflectionContext.h, update them there if you | |||
| // make changes here. | |||
There was a problem hiding this comment.
Once hoisted, could we not re-use the same declaration of the flags?
| var threadList: UnsafeMutablePointer<mach_port_t>? | ||
| var threadCount: mach_msg_type_number_t = 0 | ||
|
|
||
| let result = task_threads(task, &threadList, &threadCount) |
There was a problem hiding this comment.
Is there a formal signature for task_threads? I worry about the type change here without any type of memory rebinding.
There was a problem hiding this comment.
That's mach for you. The signature is:
kern_return_t task_threads(task_inspect_t target_task, thread_act_array_t *act_list, mach_msg_type_number_t *act_listCnt);
This one actually isn't too bad. thread_info is where it gets really gnarly. These APIs aren't nice to use from Swift. I think I got all the types right though.
There was a problem hiding this comment.
I don't see how UnsafeMutablePointer<mach_port_t> (aka mach_port_t *) is the same as thread_act_array_t. Do we not need to rebind the memory?
There was a problem hiding this comment.
It’s a typedef, everything is a Mach port in the end.
typedef mach_port_t thread_act_t;
typedef thread_act_t *thread_act_array_t;
There was a problem hiding this comment.
Ah, thanks! I wish that this was easy to lookup.
…currency runtime. When possible, decode the DrainLock/ExecutionLock fields of tasks and actors in concurrency runtimes built with priority escalation, and show the corresponding thread info in swift-inspect output. We weren't properly decoding actor flags previously, so fix that up as well and have Remote Mirror split them out into separate fields so clients don't have to. We were missing the Job Storage field from the definition of DefaultActorImpl in RuntimeInternals.h, fix that so we actually read the right data. rdar://88598003
mikeash
force-pushed
the
swift-inspect-locks
branch
from
9cd3334 to
ae2b514
Compare
|
I extracted the actor flags constants into a common header, and switched all the booleans to actual |
|
@swift-ci please test |
When possible, decode the DrainLock/ExecutionLock fields of tasks and actors in concurrency runtimes built with priority escalation, and show the corresponding thread info in swift-inspect output.
We weren't properly decoding actor flags previously, so fix that up as well and have Remote Mirror split them out into separate fields so clients don't have to. We were missing the Job Storage field from the definition of DefaultActorImpl in RuntimeInternals.h, fix that so we actually read the right data.
rdar://88598003