25x nonparallelized speedup by avoiding MSBuildWorkspace in example tester #1338
Conversation
…ulti-project examples
|
I'd like to add the ExampleTester and ExampleExtractor projects to the filter for the GitHub Actions .yaml that runs ExampleExtractor and ExampleTester, so that we can see how the run is affected by code changes and not just by changes under the (Update: this is handled in #1336) |
|
Also, I need to investigate a failing example. The reason I didn't investigate this example before pushing is that this example also fails for me on my local machine with the latest commit on
(Update: this is handled in #1336) |
CS1661 is listed twice in the Expected list, otherwise the lists are the same – cause of problem? |
|
@Nigel-Ecma Interesting, thank you. Sounds likely to be a change in compiler behavior. Investigating at #1336 (comment), since I realized that is the PR that will first bring on the change. (Update: this is handled in #1336) |
|
Can I check, is the intention that this replaces #1337? I'd personally rather avoid parallelization unless we actually need it. |
|
I would want to keep #1337 since it allows working with each example as a unit test in addition to running via the CLI. Then, parallelization across the unit tests could be disabled if we didn't want it to be possible. What's the nature of your concern about parallelizing? |
Only that my experience is it can make life more complicated (and harder to diagnose) for very little benefit. I'll try to review both PRs in the next couple of days. |
|
I'm happy to make life easier by removing anything worrisome. 👍 |
There was a problem hiding this comment.
I'm good with this as well. Tagging @jskeet if we can review and merge offline.
|
@jskeet Would you be willing to take a look? |
| csproj.CompilationOptions); | ||
| } | ||
|
|
||
| private static readonly FrozenDictionary<string, OutputKind> SupportedOutputKinds = new KeyValuePair<string, OutputKind>[] |
There was a problem hiding this comment.
Any reason for using FrozenDictionary instead of ImmutableDictionary? Mostly out of curiosity, really. (I haven't used the frozen collections, but have used immutable collections a fair amount. From what I've just read, I suspect that most of the time I possibly should be using frozen collections...)
There was a problem hiding this comment.
Stephen Toub assures me that the performance of ImmutableDictionary is irredeemably bad 😄 My understanding is that FrozenDictionary does optimization passes to lay out the memory for super fast lookups, whereas ImmutableDictionary retains an internal structure that compromises with a need for future copy-on-update reusing substructures.
There was a problem hiding this comment.
Fascinating, thanks. Over the weekend I shall try converting everything in election209.uk from immutable to frozen. Is there any reason not to just use frozen collections everywhere, in a wholesale shift from immutable, if they're "create once and then only read" (rather than using the pseudo-mutators)?
There was a problem hiding this comment.
(Oh, I see it's only dictionary and set - so I don't need to move away from ImmutableList...)
There was a problem hiding this comment.
I don't believe these is a reason not to use frozen collections in those never-update, read-many-times scenarios. I still use ImmutableDictionary plenty in places where the extra speed just doesn't matter, just for consistency. Switching over does sound potentially helpful for a web app.
ImmutableList's internal structure doesn't permit random access, yet the class has a indexer which thus performs pretty slowly if you have a loop with repeated indexer usage. Unless there will be frequent updates, I usually stick with ImmutableArray which I've been told is even more favorable than arrays for performance because there's no variance checking needed.
Running ExampleTester on my machine takes 8 minutes. With the changes in this PR, it takes 19 seconds. That's a 25x speedup!
That's not even parallelizing- parallelized together with #1337, the entire suite of example tests runs in 8 seconds.
The goal is twofold:
This PR speeds up all tests except the six or so multi-csproj examples which include project references. The way it speeds up the tests is by reading and interpreting the csproj XML directly. Hopefully, the new code is straightforward enough to extend as needed.
It's important to preserve the fidelity of the testing. Because of this, the fast path will bail if it sees any csproj element that it does not fully understand, falling back to the existing MSBuildWorkspace code which invokes full MSBuild out-of-process. When an example starts using an unrecognized csproj property or item, the tester will quietly fall back to the slow path without logging any notices about this. If you're running the tester with a debugger attached which has opted in to stopping on handled exceptions, you'll hit a handled NotImplementedException that will be the only clue that you're falling back to the slow path.