Re: [PATCH 12/12] gdb: use two displaced step buffers on amd64/Linux

[Pedro Alves <pedro@palves.net>]

On 11/26/20 9:43 PM, Simon Marchi wrote:
> On 2020-11-25 3:56 p.m., Simon Marchi wrote:
>> On 2020-11-25 3:07 p.m., Simon Marchi wrote:
>>> We still have to figure out why the performance regresses compared to
>>> master.  One thing is that we still do an extra resume attempt each
>>> time.  We do one resume where the disp step prepare succeeds and one
>>> more where it fails.  That's twice as much work as before, where we did
>>> one successful prepare and then skipped the next ones.  I'll make an
>>> experimental change such that the arch can say "the prepare worked, but
>>> now I'm out of buffer", just to see how that improves (or not)
>>> performance.
>>
>> That made just a small difference.  Let's call this new patch E:
>>
>>  #9 + A + B + C + D + E: 105,727
> 
> Ok, I played a bit more with this and found a case where we do more
> work that may explain the difference.
> 
> handle_signal_stop calls finish_step_over, which calls start_step_over.
> Let's say that the displaced step buffer is used by some thread and
> another thread either hits the breakpoint, we go into start_step_over to
> see if we can start a new step over.  We can't, because the event thread
> isn't the one that is using the displaced stepping buffer, the buffer is
> still occupied.
> 
> In the pre-patch code, that early check in start_step_over:
> 
>       /* If this inferior already has a displaced step in process,
> 	 don't start a new one.  */
>       if (displaced_step_in_progress (tp->inf))
> 	continue;
> 
> means that we won't even try one resumption, we'll skip over all threads
> in the chain (the only cost is the cost of iteration).
> 
> In the patch I previously called "C", start_step_over starts with the
> assumption that the inferior has an available buffer, and needs to do at
> least one resumption attempt that returns "unavailable" before it
> assumes there are no more available.  That one unnecessary resumption
> attempt happens over and over and ends up being costly.
> 
> I changed my patch C to make the "unavailable" property persistent,
> instead of just using it locally in start_step_over.  If a displaced
> step prepare returns "unavailable", we mark that this inferior has no
> more buffers available, and only reset this flag when a displaced step
> for this inferior finishes.  Doing so, when a thread hits the breakpoint
> and start_step_over is called, the unavailable flag is already set.
> Last time we tried to prepare a displaced step, there were no buffers
> available.  And if no displaced step finished for this inferior since,
> then why bother asking?  As a result, we don't even try one resume if
> the buffers are already take, just like with the pre-patch code.
> 
> I re-did some tests with the new patch C:
> 
> master:             110,736
> 9:                   19,973
> 9 + A + B + C:      107,711
> 9 + A + B + C + E:  111,135
> Everything up to
>  using two displaced
>  stepping buffers:  110,697
> 
> Note that I dropped patch D, which was to break out of the
> start_step_over loop when getting "unavailable".  It didn't help much
> and wasn't desired functionality anyway.
> 
> And for the reminder, patch E is that when returning "_OK", the arch can
> also tell us "that was the last one, any more request would return
> unavailable".  So that allows to do just one resumption attempt (at
> most) per start_step_over invocation, rather than 2.
> 
> What this shows is that by using everything up to patch E, we are pretty
> much on par with the performance, but with the more flexible
> architecture.  That makes sense, as (I think) we do pretty much the same
> work.
> 
> Re-thinking about patch C and E, I'm thinking that we could let the
> architecture's displaced stepping implementation decide when it sets the
> "unavailable" flag, instead of the core of GDB setting it.  The
> architecture would then choose the policy it wants to adopt:
> 
> - set the flag when it used its last buffer (equivalent to patch E)
> - set the flag when it returns "_UNAVAILABLE" (equivalent to patch C')
> - never set the unavavaible flag (equivalent to patch 9)
> 
> It also shows that using two buffers on x86-64 doesn't improve
> performance (unless there's another unfortunate bottleneck I haven't
> found).  But that's what I expected.  My hypothesis is that the actual
> step portion of the displaced step is so fast that we never really use
> the buffers in parallel.  If we start two displaced steps, by the time
> we prepare the second one, the first one has already finished and is
> waiting for GDB to clean it up.  If for some reason we could displaced
> step an instruction that took, say, 1 second to execute, then having
> many displaced step buffers would be useful, because GDB would be able
> to start them all and have them really run in parallel.
> 
> I'll prepare a user branch with all those fixup patches so you can take
> a look, and if you think it makes sense I'll be ready for a v2.

I took a look and your branch, and it makes sense to me.
I run the perf test on my end, and the performance gap is practically
gone on my end as well.  I'm happy with that.