pkg/proc/gdbserial: optimize gdbwire backend #3715
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aarzilli
reviewed
Apr 30, 2024
There's some failures on the rr backend, I'm working to address those now. |
|
I'm actually seeing failures already with RR on our latest tagged version with the latest RR: RR failureWe should create a separate issue to track this. In the meantime I can restrict my changes to only run when we have a debugserver backend (non-rr). We really need a builder for RR but that is kind of difficult due to limitations around RR in virtualized environments. |
|
Created #3717 for RR failures not related to this PR. |
aarzilli
reviewed
May 3, 2024
pkg/proc/gdbserial/gdbserver.go
Outdated
| if jstopInfo != nil { | ||
| _, shouldQueryStopInfo = jstopInfo[th.ID] | ||
| } | ||
| if shouldQueryStopInfo { |
There was a problem hiding this comment.
I don't get this change. If we have a jsonstop record for the thread we do a second query to get a stock packet for the thread? We do this regardless of whether we know if the backend supports the qThreadStopInfo packet?
There was a problem hiding this comment.
Yeah, as of now I'm only limiting the number of stopInfo packets we send to the threads listed in jstopinfo. That way we don't issue stop info packets for every thread, every time, only the ones we know are in some interesting state.
There is a further optimization which I've noted in the TODO comment where we use only the jstopinfo packet when present and avoid sending any stopInfo packets at all. I've held off on doing that in this PR as the change will be more involved and I didn't want to add too much noise to this patch, so I'm going to send that in as a follow up.
There was a problem hiding this comment.
However I did fix the issue with not taking support of qThreadStopInfo into account.
This change optimizes the gdbwire backend by reducing the number of round trips we have to make to debugserver. It does this by using the jstopinfo packet to only query threads which we know to have a stop reason, and it also uses the registers returned by the 'T' packet to avoid issuing a bunch of 'p' packets to get the register values.
derekparker
force-pushed
the
wip/optimize-gdbwire
branch
from
07f217f
to
a73e62d
Compare
aarzilli
approved these changes
May 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change optimizes the gdbwire backend by reducing the number of
round trips we have to make to debugserver. It does this by using the
jstopinfo packet to only query threads which we know to have a stop
reason, and it also uses the registers returned by the 'T' packet
to avoid issuing a bunch of 'p' packets to get the register values.