Make the CastCache table rooted by a managed static field. #676
Conversation
I agree that this is prep work for fully managed casting cache impl, but it is not simpler than the implementation with handles. It is more lines of code and it has several hundred bytes larger footprint due to an extra type loaded. |
|
Conceptually working with a pointer is simpler than working with a GC handle. But having to load an extra managed class probably balances that out. I guess opening this to managed code is more important goal here. For that sharing a static field is more convenient than sharing a handle. |
|
It may be best to fold this into a change that actually moves the casting logic to managed code so that it is an net improvement. It is a tiny change, so there is no significant upside in factoring it into a separate PR. |
src/coreclr/src/vm/castcache.cpp
Outdated
| @@ -136,7 +139,7 @@ TypeHandle::CastResult CastCache::TryGet(TADDR source, TADDR target) | |||
| } | |||
| CONTRACTL_END; | |||
|
|
|||
| BASEARRAYREF table = (BASEARRAYREF)ObjectFromHandle(s_cache); | |||
| BASEARRAYREF table = *s_pTableRef; | |||
|
|
|||
| // we use NULL as a sentinel for a rare case when a table could not be allocated | |||
| // because we avoid OOMs in conversions | |||
There was a problem hiding this comment.
Nit:
| // because we avoid OOMs in conversions | |
| // because we avoid OOMs |
Casting is not conversion. IIRC, we had discussion about this on the original PR and most places were fixed, but it looks like this one was missed.
|
I needed feedback on whether this is something we would be ok with doing. The change is self-contained, so it could be an independent step from the rest, but it is indeed a fairly small change. |
This makes managing the cast table a bit simpler (no handles).
Also a prerequisite to accessing the cast cache directly from managed code (re: https://github.com/dotnet/coreclr/issues/27931)