Re: [PATCH v3 4/5] gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-01-11 3:28 p.m., Simon Marchi wrote:
> I see more of ~1.33 % slowdown, but it's consistent too.
> 
> It could be the std::set allocation.  I changed all_process_targets to make it
> return an array of 1 element, just to see what happens, it didn't seem to help.
> See attached patch "0001-Test-returning-something-else-than-std-set-in-all_pr.patch"
> if you want to try it.
> 
> It's maybe due to the fact that we now iterate on all threads at every handled
> event? fetch_inferior_event calls ~scoped_disable_commit_resumed, which calls
> maybe_commit_resumed_all_process_targets, which iterates on all threads.  The
> loop actually breaks when it finds a thread with a pending status, but that
> still makes this function O(number of threads).

I did some more testing regarding this.

I am testing the following scenarios:

- baseline: up to the previous patch ("gdb: move commit_resume to
  process_stratum_target") applied.
- original patch: baseline + this patch applied
- always commit resume: I take the original patch, and I remove the loop over
  all threads to check if there are resumed threads with a pending status.
  Instead, I just let it do the commit-resume all the time.
- all no-op: I make these no-ops:

    - maybe_commit_resumed_all_process_targets
    - scoped_disable_commit_resumed::scoped_disable_commit_resumed
    - scoped_disable_commit_resumed::~scoped_disable_commit_resumed

  This essentially gets rid of all calls to all_process_targets and its std::set
  allocation.  It wouldn't be a correct patch, but the native target doesn't care,
  it's just to see the impact of these functions.

The results are:

baseline: 81,534
original patch: 78,926
always commit resume: 80,416
all no-op: 80,890

I don't really understand why "all no-op" would be slower than the baseline, there
isn't much left at this point.

Still, we see, with the difference between "original patch" and "always commit
resume", that iterating on all threads to see if there's one resumed with a
pending status has a non-negligible cost.  To speed this up, we could maintain
a per target or per inferior count of "number of resumed threads with a pending
status".  I think it would be a bit tricky to get completely right, to make sure
this count never gets out of sync with the reality.

Another similar idea would be to keep a per inferior list / queue of resumed
threads with pending statuses.  That would serve the same purpose as the count
from above (checking quickly if there exists a resumed thread with pending status)
but could also make random_pending_event_thread more efficient.
random_pending_event_thread would just return the first thread in that list that
matches waiton_ptid (most of the time, that will be the first in the list because
we are looking for any thread).  It would no longer be random, but it would be
FIFO, but I think it would achieve the same goal of ensuring no thread starves
because it is never the chosen one.  There would be the same difficulty as with
the counter though, to make sure that this list gets in sync with the reality.

Simon