Coretype variables renamed back to netfx counterpart for reflection based serialization #11910

ViktorHofer · 2017-05-25T13:23:08Z

Relates to https://github.com/dotnet/corefx/issues/19119

This PR will be merged into the 2.0 branch. See issue above for more details.

jkotas · 2017-05-25T13:39:30Z

src/mscorlib/shared/System/Char.cs

@@ -26,7 +26,7 @@ public struct Char : IComparable, IComparable<Char>, IEquatable<Char>, IConverti
        //
        // Member Variables
        //
-        private char _value;
+        private char m_value; // Do not rename (binary serialization)


I do not think it is necessary to rename these to allow binary serialization.

Binary serializers have to treat primitive types like char or byte in special way. They cannot depend on the fields in them. Otherwise, they would end up with infinite recursion.

Do you have a case where the current names break something?

The primitives shouldn't need renames since the serialization format special-cases them. The other changes should still be necessary.

We were hitting a few issues and after the rename they were gone. Could be related to something else, needs more investigation. Due to the fact that we need to finish this for 2.0 we (@morganbr @danmosemsft @stephentoub @krwq) decided to keep the changes on the Primitives.

Could you please share details about the issues that you are hitting that are "fixed" by this?

I will follow up on your question later...

krwq · 2017-05-25T22:44:34Z

@stephentoub I've made the EqualityComparers serialize like desktop PTAL. cc: @ViktorHofer

stephentoub · 2017-05-25T22:56:50Z

src/mscorlib/src/System/Collections/Generic/EqualityComparer.cs

@@ -401,6 +402,14 @@ internal override int LastIndexOf(T[] array, T value, int startIndex, int count)
            }
            return -1;
        }
+
+        public void GetObjectData(SerializationInfo info, StreamingContext context)


The type needs to implement ISerializable as well, no? Same for the other one. Did this change alone make the tests pass?

yes, I was just going to send patch with this 😉

Good catch @krwq!
cc @morganbr

@ViktorHofer thank @stephentoub 😉

jkotas · 2017-05-26T16:04:46Z

src/mscorlib/src/System/Collections/Hashtable.cs

@@ -66,6 +66,7 @@ namespace System.Collections
    // 
    [DebuggerTypeProxy(typeof(System.Collections.Hashtable.HashtableDebugView))]
    [DebuggerDisplay("Count = {Count}")]
+    [Serializable]


This is not going to work because of it will conflict with public Hashtable in corefx. When deserializing payload from desktop, the binary serializer will not know whether it needs to create this internal Hashtable or public Hashtable in corefx.

What is this needed for? If it is really needed, I think it needs to be fixed by moving the Hashtable from corefx to corelib.

More generally, are we actually exposing this Hashtable from any APIs, e.g. are we returning this from public APIs that return IDictionary? If so, we probably want to move the "real" Hashtable to CoreCLR anyway, to handle existing code that casts the IDictionary to Hashtable.

Yes we are in at least a few cases I just found out. After my build is finished I can tell you the concrete types that are using it.

More generally, are we actually exposing this Hashtable from any APIs, e.g. are we returning this from APIs that return IDictionary?

We have been engineering around this problem (e.g. Environment.GetVariables). I agree that we should finally bite the bullet and move Hashtable to CoreLib if there is any reason for it.

Is there anything I need to watch out for when moving it from corefx to coreclr?

TimeZone is currently not serializable because of it: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/CurrentSystemTimeZone.Cache.cs#L33

How is it possible that StringDictionary which lives in corefx uses the internal Hashtable from coreclr?
https://github.com/dotnet/corefx/blob/master/src/System.Collections.Specialized/src/System/Collections/Specialized/StringDictionary.cs#L21

Deserialization of type System.Collections.Specialized.StringDictionary failed System.Runtime.Serialization.Formatters.Tests.BinaryFormatterTests.ValidateTfmHashes(original: [System.Collections.DictionaryEntry, System.Collections.DictionaryEntry], tfmBase64Hashes: [\"AAEAAAD/////AQAAAAAAAAAMAgAAAGFTeXN0ZW0uQ29sbGVjdG\"..., \"AAEAAAD/////AQAAAAAAAAAMAgAAAElTeXN0ZW0sIFZlcnNpb2\"...]) [FAIL] System.Runtime.Serialization.SerializationException : Type 'System.Collections.Hashtable' in Assembly 'System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e' is not marked as serializable.

Or is this already the mixup because I marked both as serializable?

StringDictionary should be using CoreFX's Hashtable.

If TimeZone is the only reason for moving HashTable, we could also drop serialization on TimeZone. When I added it to the list, I didn't realize it's obsolete. We do have data showing it used in serialization, but it's rare (~0.001%).

jkotas · 2017-05-26T16:11:23Z

The merge of master did not seem to get done right. The diff shows many unrelated changes that makes it hard to review. Could you please clean it up?

ViktorHofer · 2017-05-26T16:12:43Z

Yes will do.

ViktorHofer · 2017-05-26T16:14:17Z

done

danmoseley · 2017-05-27T23:14:58Z

src/mscorlib/src/System/Collections/Generic/EqualityComparer.cs

+
+        public void GetObjectData(SerializationInfo info, StreamingContext context)
+        {
+            // The LongEnumEqualityComparer does not exist on 4.0 so we need to serialize this comparer as ObjectEqualityComparer


is 4.0 relevant to us?

It doesn't matter. Because this code exists in 4.7, the type in 4.7 doesn't have a deserialization ctor, which means core has to do the same dance. The comment could be tweaked if desired, though.

For performance, Lazy was completely rewritten for .NET Core 2.0 and has an entirely different format than desktop; trying to get it to match the desktop serialization format would require either reverting or providing a complicated custom serialization/deserialization implementation to try to match. Lazy can also wrap an Exception that occurred from trying to instantiate the object, and the only exception types that are serializable as of now in core are the base Exception and AggregateException. As such, we're cutting it from the list of supported types in 2.0. An easy workaround is simply to do what the implementation does: serialize lazy.Value instead of lazy.

danmoseley · 2017-05-31T00:31:40Z

Merging in this state to unblock 2.0 porting. I"ll follow up with @jkotas offline and we can rename the primitive type members back if necessary.

…ased serialization (dotnet#11910) * Variables renamed for reflection based serialization * Make EqualityComparers serialize like desktop * add missing interfaces * TimeZone serializable added * Internal hashtable serializable * Removed TimeZone as serializable type * Remove Lazy<T>'s [Serializable] attribute for 2.0 For performance, Lazy was completely rewritten for .NET Core 2.0 and has an entirely different format than desktop; trying to get it to match the desktop serialization format would require either reverting or providing a complicated custom serialization/deserialization implementation to try to match. Lazy can also wrap an Exception that occurred from trying to instantiate the object, and the only exception types that are serializable as of now in core are the base Exception and AggregateException. As such, we're cutting it from the list of supported types in 2.0. An easy workaround is simply to do what the implementation does: serialize lazy.Value instead of lazy. * tiny fixes to equalitycomparer.cs

* Scale back [Serializable] on CoreCLR types (#11765) Removes SerializableAttribute from CoreCLR types not intended to be serializable as well as adding special handling to MulticastDelegate to prevent serializing delegates (which can't be correctly serialized cross platform/runtime). * ISerializable cleanup (#11873) Changes to throw PlatformNotSupportedException from ISerializable.GetObjectData and serialization constructors on non-Serializable types. Also removes private serialization constructors and some unneeded code that was used to support serializing non-serializable types. A few tests testing GetObjectData implementations are also removed. For Exception types, we call base instead of throwing from GetObjectData to be consistent with the many Exceptions that don't override GetObjectData * IDeserializationCallback cleanup Throws PlatformNotSupportedException from IDeserializationCallback and IObjectReference implementations on types that are no longer serializable. * Coretype variables renamed back to netfx counterpart for reflection based serialization (#11910) * Variables renamed for reflection based serialization * Make EqualityComparers serialize like desktop * add missing interfaces * TimeZone serializable added * Internal hashtable serializable * Removed TimeZone as serializable type * Remove Lazy<T>'s [Serializable] attribute for 2.0 For performance, Lazy was completely rewritten for .NET Core 2.0 and has an entirely different format than desktop; trying to get it to match the desktop serialization format would require either reverting or providing a complicated custom serialization/deserialization implementation to try to match. Lazy can also wrap an Exception that occurred from trying to instantiate the object, and the only exception types that are serializable as of now in core are the base Exception and AggregateException. As such, we're cutting it from the list of supported types in 2.0. An easy workaround is simply to do what the implementation does: serialize lazy.Value instead of lazy. * tiny fixes to equalitycomparer.cs * Merge pull request #11995 from ViktorHofer/SerializationFollowUp Internal hashtable serialization attribute removed * Fixes deserializaing TimeZoneInfo by putting back the IDeserializationCallback.OnDeserialization method. (#12024)

ViktorHofer added the area-Meta-corelib label May 25, 2017

ViktorHofer added this to the 2.1.0 milestone May 25, 2017

ViktorHofer self-assigned this May 25, 2017

ViktorHofer requested review from stephentoub, danmoseley and morganbr May 25, 2017 13:23

dnfclas added the cla-already-signed label May 25, 2017

stephentoub approved these changes May 25, 2017

View reviewed changes

jkotas reviewed May 25, 2017

View reviewed changes

stephentoub reviewed May 25, 2017

View reviewed changes

jkotas reviewed May 26, 2017

View reviewed changes

ViktorHofer and others added 5 commits May 26, 2017 18:13

Variables renamed for reflection based serialization

2df18de

Make EqualityComparers serialize like desktop

13d0a1f

add missing interfaces

abfe88c

TimeZone serializable added

2e5e368

Internal hashtable serializable

4f2b9ec

ViktorHofer force-pushed the SerializationTests branch from 0b82d74 to 4f2b9ec Compare May 26, 2017 16:14

danmoseley reviewed May 27, 2017

View reviewed changes

ViktorHofer and others added 3 commits May 30, 2017 18:48

Removed TimeZone as serializable type

31307c2

tiny fixes to equalitycomparer.cs

5af4dc4

danmoseley merged commit 48d5a81 into dotnet:master May 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coretype variables renamed back to netfx counterpart for reflection based serialization #11910

Coretype variables renamed back to netfx counterpart for reflection based serialization #11910

ViktorHofer commented May 25, 2017

jkotas May 25, 2017

jkotas May 25, 2017

morganbr May 25, 2017

ViktorHofer May 26, 2017

jkotas May 26, 2017

ViktorHofer May 26, 2017

krwq commented May 25, 2017

stephentoub May 25, 2017 •

edited

Loading

krwq May 25, 2017

ViktorHofer May 26, 2017

krwq May 26, 2017

jkotas May 26, 2017

stephentoub May 26, 2017 •

edited

Loading

ViktorHofer May 26, 2017

jkotas May 26, 2017

ViktorHofer May 26, 2017

ViktorHofer May 26, 2017

ViktorHofer May 26, 2017 •

edited

Loading

morganbr May 26, 2017

morganbr May 26, 2017

jkotas commented May 26, 2017

ViktorHofer commented May 26, 2017

ViktorHofer commented May 26, 2017

danmoseley May 27, 2017

stephentoub May 27, 2017

danmoseley commented May 31, 2017

Coretype variables renamed back to netfx counterpart for reflection based serialization #11910

Coretype variables renamed back to netfx counterpart for reflection based serialization #11910

Conversation

ViktorHofer commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq commented May 25, 2017

stephentoub May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented May 26, 2017

ViktorHofer commented May 26, 2017

ViktorHofer commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented May 31, 2017

stephentoub May 25, 2017 •

edited

Loading

stephentoub May 26, 2017 •

edited

Loading

ViktorHofer May 26, 2017 •

edited

Loading