[lldb] Cache Task pointer location and Task names #10894
Conversation
felipepiovezan
force-pushed
the
felipe/cache_tls
branch
4 times, most recently
from
4d4f808 to
5da29ab
Compare
|
@swift-ci test |
|
|
||
| if (auto it = m_tid_to_task_addr_location.find(real_thread.GetID()); | ||
| it != m_tid_to_task_addr_location.end()) { | ||
| #ifndef NDEBUG |
There was a problem hiding this comment.
The asserts look unlikely to break, if they're going to make asserts builds slow, should this be #ifdef EXPENSIVE_CHECKS instead?
There was a problem hiding this comment.
The asserts look unlikely to break
The uncertainty here is sort of why we thought of putting the assert: can we truly cache TLS? Nobody I talked to was able to give me a definitive answer, and experimenting failed to produce counter examples... Looking at the code of libpthread also did not produce a good definitive answer :/
they're going to make asserts builds slow,
Sort of! They will be "slow" for slow connections only. On a wired connection, the slowdown we're talking about is a step operation going from 200ms to 300ms.
Completely disabling the Task plugin would make the step be about 100ms.
| #endif | ||
| return ReadTaskAddr(it->second, *thread.GetProcess()); |
There was a problem hiding this comment.
if this read fails, should the cache entry be evicted?
felipepiovezan
force-pushed
the
felipe/cache_tls
branch
from
5da29ab to
d5792ea
Compare
| addr_t task_addr = process.ReadPointerFromMemory(task_addr_location, status); | ||
| if (status.Fail()) | ||
| return llvm::createStringError("could not get current task from thread: %s", | ||
| status.AsCString()); |
There was a problem hiding this comment.
A Status is an llvm::Error, so this should be
llvm::joinErrors(llvm::createStringError("could not get current task from thread"), status.takeError())
| Status status; | ||
| addr_t task_addr = process.ReadPointerFromMemory(task_addr_location, status); | ||
| if (status.Fail()) | ||
| return llvm::createStringError("could not get current task from thread: %s", |
There was a problem hiding this comment.
A Status is an llvm::Error, so this should be
| return llvm::createStringError("could not get current task from thread: %s", | |
| llvm::joinErrors(llvm::createStringError("could not get current task from thread"), status.takeError()) |
lldb/include/lldb/Target/Target.h
Outdated
| @@ -201,6 +201,8 @@ class TargetProperties : public Properties { | |||
|
|
|||
| bool GetSwiftUseTasksPlugin() const; | |||
|
|
|||
| bool GetSwiftCacheTaskPtrLocation() const; | |||
There was a problem hiding this comment.
| bool GetSwiftCacheTaskPtrLocation() const; | |
| bool GetSwiftCacheTaskPointerLocation() const; |
unless that's the canonical name for it
There was a problem hiding this comment.
It needs to follow the name of the setting in the td file, so we would spell "pointer" there too, which makes sense to help with the apropos command
…tTasks Obtaining the TLS from debugserver over a wired (or slower) conection was identified as a major source of slowdown after introducing OperatingSystemSwiftTasks. This is due to multiple memory reads being necessary: 1. Memory reads to obtain auxiliary information from libpthread that is then sent to the jThreadExtendedInfo packet. See "plo_pthread_tsd_base_address_offset","plo_pthread_tsd_base_offset" and "plo_pthread_tsd_entry_size". 2. A packet requesting jThreadExtendedInfo. Both 1 and 2 are done per thread, on every stop. However, there is strong evidence that we can safely cache the TLS pointer after a certain point in the thread lifetime, which is what this patch accomplishes. Caching is done at the OperatingSystemSwiftTasks level, not at the core Thread class. This is done for two reasons: 1. To minimize the surface area / risk of this change. 2. jThreadExtendedInfo can produce an invalid TLS address earlier in the thread lifetime, so we can only cache this if a memory read at that location is successful. On local testing over a wired connection, this reduces stepping time by about 100ms, from an average of 300ms.
felipepiovezan
force-pushed
the
felipe/cache_tls
branch
from
d5792ea to
15fe1c2
Compare
|
@swift-ci test |
This is a useful fallback for debugging.
…SwiftTasks Computing a Task name can be an expensive operation, especially over slower connections to the inferior process. To improve this situation, this patch: * Makes the computation lazy, and * Caches the result of the computation (a task name is immutable). The first point is the most important, as prior to this we were computing the task name on every stop, even when the same MemoryThread was reused between two consecutive stops. On local testing with a wired connection, this reduces stepping time by around 20-30ms, out of an average of 230ms.
felipepiovezan
force-pushed
the
felipe/cache_tls
branch
from
15fe1c2 to
1d2a6be
Compare
|
addressed review comments |
|
@swift-ci test |
This PR reduces the number of packets that need to be communicated between debugserver and LLDB when deciding whether a thread is executing a task. This is accomplished in two ways:
A) Caching the TLS location of the task pointer for each real thread.
B) Caching and lazily evaluating the name of a task
rdar://151400459