Skip to content

Override removal#456

Merged
marek-safar merged 1 commit into
dotnet:masterfrom
Unity-Technologies:master-override-removal
Feb 8, 2019
Merged

Override removal#456
marek-safar merged 1 commit into
dotnet:masterfrom
Unity-Technologies:master-override-removal

Conversation

@mrvoorhe
Copy link
Copy Markdown
Contributor

@mrvoorhe mrvoorhe commented Feb 1, 2019

If no instance of a type can exist, then it's safe to not mark overrides as long as the method being overridden is not abstract.

This change should be good to go. I've added WIP as I'm still running these changes through our il2cpp test suite as that is a valuable way to get some extra coverage on linker changes.

Do not land this branch before #454 otherwise there will be problems.

@mrvoorhe mrvoorhe requested a review from marek-safar as a code owner February 1, 2019 19:43
@mrvoorhe
Copy link
Copy Markdown
Contributor Author

mrvoorhe commented Feb 4, 2019

@marek-safar FYI for perspective this PR is not a particularly big size for a Console.WriteLine("Hello world") program. IT only saves 0.06%. It does better in other scenarios, for example, in the artificial typeof(System.Xml.XmlDocument) scenario in #455 this PR is able to get the same size savings.

@mrvoorhe mrvoorhe force-pushed the master-override-removal branch 2 times, most recently from b4c1907 to d7e3ef9 Compare February 4, 2019 15:00
@mrvoorhe
Copy link
Copy Markdown
Contributor Author

mrvoorhe commented Feb 4, 2019

Fixed the test failure. EmbeddedLinkXmlPreservesAdditionalAssemblyWithOverriddenMethod needed to be updated to mark a .ctor on some types.

I still need to check on the il2cpp test results w/ this change before I drop the WIP

@mrvoorhe mrvoorhe changed the title WIP : Master override removal Override removal Feb 8, 2019
@mrvoorhe mrvoorhe force-pushed the master-override-removal branch from d7e3ef9 to 0da49a6 Compare February 8, 2019 12:56
@mrvoorhe
Copy link
Copy Markdown
Contributor Author

mrvoorhe commented Feb 8, 2019

@marek-safar This PR is ready for final review. After preserving the .ctor() of InternalThread our il2cpp test suites were green. I also preserved the .ctor() of MonoListItem because looking at our mono runtime code I found that it should be, but that one doesn't seem to have been as critical as the InternalThread.

Do you want me to open a PR to mono master adding either of these .ctor() preserves to mscorlib.xml? You had seemed to imply that InternalThread has been renamed?

@mrvoorhe
Copy link
Copy Markdown
Contributor Author

mrvoorhe commented Feb 8, 2019

Looks like after rebasing and picking up the Finalize() fix I have a couple test failures. I will take a look.

@mrvoorhe mrvoorhe force-pushed the master-override-removal branch from 0da49a6 to 655c2c3 Compare February 8, 2019 13:08
@mrvoorhe
Copy link
Copy Markdown
Contributor Author

mrvoorhe commented Feb 8, 2019

Easy fix. Needed to update the NeverInstantiatedTypeWithOverridesFromObject tests to no longer assert that the overrides from System.Object are kept 😄

@marek-safar marek-safar merged commit 6b87e44 into dotnet:master Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants