Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance gain from caching of GetProperties #92

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

khalidsalomao
Copy link
Contributor

There is a performance gain from caching GetProperties result.

Since the type properties are immutable we can safely save it in a static property for ReflectionDocumentMapper<T>.

A simple benchmark indicates a performance gain of 5-10x (depends on the number of properties a class have).

Here is the code that you can open in LinqPad and test it!
https://gist.github.com/khalidsalomao/7f3a6c3f6c514f584d6a

@chriseldredge, when I was review ReflectionDocumentMapper<T> I noticed that the GetProperties only return instance properties. What do you think of also supporting its parent public properties by passing the flag BindingFlags.FlattenHierarchy?

There is a performance gain from caching `GetProperties` result. 

Since the type properties are immutable we can safely save it in a static property for `ReflectionDocumentMapper<T>`.

A simple benchmark indicates a  performance gain of 5-10x (depends on the number of properties a class have).

Here is the code that you can open in LinqPad and test it!
https://gist.github.com/khalidsalomao/7f3a6c3f6c514f584d6a
@chriseldredge
Copy link
Owner

Since there are additional reflection calls in BuildFieldMap and BuildFieldKeyMap, does it make sense to cache the entire instance of ReflectionDocumentMapper<T> for reuse? Perhaps LuceneDataProvider should keep an instance cache of T to ReflectionDocumentMapper<T>?

Regarding BindingFlags.FlattenHierarchy, it probably makes sense to do that. I suppose it could break existing projects if they're not expecting it, but it seems unlikely.

@khalidsalomao
Copy link
Contributor Author

Sure, caching ReflectionDocumentMapper<T> in LuceneDataProvider will be better. I will take a look into that!

Regarding BindingFlags.FlattenHierarchy, it was my mistake. Best keep it as it is! After a while, I remembered that it affects only static members... Sorry about that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants