Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix XmlSchemaTest on UWP#19912

Merged
shmao merged 1 commit intodotnet:masterfrom
shmao:19830-XmlSchemaTest
May 18, 2017
Merged

Fix XmlSchemaTest on UWP#19912
shmao merged 1 commit intodotnet:masterfrom
shmao:19830-XmlSchemaTest

Conversation

@shmao
Copy link
Contributor

@shmao shmao commented May 17, 2017

The PR is to enable using XmlSchema on UWP in ReflectionOnly mode. XmlSchema.Write.Write internally create the serializer for typeof(XmlSchema), see XmlSchema.cs. As XmlSchema has many known types (e.g. see XmlSchema.Items), we need to keep the metadata for all of those known types to make the serializer work in ReflectionOnly mode. The PR is to keep metadata for types under System.Xml.Schema. I tried to find a smaller closure for types to include, but I didn't find one.

Before the fix, the size of the output of XmlSerializer test was 18,553 KB; after the fix, the size became 18,579 KB.

Fix #19830

@shmao
Copy link
Contributor Author

shmao commented May 17, 2017

/cc: @morganbr @weshaggard

@shmao shmao requested review from huanwu, mconnew and zhenlan May 17, 2017 22:57
@shmao shmao self-assigned this May 17, 2017
@morganbr
Copy link

This change doesn't actually root anything, it just keeps metadata for types if they're being kept anyway. Is that your intention?

@shmao
Copy link
Contributor Author

shmao commented May 17, 2017

it just keeps metadata for types if they're being kept anyway

@morganbr can you please explain this a little bit? I can confirm that the fix did fix the test. Before the fix, the test failed due to errors like

there was an error reflecting type System.Xml.Schema.XmlSchema

What metadata would be kept by the entry I'm adding?

@morganbr
Copy link

Your directive (dynamic="Required Public") says to enable metadata for types/methods that get used, but not to keep any others. Let's say the namespace looks like this:

namespace System.Xml.Schema.XmlSchema
{
    public class A
    {
         public void F1() {}
         public void F2() {}
    }
    public class B {}
}

And the rest of the program looked like:

public static void Main()
{
    new A().F1();
}

Then A and F1 would be kept and have metadata. However, there would be no references to B or F2, so they would be removed. If that's acceptable, this is a good directive. If you intended to reflect over B or F2, you probably needed Dynamic="Required Public"

@shmao
Copy link
Contributor Author

shmao commented May 17, 2017

@morganbr Thanks for the explanation and the example. I think Public might be good enough. Here's part of the definition of XmlSchema,

public class XmlSchema
{
        [XmlElement("annotation", typeof(XmlSchemaAnnotation)),
         XmlElement("attribute", typeof(XmlSchemaAttribute)),
         XmlElement("attributeGroup", typeof(XmlSchemaAttributeGroup)),
         XmlElement("complexType", typeof(XmlSchemaComplexType)),
         XmlElement("simpleType", typeof(XmlSchemaSimpleType)),
         XmlElement("element", typeof(XmlSchemaElement)),
         XmlElement("group", typeof(XmlSchemaGroup)),
         XmlElement("notation", typeof(XmlSchemaNotation))]
        public XmlSchemaObjectCollection Items
        {
            get { return _items; }
        }
}

Type XmlSchema is rooted by new XmlSerializer(typeof(XmlSchema)). Would Public keep the metadata for other types shown above? For example, XmlSchemaObjectCollection, XmlSchemaNotation(not sure about this as it's used by the attribute), and etc. If yes, Public is good.

@morganbr
Copy link

It would keep metadata for the types, but it would only have it for their members if they were used somewhere else.

@shmao shmao merged commit fb3b056 into dotnet:master May 18, 2017
@karelz karelz modified the milestone: 2.1.0 May 19, 2017
MichalStrehovsky added a commit to MichalStrehovsky/corefx that referenced this pull request Jul 17, 2018
This corrects the size on disk disaster caused by dotnet#19912. After that pull request, all of the schema validation stuff would always be included for any app that depends on System.Private.Xml because during initial analysis we consider a lot of things in S.P.Xml necessary as a precaution for SG generating references to it. The RD.XML in S.P.Xml then roots things forever.

I spent some time getting a repro of the original failure and convinced myself this is not a scenario that would require RD.XML in the S.P.Xml assembly.

Here's the facts:
* This test uses a test hook to explicitly disable pregenerated serialization code for `XmlSchema`. (https://github.com/dotnet/corefx/blob/1afc5360013bedc4099875c836342f493b083e5f/src/System.Private.Xml/src/System/Xml/Schema/XmlSchema.cs#L175 that is on stack at the time of failure is an analysis slam dunk, so we do have pregenerated code, we just don't use it because of the hook)
* The test is then testing the reflection fallback path to serialize XmlSchema. This doesn't work. Reflection fallback path won't in general work for _any framework provided type_ because there's no RD.XML to root it. Reflection fallback really only works for user types because of our default RD.XML that roots all user types (we also use that for the CoreFX tests, which is why the other reflection fallback tests don't hit issues).
MichalStrehovsky added a commit that referenced this pull request Jul 18, 2018
This corrects the size on disk regression caused by #19912. After that pull request, all of the schema validation stuff would always be included for any app that depends on System.Private.Xml because during initial analysis we consider a lot of things in S.P.Xml necessary as a precaution for SG generating references to it. The RD.XML in S.P.Xml then roots things forever.

I spent some time getting a repro of the original failure and convinced myself this is not a scenario that would require RD.XML in the S.P.Xml assembly.

Here's the facts:
* This test uses a test hook to explicitly disable pregenerated serialization code for `XmlSchema`. (https://github.com/dotnet/corefx/blob/1afc5360013bedc4099875c836342f493b083e5f/src/System.Private.Xml/src/System/Xml/Schema/XmlSchema.cs#L175 that is on stack at the time of failure is an analysis slam dunk, so we do have pregenerated code, we just don't use it because of the hook)
* The test is then testing the reflection fallback path to serialize `XmlSchema`. This doesn't work. Reflection fallback path won't in general work for _any framework provided type_ because there's no RD.XML to root it. Reflection fallback really only works for user types because of our default RD.XML that roots all user types (we also use that for the CoreFX tests, which is why the other reflection fallback tests don't hit issues).
MichalStrehovsky added a commit to MichalStrehovsky/corefx that referenced this pull request Aug 10, 2018
This corrects the size on disk regression caused by dotnet#19912. After that pull request, all of the schema validation stuff would always be included for any app that depends on System.Private.Xml because during initial analysis we consider a lot of things in S.P.Xml necessary as a precaution for SG generating references to it. The RD.XML in S.P.Xml then roots things forever.

I spent some time getting a repro of the original failure and convinced myself this is not a scenario that would require RD.XML in the S.P.Xml assembly.

Here's the facts:
* This test uses a test hook to explicitly disable pregenerated serialization code for `XmlSchema`. (https://github.com/dotnet/corefx/blob/1afc5360013bedc4099875c836342f493b083e5f/src/System.Private.Xml/src/System/Xml/Schema/XmlSchema.cs#L175 that is on stack at the time of failure is an analysis slam dunk, so we do have pregenerated code, we just don't use it because of the hook)
* The test is then testing the reflection fallback path to serialize `XmlSchema`. This doesn't work. Reflection fallback path won't in general work for _any framework provided type_ because there's no RD.XML to root it. Reflection fallback really only works for user types because of our default RD.XML that roots all user types (we also use that for the CoreFX tests, which is why the other reflection fallback tests don't hit issues).
zacharycmontoya pushed a commit that referenced this pull request Aug 17, 2018
* Fix RD.XML for System.Private.Xml (#31125)

This corrects the size on disk regression caused by #19912. After that pull request, all of the schema validation stuff would always be included for any app that depends on System.Private.Xml because during initial analysis we consider a lot of things in S.P.Xml necessary as a precaution for SG generating references to it. The RD.XML in S.P.Xml then roots things forever.

I spent some time getting a repro of the original failure and convinced myself this is not a scenario that would require RD.XML in the S.P.Xml assembly.

Here's the facts:
* This test uses a test hook to explicitly disable pregenerated serialization code for `XmlSchema`. (https://github.com/dotnet/corefx/blob/1afc5360013bedc4099875c836342f493b083e5f/src/System.Private.Xml/src/System/Xml/Schema/XmlSchema.cs#L175 that is on stack at the time of failure is an analysis slam dunk, so we do have pregenerated code, we just don't use it because of the hook)
* The test is then testing the reflection fallback path to serialize `XmlSchema`. This doesn't work. Reflection fallback path won't in general work for _any framework provided type_ because there's no RD.XML to root it. Reflection fallback really only works for user types because of our default RD.XML that roots all user types (we also use that for the CoreFX tests, which is why the other reflection fallback tests don't hit issues).

* Add RemovableFeatureAttribute

* Make non-file URL support optional in XmlUrlResolver

This adds [removable feature annotation](dotnet/designs#42) to `XmlDownloadManager`.

If the user specifies that they would like to remove support for this at publish/native compilation time, the linker/compiler is going to replace the method body of `CreateWebRequestOrThrowIfRemoved` with a throwing method body. The exception message is going to inform the user that the feature has been removed because they opted into the removal.

Contributes to #30597. Saves 1.2 MB of the size of the Windows UWP People app. This is a size on disk regression that came with NetStandard 2.0.

* Annotate reflection fallback for S.P.DataContractSerialization (#31533)

This adds [removable feature annotation](dotnet/designs#42) to data
contract serialization.

If the user specifies that they would like to remove support for this
at publish/native compilation time, the linker/compiler is going to
replace the annotated method bodies with a throwing method body.
The exception message is going to inform the user that the feature
has been removed because they opted into the removal. The throwing
method body is significantly smaller than the transitive closure
of code reachable from the original method body.

Contributes to #30597. Turning this feature off in the UWP People app
saves 4% of size. This is a size on disk regression that came with
the new version of CoreFX and blocks the Windows team in picking up
the latest framework. There is a zero size growth goal.

* Sync SR class with the CoreRT copy

This ports over the change from
dotnet/corert@0ac83cb.

When UsingResourceKeys is true and we stripped the resources, the
existing code would have returned null.
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
This corrects the size on disk regression caused by dotnet/corefx#19912. After that pull request, all of the schema validation stuff would always be included for any app that depends on System.Private.Xml because during initial analysis we consider a lot of things in S.P.Xml necessary as a precaution for SG generating references to it. The RD.XML in S.P.Xml then roots things forever.

I spent some time getting a repro of the original failure and convinced myself this is not a scenario that would require RD.XML in the S.P.Xml assembly.

Here's the facts:
* This test uses a test hook to explicitly disable pregenerated serialization code for `XmlSchema`. (https://github.com/dotnet/corefx/blob/dotnet/corefx@1afc5360013bedc4099875c836342f493b083e5f/src/System.Private.Xml/src/System/Xml/Schema/XmlSchema.cs#L175 that is on stack at the time of failure is an analysis slam dunk, so we do have pregenerated code, we just don't use it because of the hook)
* The test is then testing the reflection fallback path to serialize `XmlSchema`. This doesn't work. Reflection fallback path won't in general work for _any framework provided type_ because there's no RD.XML to root it. Reflection fallback really only works for user types because of our default RD.XML that roots all user types (we also use that for the CoreFX tests, which is why the other reflection fallback tests don't hit issues).

Commit migrated from dotnet/corefx@6867b6f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XmlSerializerTests.XmlSchemaTest failed on UWP

5 participants