Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported #56604

Merged
merged 6 commits into from
Aug 3, 2021
Merged

Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported #56604

merged 6 commits into from
Aug 3, 2021

Conversation

MaximLipnin
Copy link
Contributor

Fixes #56533

jkotas
jkotas reviewed Jul 30, 2021
View reviewed changes
src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs Outdated
@@ -109,7 +110,7 @@ internal enum SerializationMode

public class XmlSerializer
{
internal static SerializationMode Mode { get; set; } = SerializationMode.ReflectionAsBackup;
internal static SerializationMode Mode { get; set; } = RuntimeFeature.IsDynamicCodeSupported ? SerializationMode.ReflectionAsBackup : SerializationMode.ReflectionOnly;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
internal static SerializationMode Mode { get; set; } = RuntimeFeature.IsDynamicCodeSupported ? SerializationMode.ReflectionAsBackup : SerializationMode.ReflectionOnly;
internal static SerializationMode Mode => RuntimeFeature.IsDynamicCodeSupported ? SerializationMode.ReflectionAsBackup : SerializationMode.ReflectionOnly;

Is it possible to return the constant directly instead of caching it in a static variable, to enable linker to trim the unreachable code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some tests checking for the Mode setter, e.g. https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs#L22-L27

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those even used? Where are these compilation constants set?

runtime/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs

Line 21 in 8190713

#if ReflectionOnly|| XMLSERIALIZERGENERATORTESTS

cc: @StephenMolloy

Copy link
Member

@stephentoub stephentoub Jul 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nevermind, there's a separate project that sets at least one of them:

runtime/src/libraries/System.Private.Xml/tests/XmlSerializer/ReflectionOnly/System.Xml.XmlSerializer.ReflectionOnly.Tests.csproj

Line 3 in 8190713

<DefineConstants>$(DefineConstants);ReflectionOnly</DefineConstants>

It'd be good at some point to find a way to test this while not negatively impacting the size of what we ship.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, at this stage of the game, I'd stick with the original proposed change that caches the value at initialization so we maintain the ability to override when needed. (Which I believe is pretty much just testing right now.) We have a desire to potentially make the reflection-based serializer the primary option in the future, but we've got work to do to bring it up to par with the ILGen serializer before we can do that. Having a switch that is not stuck in one position is helpful.

@jkotas
Copy link
Member

jkotas commented Jul 31, 2021

Revert Mode to usual property

You may be able to keep this optimization by using the same pattern as DataContractSerializer:

        private static SerializationMode s_mode = SerializationMode.ReflectionAsBackup;

        internal static SerializationMode Mode
        {
            get => RuntimeFeature.IsDynamicCodeSupported ? s_mode : SerializationMode.ReflectionOnly; }
            set => s_mode = value;
        }

@MaximLipnin
Copy link
Contributor Author

We can merge it if there are no any other comments.

@MaximLipnin MaximLipnin merged commit b3ab2eb into dotnet:main Aug 3, 2021
@MaximLipnin MaximLipnin deleted the use_refelectiononly_for_xmlserializer branch August 3, 2021 09:27
thaystg added a commit to thaystg/runtime that referenced this pull request Aug 3, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_fix_56249
c0e7fb2 
* origin/main: (64 commits)
  [wasm][debugger] Create test Inherited Properties (dotnet#56754)
  Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
  Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
  [mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
  Update windows-requirements.md (dotnet#56476)
  Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
  [wasm][debugger] Inspect static class (dotnet#56740)
  Fix stack overflow handling issue in GC stress (dotnet#56733)
  Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
  Move Windows Compat pack to NuGet pack task (dotnet#56686)
  Fix build error when building some packages (dotnet#56767)
  Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
  Fix race in crossdac publishing with PGO (dotnet#56762)
  Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
  Use ComWrappers in some Marshal unit-tests and update platform metadata  (dotnet#56595)
  Set `DisableImplicitNamespaceImports_Dotnet=true` to workaround sdk issue (dotnet#56744)
  Make sure ServerGCHeapDetails is up to date (dotnet#56056)
  [libraries] Reenable System.Diagnostics.DiagnosticSorce.Switches.Tests on mobile (dotnet#56737)
  Disable failing arm64 win10 Graphics.FromHdc tests  (dotnet#56732)
  Match xplat event source conditions (dotnet#56435)
  ...
thaystg added a commit to thaystg/runtime that referenced this pull request Aug 4, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_implement_debug…
33652d5 
…ger_proxy_attribute

* origin/main: (340 commits)
  add RID for Debian 11 (dotnet#56789)
  [wasm] [debugger] Skip thread static field (dotnet#56749)
  Fix timeouts in coreroot_determinism test in GC stress mode (dotnet#56770)
  Use File.OpenHandle in Socket.SendFile directly (dotnet#56777)
  accept empty realm for digest auth (dotnet#56369) (dotnet#56455)
  [wasm][debugger] Create test Inherited Properties (dotnet#56754)
  Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
  Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
  [mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
  Update windows-requirements.md (dotnet#56476)
  Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
  [wasm][debugger] Inspect static class (dotnet#56740)
  Fix stack overflow handling issue in GC stress (dotnet#56733)
  Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
  Move Windows Compat pack to NuGet pack task (dotnet#56686)
  Fix build error when building some packages (dotnet#56767)
  Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
  Fix race in crossdac publishing with PGO (dotnet#56762)
  Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
  Use ComWrappers in some Marshal unit-tests and update platform metadata  (dotnet#56595)
  ...
@MihaZupan MihaZupan mentioned this pull request Aug 5, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@StephenMolloy StephenMolloy StephenMolloy left review comments

@stephentoub stephentoub stephentoub left review comments

@jkotas jkotas jkotas approved these changes

@steveisok steveisok steveisok approved these changes

@directhex directhex Awaiting requested review from directhex

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@mdh1418 mdh1418 Awaiting requested review from mdh1418

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@MaximLipnin @jkotas @steveisok @stephentoub @StephenMolloy