Fix closure #693
Fix closure #693
Conversation
|
There are actually more places with closure, but I think all of them are executed during proxy generation and don't affect the proxy usage later on. |
| var cacheKey = new CacheKey(proxiedMethod, type); | ||
|
|
||
| return cache.GetOrAdd(cacheKey, ck => ObtainMethod(proxiedMethod, type)); | ||
| return cache.GetOrAdd(cacheKey, static ck => ObtainMethod(ck.Method, ck.Type)); |
There was a problem hiding this comment.
(Theoretically generated code could be reduced by changing ObtainMethod such that it accepts a single CacheKey argument, then it could be used directly here in place of the lambda function wrapping it. But that's a different refactoring perhaps for another time.)
There was a problem hiding this comment.
Do you mean method group? I don't mind doing that if you like.
There was a problem hiding this comment.
I guess so, yes. I meant replacing the whole ck => ObtainMethod(ck.Method, ck.Type) with just ObtainMethod, which might be possible if that method were made to accept a singleCacheKey argument instead of two. But I just mentioned in it passing, no need to change that.
|
If there are more such cases that are equally trivial to fix, then by all means, please go ahead. However when the refactorings get more complicated / time intensive, or when they negatively affect code legibility / make the code harder to understand, those are probably cases we should leave unmodified as the change wouldn't seem worth it. I'm happy to merge this but if you want to add more, I can wait a little. Just let me know how you'd like to proceed. |
|
Fixing other places will require adding a third argument to |
|
Looks like .NET broke our test suite once again. Will need to fix that in a separate PR before we can merge this one here. |
|
@jonorossi, true. Thanks for the friendly reminder. I have been busy elsewhere lately and couldn't remember right away where exactly we left off. Will try to clean up our build accordingly. |
|
Guess I can made a dedicated pull-request using my last commit from #694 (That fixes the build-pipeline by removing obsolete netcoreapp2.1 + netcoreapp3.1 from unit-testing) |
|
@snakefoot, thanks for the offer, that shouldn't be necessary. The TFMs are now updated and builds are passing again. @gao-artur, sorry for the long wait. Merging now. Thanks for the optimization. |
4 participants
InvocationHelper.GetMethodOnTypeis in on a hot path and creates relatively much garbage.ProxyUtil.AreInternalsVisibleToDynamicProxyis not on a hot path but was easy to fix.There is another closure in
BaseProxyGenerator.GetProxyType, which is a bit harder to fix, but since it's not a hot path I left it as is. Let me know if you want to fix it too.