Soundness feedback requested: Increasing cache success in S.L.Expressions

[JonHanna]

Expressions contains a CanCache(Type) method that reports on the safety of caching a delegate or information about a method based on whether the type is a mscrolib type that (if generic) has only mscorlib type parameters.

The netfx version allows not just mscorlib types to be considered safe, but also those in System.Core where the cache lives. While there aren't as many types in corefx's S.L.Expressions as there are in netfx's S.Core, this basic principle seems sound to me.

It also seems worth doing, tracking hits and misses (of the outer calls only, ignoring its recursive calls on generic parameters except as they affect the final result) has the tests for Microsoft.CSharp go from 0.05% hits to 54.40% and the tests for System.Dynamic.Runtime go from 0.27% to 41.87%. (S.L.Expressions itself goes from 49.59% to 54.34%, and if anything I was expecting it to be an unfair comparison in explicitly using Expressions types more than other tests would, but it it also does a lot of permutations on the primitive types that would have a high hit rate). That would seem to be enough to suggest that dynamic is attempting to cache with so low a success rate as to maybe not be worth trying, but with the change to include Expressions' types it becomes much more useful.

And it seems safe to me, as surely if the types in Expressions were collectible then collecting would also make the static cache collected.

Question 1: Am I wrong in this? Could a type in Expressions be collectible without the TypeUtils type becoming collectible (unreachable at least, even if not yet collected) and making the static cache collectible? Because if I am wrong, then this idea isn't safe.

Question 2: If I'm correct in this, and caching uses of these types would be safe, would it also be safe to consider types in assemblies referenced by Expressions (transitively) safe to cache?

Question 3: Iff the answer to the first question is "safe" and to the second is "not safe", would the assemblies of types that are base types or implemented interfaces of safe types, also be safe (probably too much of a burden to check that to pay off anyway, but figuring out an efficient way to check on this is a separate investigation).

[jkotas]

Would it be better to introduce a IsCollectible property on a Type instead, and use it here?

[JonHanna]

@jkotas I don't know how feasible the implementation of that would be, though it could likely bring many real-world uses of dynamic from near-zero up to 100%, and be useful in many other places, +1.

I'd still be interested in attempting the above in the meantime, since if my reasoning is correct then it would still be safe for S.L.Expressions to cache types from itself even if it had been loaded in a way that made its types collectible (can this even happen right now without dotnet/coreclr#8677? anyway caution suggests assuming that makes it possible) because if the assembly was unloaded again then the TypeUtils type and its cache would itself become collectible.

[JonHanna]

(Also, if we could have IsCollectible and if typeof(NonCollectibleGeneric<CollectibleTypeArgument).IsCollectible was false, it would make the code easier. It's existence might also lead to it being discovered by people who would realise they should have used it to remove a memory leak they aren't aware they have. (Related: I'm not entirely easy with the way Microsoft.CSharp caches types it sees, though I can't think of a good way around it that wouldn't slow a lot of things up).

[JonHanna]

Looking at the history of the implementation, it had been in src/Common which would make the way netfx identified the assembly by looking for that of typeof(Expression) no longer valid, and I think that may be why it was dropped. I'm going to go ahead and make a PR, though I'll keep this open for a while in case anyone does see a danger.

[JonHanna]

Scratch that. I looked into how one would implement such an IsCollectible and I can see how to get to it through reflection today. Thanks @jkotas

[jkotas]

Note that System.Linq.Expressions has direct access to CoreLib. It means that you can use CoreLib APIs that are not exposed in public contracts (yet). It is preferable to do it that way instead of private reflection.

[JonHanna]

@jkotas but RuntimeType is still internal, so

public static bool CanCache(this Type t) => t is RuntimeType rt && rt.IsCollectible();

Gives the error I'd expect. Are you saying there's a way around that?