Replace RAR cache BinaryFormatter serialization with Bond #3868
Conversation
| /// <summary> | ||
| /// Implements IInputStream on top of System.Stream | ||
| /// </summary> | ||
| internal class InputStream : InputBuffer |
There was a problem hiding this comment.
Bond only exposes its OutputStream/InputStream wrappers on .NET Framework since internally it depends on a .NET Framework-only unsafe assembly, requiring you to either write your own, or... copy-pasta it. Not the greatest solution, but using the safe version of InputBuffer/OutputBuffer seems to work perfectly fine on .NET Core.
I'm also thinking "Add Bond" should be a separate PR? This is already big as it is.
|
|
||
| namespace Microsoft.Build.Tasks | ||
| { | ||
| internal class StateFileCache<T> where T : StateFileCachePayload, new() |
There was a problem hiding this comment.
Mostly a copy-pasta of StateFileBase. Know it's not ideal to have these two essentially serving the same purpose, but the only way to remove that class is to convert other deriving classes to Bond. Not sure if that's worth it.
| @@ -344,6 +345,8 @@ public static BuildManager DefaultBuildManager | |||
| /// <exception cref="InvalidOperationException">Thrown if a build is already in progress.</exception> | |||
| public void BeginBuild(BuildParameters parameters) | |||
| { | |||
| System.Threading.Tasks.Task.Run(() => { ResolveAssemblyReference.InitializeSerializers(); }); | |||
There was a problem hiding this comment.
Hack to ensure this executes early enough in the perf tests. Need a better location to asynchronously initialize the static Bond Serializer/Deserializer, and early enough to finish init'ing before RAR executes.
| { | ||
| internal sealed partial class SystemState | ||
| { | ||
| private class SystemStateCachePayloadExtractor |
There was a problem hiding this comment.
Not sure if I like this Hydrator/Extractor naming scheme
src/Build/Microsoft.Build.csproj
Outdated
| @@ -28,6 +28,7 @@ | |||
|
|
|||
| <ItemGroup> | |||
| <ProjectReference Include="..\Framework\Microsoft.Build.Framework.csproj" /> | |||
| <ProjectReference Include="..\Tasks\Microsoft.Build.Tasks.csproj" /> | |||
There was a problem hiding this comment.
Adding this dependency seems suspicious. My understanding of the projects are:
- Microsoft.Build.Framework - Interfaces and classes shared by MSBuild and Tasks
- Microsoft.Build - Core build logic. Depends only on Framework
- Microsoft.Build.Tasks - Various tasks. Depends only on Framework
- Microsoft.Build.Tasks.Utilities - Optional helpful utilities for task writers
Adding this dependency seems to break that pattern.
There was a problem hiding this comment.
I definitely agree, this was just a quick hack me and @cdmihai came up with for perf tests to make sure the serialization initializers are called early enough.
There was a problem hiding this comment.
Yeah. A lot of the "shared" code which is compiled into multiple assemblies is to avoid exactly this issue :( We'll need to look at a way to avoid this. And clearly we can't have this as "shared" code because it would then have to be initialized per assembly.
|
Well this is definitely a head scratcher. Debugging why parallel builds via is omitted from some command line args passed into Csc during an incremental parallel build (first build always succeeds), but not a single-proc build. And it's always omitted from exactly 5 different projects, which change every build. And a failed multi-proc build causes all following single-proc builds to fail. And subsequent single-proc builds still shuffle exactly 5 random projects to omit Then just replacing https://github.com/Microsoft/msbuild/blob/e2e670bbe25a8979efdc96f0deb64d08019e82f3/src/Tasks/StateFileCache.cs#L40-L42 and https://github.com/Microsoft/msbuild/blob/e2e670bbe25a8979efdc96f0deb64d08019e82f3/src/Tasks/StateFileCache.cs#L86-L89 with |
This reverts commit e2e670b.
|
As with the RAR-as-a-service prototype, let's close this since we haven't been able to land it yet. Hopefully we can come back to it when chasing #2015 again in the future. |
The SystemState cache in
ResolveAssemblyReferencecurrently uses the built-in .NET BinaryFormatter to serialize the cache to disk. The serialization format produced byBinaryFormatteris:This modifies
SystemStateto serialize it's cache via Bond. Note the other caches deriving from StateFileBase have not been touched; I don't seem to have a repo that utilizes these and I don't think they're as important. However, those classes much simpler data structures thanSystemStateso converting them to the new serialization would be fairly straightforward.Perf trace of '''ResolveAssemblyReference''' (left is this PR, right is master):
Specifically the deserialization times here:
Just for completeness these are the results I get from running the perf suite, but I'm assuming these all start from a clean run since the numbers don't change. Especially since, as seen above, I get pretty massive gains locally.
DesignTimeBuild Time (ms) (src\msbuild.A vs src\msbuild.chcasta)
Tried to break up the commits as best as possible, but some notes since overall this is a fairly large PR (although much of it is just boilerplate/copy-paste):
Some Bond code is copied in the first commit. Bond only exposes its
OutputStream/InputStreamwrappers on .NET Framework since internally it depends on a .NET Framework-only unsafe assembly, requiring you to either write your own, or... copy-pasta it. Not the greatest solution, but using the safe version of that assembly seems to work perfectly fine on .NET Core.Bond is much stricter about what you're allowed serialize into, and doesn't really have a mechanism for more complex type mappings. The result is... a ton of ugly conversion boilerplate.
This guy is mostly a copy-pasta of StateFileBase. Know it's not ideal to have these two essentially serving the same purpose.
Need a better location to asynchronously initialize the static Bond Serializer/Deserializer, and early enough to finish init'ing before RAR executes. Last commit was a bit of a hack to get it working in the perf tests.