Cache type refs during import #224
Conversation
- importing many types/methods would result in millions of duplicate TypeRef objects, this basic caching saves lots of memory in that use case. - one issue with it has to do with Cecil not filling up GenericParameters for all types (hence the workaround). Was seen during Dapper or Bcl UTs.
| var key = TypeRefKey.From (type); | ||
| if (cache.TryGetValue (key, out reference)) | ||
| { | ||
| // Cecil only fills TypeRef GenericParameters if used ( bug ?) |
There was a problem hiding this comment.
btw this should be properly fixed IMO, so I don't expect this to be merged as-is 馃槃
I didn't quite find where to make the proper fix though.
AFAICT Cecil creates trivial GenericParameter's only upon use during assembly read.
|
It's now easy for you to override the MetadataImporter with your own. See ReaderParameters.MetadataImporterProvider |
|
Oh missed that, thanks. |
|
I'm pretty sure you can extend MetadataImporter and override ImportReference to call base.ImportReference only if not cached. |
|
That would work if the MetadataImporter called ImportReferences in its various Import methods, but it doesn't, it calls ImportType instead (private). |
|
(I can make a PR to change only that, and will be able to manage the caching on my side then) |
|
Right. Let me think about it some more and I'll get back to you. |
|
@jbevain is this still a viable change? |
TypeRef objects, this basic caching saves lots of memory in that use case.
for all types (hence the workaround). Was seen during Dapper or Bcl UTs.
I guess this change can be debated, but as it bring a large perf impact for ILRepack (~20%), I kind'o need it 馃槃
Other ways to tackle it would be:
If those sound more appropriate for Cecil, I can modify this PR accordingly (though IMO, all import use cases would benefit from this cache).
MethodRef could/should be cached similarly, but the impact is lesser than TypeRefs.