ATS: performance updates - #322
Conversation
There was a problem hiding this comment.
This good because it creates a compiled delegate to create instances. I'm fine with it staying like this for now, and possibly for always. However, it is possible that for project-n the delegate created will be interpreted rather than truly compiled. This is one of the reasons we are avoiding creating "compiled" delegates where possible. Take a look at SimpleEntityKeyFactory and related classes. In these classes we avoid having to compile any dynamic code by creating a generic type for the factory using Reflection, creating one instance of this factory, then caching the factory instance such that it can be used over and over to create SimpleEntityKey instances without Reflection and without using any "compiled" delegates. All that being said, I am fine with leaving this code as is for now, especially since it is for ATS which does not currently intersect with project-n.
There was a problem hiding this comment.
I talked to Arthur about this. I think it is worth having a TODO to consider doing what he described later.
There was a problem hiding this comment.
Agreed. For now, I'm intentionally not caching instances to avoid storing an adapter for every entity in the table. This would be faster on the CPU in some cases, but worse for memory in others. These issues can be mitigated by limiting the cache size or using a different method of generic type construction. I'll add a TODO to investigate these options and their performance gains.
|
|
…iting emulator issues #322 and #319; remove diagnostic md files Agent-Logs-Url: https://github.com/dotnet/efcore/sessions/773a262b-df07-464f-9364-92c079d190c7 Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Includes some renaming and whitespace changes.
Notable changes:
TableEntityFactory - uses thread-safe caching and no longer uses the slow .Invoke() method for instance construction.
TableFilter - refactored so it no longer uses reflection
Namespace changes - moved extensions to the namespace they are extending-->Should light up in Intellisense
Also, adds more detailed tests