Enable cache for RAR #2792

AndyGerlicher · 2017-12-09T23:06:24Z

Preliminary numbers on a basic netcoreapp2.0 -> netstandard2.0 library sln:

Tip of exp/update-toolset branch rebuild:

      439 ms  ResolveAssemblyReferences                  2 calls
      437 ms  ResolveAssemblyReference                   2 calls

This branch with RAR *.Core.cache file:

      238 ms  ResolveAssemblyReferences                  2 calls
      237 ms  ResolveAssemblyReference                   2 calls

This branch after deleting *.Core.cache:

      312 ms  ResolveAssemblyReferences                  2 calls
      311 ms  ResolveAssemblyReference                   2 calls

So even without the cache file it's still after with binary serialization turned on. Not sure why (there's a lot of code that differs). Also haven't done anything other than manual testing.

cdmihai · 2017-12-12T18:52:34Z

src/Directory.BeforeCommon.targets

@@ -10,7 +10,7 @@
  </PropertyGroup>

  <PropertyGroup>
-    <DefineConstants>$(DefineConstants);STANDALONEBUILD</DefineConstants>
+    <DefineConstants>$(DefineConstants);STANDALONEBUILD;FEATURE_BINARY_SERIALIZATION</DefineConstants>


nit: Remove it from the net4* pile? Or is the pile intent to document what's on net4*. In which case there should be a similar pile for .net core?

cdmihai · 2017-12-12T18:55:50Z

src/Build.UnitTests/BackEnd/TaskHostConfiguration_Tests.cs

@@ -361,13 +361,8 @@ public void TestTranslationWithValueTypesInDictionary()
                1,
                Directory.GetCurrentDirectory(),
                null,
-#if FEATURE_THREAD_CULTURE


Maybe it's not worth doing this until we get the green light for removing ns13? If we can't remove ns13, we'll have to put this back. Same goes for the other removed flags.

cdmihai · 2017-12-12T19:01:27Z

src/Shared/AssemblyNameExtension.cs

+
+        public void GetObjectData(SerializationInfo info, StreamingContext context)
+        {
+            if (asAssemblyName != null)


Probably nice to have it sync with https://github.com/Microsoft/msbuild/blob/master/src/Shared/AssemblyUtilities.cs#L62

cdmihai · 2017-12-12T19:06:30Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -2012,7 +2012,8 @@ Copyright (C) Microsoft Corporation. All rights reserved.
        expensive to write the newly created cache file.
        -->
    <PropertyGroup>
-      <ResolveAssemblyReferencesStateFile Condition="'$(BuildingProject)'=='true'">$(IntermediateOutputPath)$(MSBuildProjectFile)ResolveAssemblyReference.cache</ResolveAssemblyReferencesStateFile>
+      <ResolveAssemblyReferencesStateFile Condition="'$(BuildingProject)'=='true' and '$(MSBuildRuntimeType)' != 'Core'">$(IntermediateOutputPath)$(MSBuildProjectFile)ResolveAssemblyReference.cache</ResolveAssemblyReferencesStateFile>


It would be nice to leave a comment saying why there's two

AndyGerlicher · 2017-12-14T23:55:24Z

https://github.com/dotnet/corefx/issues/25930

danmoseley · 2017-12-15T03:18:19Z

src/Shared/AssemblyNameExtension.cs

+        {
+            if (asAssemblyName != null)
+            {
+                info.AddValue("asAssemblyName", true);


It probably doesn't matter a lot but these could just be something like "AN" not "asAssemblyName" which adds up, with 100 assembly names that's about 15KB of extra writes, 30KB if it's written out as UTF16 (not sure).

For similar reasons you might consider "interning" some well known values like the 3 or 4 common PK and PKT almost all our assemblies have -- writing out a marker for those instead would save a lot of bytes if you write out many of these assembly names.

danmoseley · 2017-12-15T03:20:09Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -2012,7 +2012,9 @@ Copyright (C) Microsoft Corporation. All rights reserved.
        expensive to write the newly created cache file.
        -->
    <PropertyGroup>
-      <ResolveAssemblyReferencesStateFile Condition="'$(BuildingProject)'=='true'">$(IntermediateOutputPath)$(MSBuildProjectFile)ResolveAssemblyReference.cache</ResolveAssemblyReferencesStateFile>
+      <!-- Note: Binary serialization not guaranteed to be consistent across full framework and .NET Core, so we need to use a different cache file. -->


.NET Framework and .NET Core do promise to interchange serialized data (if the type is binary serializable in both). What you mean is your special AssemblyName serializatoin scheme is not shared by older MSBuild's.

This is only necessary if you expect both MSBuild's to build the same projectwith the same obj folder. Neither side crash if they can't deserialize, they just warn and overwrite it.

danmoseley · 2017-12-15T03:24:02Z

src/Shared/AssemblyNameExtension.cs

+            var hasAsAssemblyName = info.GetBoolean("asAssemblyName");
+            if (hasAsAssemblyName)
+            {
+                var name = info.GetString("asAssemblyName.Name");


hopefully there are unit tests that would fail if there was a mistake in the serializatoin/deserialization? it's very easy to have a tpyo or suchlike.

AndyGerlicher · 2018-02-14T01:06:14Z

@dotnet-bot test Ubuntu16.04 Build for CoreCLR please

AndyGerlicher · 2018-02-14T17:32:11Z

All the tests are green here, ready for final review.

AndyGerlicher · 2018-02-14T18:49:29Z

One thing to note I removed the split to use a different cache for the Core version. I don't think we need that, but I could be wrong. They should cache the same things and the serialization changes to support Core are for both (I updated the format version).

AndyGerlicher · 2018-03-07T04:20:20Z

@dotnet-bot test Windows_NT Build for Full please

AndyGerlicher · 2018-03-07T17:45:45Z

@dotnet-bot test Windows_NT Build for Full please

Some types are dependent on WinForms

* Enable FEATURE_BINARY_SERIALIZATION for .NET Core build. * Add custom serialization for types not marked Serializable in .NET Core.

If a custom build event does not implement a serialization method, the timestamp field may get serialized as an Unspecified kind. To fix this, EventArgs will now always serialize Utc format and not convert when observed. If the event kind is Unspecified, assume it's UTC and convert when the property is accessed.

Renaming the file to avoid issues with breaking changes between 15.6 and 15.7. This is generally not an issue, but 15.6 and below will warn with a cryptic error message. If users (esp. internal users) switch back and forth in the same repo this would result in many harmless but concerning warnings.

This was used in binary serialization and supported on .NET Core 2.0.

danmoseley · 2018-03-08T23:57:38Z

Yay!

dnfclas added the cla-already-signed label Dec 9, 2017

msftgits removed the cla-already-signed label Dec 11, 2017

cdmihai reviewed Dec 12, 2017

View reviewed changes

AndyGerlicher force-pushed the binaryserialization branch from 3f701d1 to 6712ca1 Compare December 13, 2017 17:13

danmoseley reviewed Dec 15, 2017

View reviewed changes

AndyGerlicher force-pushed the binaryserialization branch from 6712ca1 to 2add5c2 Compare December 20, 2017 19:09

AndyGerlicher force-pushed the exp/update-toolset branch from e6cfe84 to c84668d Compare January 12, 2018 23:46

dsplaisted force-pushed the exp/update-toolset branch from c84668d to e35d71b Compare January 14, 2018 18:30

AndyGerlicher changed the base branch from exp/update-toolset to master February 2, 2018 21:05

AndyGerlicher force-pushed the binaryserialization branch from 2add5c2 to e1b491c Compare February 2, 2018 21:06

AndyGerlicher force-pushed the binaryserialization branch 3 times, most recently from b51b7e1 to 16c640c Compare February 13, 2018 23:45

AndyGerlicher changed the title ~~WIP: Enable cache for RAR~~ Enable cache for RAR Feb 14, 2018

jeffkl approved these changes Feb 14, 2018

View reviewed changes

AndyGerlicher force-pushed the binaryserialization branch from 16c640c to 76356ff Compare February 14, 2018 17:43

AndyGerlicher mentioned this pull request Feb 14, 2018

Remove dead code for binary serialization #2995

Merged

AndyGerlicher force-pushed the binaryserialization branch from 76356ff to 6dec148 Compare March 6, 2018 23:53

AndyGerlicher force-pushed the binaryserialization branch from 4046222 to 7f07bb3 Compare March 7, 2018 18:11

AndyGerlicher force-pushed the binaryserialization branch 5 times, most recently from 0c37aeb to db5b029 Compare March 8, 2018 19:40

AndyGerlicher added 8 commits March 8, 2018 13:47

Move ResGen cache to separate feature flag

4600a9d

Some types are dependent on WinForms

Enable cache for RAR

2cf0140

* Enable FEATURE_BINARY_SERIALIZATION for .NET Core build. * Add custom serialization for types not marked Serializable in .NET Core.

Fix intermittent unit test issue

f8330fd

Enable diagnosticMessages for xunit

f868821

Enable FEATURE_MEMORYSTREAM_GETBUFFER on Core

87f502d

This was used in binary serialization and supported on .NET Core 2.0.

Refactor some binary serialization code

9d88e82

AndyGerlicher force-pushed the binaryserialization branch 2 times, most recently from 8e8014b to 9d88e82 Compare March 8, 2018 23:53

AndyGerlicher merged commit 4037f06 into dotnet:master Mar 8, 2018

AndyGerlicher deleted the binaryserialization branch June 26, 2018 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable cache for RAR #2792

Enable cache for RAR #2792

AndyGerlicher commented Dec 9, 2017

cdmihai Dec 12, 2017

cdmihai Dec 12, 2017 •

edited

Loading

cdmihai Dec 12, 2017

cdmihai Dec 12, 2017

AndyGerlicher commented Dec 14, 2017

danmoseley Dec 15, 2017

danmoseley Dec 15, 2017

danmoseley Dec 15, 2017

danmoseley Dec 15, 2017

AndyGerlicher commented Feb 14, 2018

AndyGerlicher commented Feb 14, 2018

AndyGerlicher commented Feb 14, 2018

AndyGerlicher commented Mar 7, 2018

AndyGerlicher commented Mar 7, 2018

danmoseley commented Mar 8, 2018

Enable cache for RAR #2792

Enable cache for RAR #2792

Conversation

AndyGerlicher commented Dec 9, 2017

cdmihai Dec 12, 2017

Choose a reason for hiding this comment

cdmihai Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

cdmihai Dec 12, 2017

Choose a reason for hiding this comment

cdmihai Dec 12, 2017

Choose a reason for hiding this comment

AndyGerlicher commented Dec 14, 2017

danmoseley Dec 15, 2017

Choose a reason for hiding this comment

danmoseley Dec 15, 2017

Choose a reason for hiding this comment

danmoseley Dec 15, 2017

Choose a reason for hiding this comment

danmoseley Dec 15, 2017

Choose a reason for hiding this comment

AndyGerlicher commented Feb 14, 2018

AndyGerlicher commented Feb 14, 2018

AndyGerlicher commented Feb 14, 2018

AndyGerlicher commented Mar 7, 2018

AndyGerlicher commented Mar 7, 2018

danmoseley commented Mar 8, 2018

cdmihai Dec 12, 2017 •

edited

Loading