[lldb][NFC] Prepare SwiftLanguageRuntime for vectorized memory reads #11592
Conversation
felipepiovezan
force-pushed
the
felipe/swift_prepare1
branch
from
56279ac to
7458f53
Compare
|
@swift-ci test |
|
|
||
| /// Helper function to read all `pointers` from process memory at once. | ||
| /// Consumes any errors from the input by propagating them to the output. | ||
| static llvm::SmallVector<llvm::Expected<addr_t>> MultiReadPointers( |
There was a problem hiding this comment.
How likely do you think will we hit the "small" case here? Would using a std::vector + .reserve() get us the same performance with less overhead?
There was a problem hiding this comment.
Storing llvm::Error in an array also makes me uncomfortable. How do we guarantee we check them all? Is it likely that only some of them fail? Is that actionable?
Otherwise I'd return an Expected<vector> and join all errors inside the function.
There was a problem hiding this comment.
How likely do you think will we hit the "small" case here?
The default size is 4 for this type: https://godbolt.org/z/1nbbM5Y7s
This is generally the number of threads I see when running apps in some scenarios.
Would using a std::vector + .reserve() get us the same performance with less overhead?
I believe this is a misconception: the different "small size" does not produce any overhead, it's just a different stack pointer offset.
How do we guarantee we check them all?
The same way we guarantee a single Expected is checked. Why do you believe a vector is different?
Is it likely that only some of them fail?
Very likely! Early in the thread lifetime, LLDB will get bogus addresses for its TLSs, and the memory reads will fail for those.
Is that actionable?
Not only is it actionable, but we have to act on it (by reporting the thread is not running a task).
Otherwise I'd return an Expected and join all errors inside the function.
We have to distinguish which reads fail, to know which threads are not running Tasks.
There was a problem hiding this comment.
I believe this is a misconception: the different "small size" does not produce any overhead, it's just a different stack pointer offset.
The overhead I meant was the copying that happens when we exceed the small size.
As for the Expecteds — if we can deal with partial successes in meaningful way, I have no objections to this!
There was a problem hiding this comment.
Actually — does growing (and thus copying the elements) even work with the elements being llvm::Expected? Does it do a std::move()?
There was a problem hiding this comment.
The overhead I meant was the copying that happens when we exceed the small size.
Ah, I see. We can indeed call reserve here to avoid copying, even with SmallVectors. I will add a call to that!
does growing (and thus copying the elements) even work with the elements being llvm::Expected? Does it do a std::move()?
Yup, all STL and STL-like containers always move elements when growing (otherwise we wouldn't be able to have containers of move-only types like unique_ptr)
felipepiovezan
force-pushed
the
felipe/swift_prepare1
branch
from
7458f53 to
89348bf
Compare
|
Addressed review feedback |
felipepiovezan
force-pushed
the
felipe/swift_prepare1
branch
2 times, most recently
from
2404196 to
7afd989
Compare
|
After talking to Jim, and based on what the expected (pun intended) API for the new "ReadPointersFromMemory" will look like (upcoming patch upstream), and based on Adrian's feedback, I have moved away from a vector of Expected's and am now using a vector of optionals. Some error reporting had to change a bit, but I think for the better. |
Once the proposal [1] is implemented, the Process class will have the ability to read multiple memory addresses at once. This patch is an NFC refactor to make the code more amenable to that future. [1]: https://discourse.llvm.org/t/rfc-a-new-vectorized-memory-read-packet/
felipepiovezan
force-pushed
the
felipe/swift_prepare1
branch
from
7afd989 to
fb7c49e
Compare
2 participants
Once the proposal 1 is implemented, the Process class will have the ability to read multiple memory addresses at once. This patch is an NFC refactor to make the code more amenable to that future.