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.

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

Add more heuristics to match coreclr/corefx linker usage #626

Closed
sbomer opened this issue Jun 18, 2019 · 4 comments
Closed

Add more heuristics to match coreclr/corefx linker usage #626

sbomer opened this issue Jun 18, 2019 · 4 comments

Comments

@sbomer
Copy link
Member

sbomer commented Jun 18, 2019

The version of ILLink.Tasks currently used in coreclr and corefx uses a private build that contains a feature with conservative heuristics. The feature was not upstreamed (see #60), and since then mono/linker has been given some similar heuristics (#223). Source-build also maintains a patch that includes the non-upstreamed heuristics.

Our new infrastructure builds the package from an internal mirror of mono/linker, where we don't want to maintain such patches. To move forward, we need to unify the two features.

This will unblock issues like https://github.com/dotnet/corefx/issues/36861, dotnet/source-build#777, and https://github.com/dotnet/coreclr/issues/23694.

The current mono/linker behavior is to:

  • Keep the targets of reflection calls to typeof(T).GetConstructor, typeof(T).GetMethod, Typeof(T).GetProperty, typeof(T).GetField, typeof(T).GetEvent, and Type.GetType("T").

The heuristics used in coreclr and corefx with -h are:

  • When we see typeof(T), keep all of T's methods and/or fields.
  • When we see property accesses of a type, keep its constructors
    These would be enabled using -h LdtokenTypeMethods,LdtokenTypeFields,InstanceConstructors, for example.

The -h heuristics are more conservative for typeof(T), and less conservative for Type.GetType("T") (which they don't detect).

An alternative to unifying the reflection heuristics would be to investigate additional roots needed in coreclr and corefx with the new reflection heuristics. I've begun this process for System.Private.CoreLib at least.

@marek-safar
Copy link
Contributor

I think the correct approach going forward is to avoid any reflection like calls unless really necessary (e.g. the implementation details leaks to public API)

@sbomer
Copy link
Member Author

sbomer commented Jun 28, 2019

FWIW, here's the diff I saw last time for System.Private.CoreLib: https://gist.github.com/sbomer/ca2d858be5fcc9ceb5aeebaabf6bbec3

@sbomer
Copy link
Member Author

sbomer commented Jun 28, 2019

Once we take in jbevain/cecil#594, these heuristics will be the blocking issue for enabling the nullable context feature in coreclr (which is important as it significantly reduces the size of System.Private.CoreLib).

edit: The current understanding is that the linker removing the attribute isn't a customer-facing problem. So we may be able to postpone, but I believe it's generally important for us to be able to take linker updates (esp. if the assessment of the impact of dropping the attribute changes).

@marek-safar
Copy link
Contributor

This is kind of duplicate of #484

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants