Test performance improvement

[logiclrd]

As part of #12553, there seemed to be a need to refine the performance of certain aspects of the automated test suite. The C.I. builds were set to a limit of 35 minutes, and with the Test Isolation changes, they were hitting that limit and failing. As a result, that PR contains code that makes the clean-up of Reference Repositories used in testing asynchronous, and also, ultimately, shuts it off entirely for C.I. builds. Between that and an increased limit of 45 minutes for C.I. builds, that PR is no longer encountering timeouts (most of the time). However, I thought of another possibility and went off on a tangent about optimizing from the other end: making the initialization of the repositories asynchronous.

I am copy/pasting that part of the discussion here, since I think it's out of scope for that PR.

[logiclrd]

This is a section of a visual representation of profiling data from a test run:

In this representation, a row highlighted in red shows the set-up for a given test, and the unhighlighted row directly beneath it is the corresponding clean-up. The black segment represents the span of time (on the X axis) that that operation was measured to take. You can see init-test-cleanup repeated over and over, with each subsequent init taking place with essentially no delay from the previous clean-up. But you can also clearly see that the init phases (red) take more time than the cleanup phases (white).

[logiclrd]

A possibility for improving the initialization speed: What if each instantiation of ReferenceRepository actually kicked off the next instance's initialization, so that it could return immediately with the initialization the preceding call to the constructor started?

Rough sketch:

public class ReferenceRepository
{
  static Task<GitModuleTestHelper> s_nextGitModule = BeginInitialize(false);
  static Task<GitModuleTestHelper> s_nextGitModuleWithCommit = BeginInitialize(true);

  GitModuleTestHelper _moduleTestHelper;

  public ReferenceRepository(bool createCommit)
  {
    if (createCommit)
      _moduleTestHelper = ClaimNext(ref s_nextGitModuleWithCommit, true);
    else
      _moduleTestHelper = ClaimNext(ref s_nextGitModule, false);
  }

  GitModuleTestHelper ClaimNext(ref Task<GitModuleTestHelper> initializer, bool createCommit)
  {
    var module = initializer.Result;

    initializer = BeginInitialize(createCommit);

    return module;
  }

  Task<GitModuleTestHelper> BeginInitialize(bool createCommit)
  {
    return Task.Run(
      () =>
      {
        var module = new GitModuleTestHelper();

        if (createCommit)
          CreateCommit("A commit message", "A");

        return module;
      });
  }

  ...
}

[logiclrd]

In proofing this out, I'm encountering some difficulty with the fact that the initialization of GitModuleTestHelper seems to include things that expect ThreadHelper.JoinableThreadContext to be set, but because the initialization is asynchronous, it can end up intersecting with the space between tests and find ThreadHelper.JoinableThreadContext == null. I think that's what's going on, anyway. I encountered a lot of NullReferenceExceptions inside the ThreadHelper.JoinableThreadFactory accessor because _taskManager == null.

[mstv]

Setting ThreadHelper.JoinableThreadContext and creating ReferenceRepo could be synchronized by means of a new Attribute implementing ITestAction which synchronizes / wraps ConfigureJoinableTaskFactoryAttribute and ReferenceRepo.
Furthermore, a ReferenceRepo template folder could be created once and then be cloned on file-system level.

[logiclrd]

Question: The approach here involves, by design, classes being started in one place and awaited in another. This generates warning VSTHRD003, which is specifically about tasks being started in one context and awaited in another.

I don't think that's actually a risk in this case, because there's no synchronization context involved and, in any case, the tasks are configured to not continue on captured context anyway.

As sketched earlier, ReferenceRepository fires up two background tasks for initializing repositories, one with a commit and one without, so that whatever a test needs later can be satisfied immediately. During shutdown, it is necessary to cancel these, which involves collecting their GitModuleTestHelper objects and Disposeing them.

This is what I've got to do this right now:

            Task<(GitModuleTestHelper Helper, string? CommitHash)> next = null;
            Task<(GitModuleTestHelper Helper, string? CommitHash)> nextWithCommit = null;

            lock (NextSync)
            {
                if (NextGitModuleTestHelper != null)
                {
                    next = NextGitModuleTestHelper;
                    NextGitModuleTestHelper = null;
                }

                if (NextGitModuleTestHelperWithCommit != null)
                {
                    nextWithCommit = NextGitModuleTestHelperWithCommit;
                    NextGitModuleTestHelperWithCommit = null;
                }
            }

            TaskManager mgr = new(new JoinableTaskContext());

            mgr.JoinableTaskFactory.Run(
                async () =>
                {
                    if (next != null)
                    {
                        (await next.ConfigureAwait(false)).Helper.Dispose();
                    }

                    if (nextWithCommit != null)
                    {
                        (await nextWithCommit.ConfigureAwait(false)).Helper.Dispose();
                    }
                });

Similarly, the site which actually claims one or the other "next" repository needs to just collect the Result of the corresponding Task. In most cases, the task will already have completed. This triggers VSTHRD002, which pretty much straight-up says, "Yeah don't do that, just always use await".

            GitModuleTestHelper moduleTestHelper;

            (moduleTestHelper, CommitHash) = initializer.ConfigureAwait(false).GetAwaiter().GetResult();

In order to use these, though, I need to wrap their final statements in #pragma warning disable VSTHRD002 / VSTHRD003. Is this okay? I see there handful of other places that suppress this same warning.

[mstv]

IIRC, tasks are automatically scheduled right on creation. I.e. there is no need for mgr.JoinableTaskFactory.Run(.
Instead of the unreadable ConfigureAwait(false) - what shall be false?!? -, use await TaskScheduler.Default; which is clearer at least in "Where does it come into effect?".

(moduleTestHelper, CommitHash) = initializer.ConfigureAwait(false).GetAwaiter().GetResult();
could be written as
(moduleTestHelper, CommitHash) = Task.Run(initializer);
couldn't it?