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

Dependably determine collectibility in S.L.Expression's CanCache #25670

Closed
wants to merge 2 commits into from

Conversation

JonHanna
Copy link
Contributor

@JonHanna JonHanna commented Dec 4, 2017

Use RuntimeType.IsCollectible() to determine collectibility of types rather than only assuming those in mscorlib are not, which has many false positives (over 99% false positives in the uses Microsoft.CSharp and System.Dynamic.Runtime make of expressions).

Build a delegate to make the call when IL generation is available, or use reflection directly when it is not.

  • Use collectible dynamic assemblies within System.Linq.Expressions.Tests

Allows for the paths where CanCache returns false to be exercised, along letting the dynamic assemblies be collected, anyway.

Use RuntimeType.IsCollectible() to determine collectibility of types
rather than only assuming those in mscorlib are not, which has many
false positives (over 99% false positives in the uses Microsoft.CSharp
makes of expressions).

Build a delegate to make the call when IL generation is available, or
use reflection directly when it is not.
Allows for the paths where CanCache returns false to be exercised, along
letting the dynamic assemblies be collected, anyway.
@JonHanna
Copy link
Contributor Author

JonHanna commented Dec 4, 2017

If #25671 is accepted and implemented then it can replace the reflection used here.

@jkotas
Copy link
Member

jkotas commented Dec 4, 2017

If #25671 is accepted and implemented then it can replace the reflection used here.

I agree that API like this would be a good idea.

The format API review is required to expose API in the public contracts only. It is not needed to expose APIs from CoreLib that are used internally within other .NET Core assemblies that depend on CoreLib directly. I think you can go ahead and this API. It would be preferable to using private reflection. The private reflection does not work well for CoreRT/.NET Native UWP - I suspect that your current change is going to cause break there.

@JonHanna
Copy link
Contributor Author

JonHanna commented Dec 4, 2017

Oh, I get you, I make it public in coreclr and corert but not in the refs. Thanks, @jkotas

@JonHanna JonHanna closed this Dec 4, 2017
@JonHanna
Copy link
Contributor Author

JonHanna commented Dec 4, 2017

@jkotas A question on that in CoreRT, which I'm not familiar with at all (the only little bit of work I've done there was some very straight-forward copy-paste of some changes in CoreCLR). How does RuntimeType work there? I can't see anything equivalent, or equivalent to its IsCollectible().

@jkotas
Copy link
Member

jkotas commented Dec 4, 2017

RuntimeType is always non-collectible in CoreRT currently.

@jkotas
Copy link
Member

jkotas commented Dec 4, 2017

@JonHanna
Copy link
Contributor Author

JonHanna commented Dec 4, 2017

Ah. I looked there but the lack of a comparable method confused me, when I shouldn't have been surprised it didn't need one. Thanks again, I'll hopefully get to this, this evening.

@karelz karelz added this to the 2.1.0 milestone Dec 4, 2017
JonHanna added a commit to JonHanna/coreclr that referenced this pull request Dec 4, 2017
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and dotnet/corefx#25670

Mirror PR to corert will need and overload making runtime types return
false there.
jkotas pushed a commit to dotnet/coreclr that referenced this pull request Dec 5, 2017
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and dotnet/corefx#25670

Mirror PR to corert will need and overload making runtime types return
false there.
dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Dec 5, 2017
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and dotnet/corefx#25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/corert that referenced this pull request Dec 5, 2017
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and dotnet/corefx#25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jashook pushed a commit to jashook/coreclr that referenced this pull request Dec 12, 2017
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and dotnet/corefx#25670

Mirror PR to corert will need and overload making runtime types return
false there.
dotnet-bot pushed a commit that referenced this pull request Jan 13, 2018
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and #25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
dotnet-bot pushed a commit that referenced this pull request Jan 13, 2018
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and #25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
safern pushed a commit that referenced this pull request Jan 16, 2018
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and #25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
safern pushed a commit that referenced this pull request Jan 16, 2018
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and #25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
@JonHanna JonHanna deleted the sle_more_cacheability branch March 12, 2018 12:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants