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

Changes for speed increase. #1

Closed
wants to merge 3 commits into from
Closed

Changes for speed increase. #1

wants to merge 3 commits into from

Conversation

deipax
Copy link

@deipax deipax commented Aug 4, 2016

Also, added a rework of FastReflection that is faster to access getter/setters. Supports Fields on POCO and uses any backing fields found automatic properties.

Added a rework of FastReflection that is faster to access and supports Fields and backing fields for automatic properties.
@jhgbrt
Copy link
Owner

jhgbrt commented Aug 22, 2016

I opened Issue #2 for this.

@jhgbrt
Copy link
Owner

jhgbrt commented Aug 22, 2016

Thank you for your contribution.

It seems this PR also changes the behavior of ConvertRefType (and maybe other things as well?) In the released version, this method throws NullReferenceException if the input is DbNull. I'm open to debate this choice, but changing it is a breaking change that I can't introduce just like that. So it would be good if you could rework the PR with just the performance-related stuff?

@deipax
Copy link
Author

deipax commented Aug 22, 2016

The vast majority of speed up comes from sending in maps of the field/property names. If M is the number of columns and N the number of rows,: The previous implementation called GetFields M*N times where the current implementation calls it just M times. The previous implementation also called GetterSettersForType() N times where the current implementation calls it just 1 time.

As to ConvertTo, yes, I did change it in a couple different ways for speed, but it was a change independent of the one I listed above and can be skipped or considered at a different point in time:

The first thing to note is that the ConvertValueType is the place in which the exception was being thrown. I was aware that I was changing what could be an interface breaking change, but only if someone was attempting to convert a null/dbnull in a try/catch and then branching logic on that .. which I, personally, try to avoid. Since the other functions were returning default(T), it seemed like a safe bet .. especially when you consider that its primary purpose is to build from data. If the reader returned dbnull, then let the property remain default. That was my preference, but it is your call.

Another way I changed the ConvertTo was to test the type of the object being sent in for conversion against ... if they were the same type, why not just unbox it and return it? Oddly enough Convert.ChangeType will spend cycles attempting to change an int to an int or a long to a long.

A note on FastReflection, I pretty much left it alone except for returning the IReadonlyDictionary interface (which I suppose is also a breaking change) ... I noticed the getter was calling a ConvertTo after retrieving the value of the property. This convert would only ever serve to change a null/dbnull to a default(T) or throw the exception if it was a value type because if the property was set, that implies that the value returned was already the correct type. Something, again, I thought as odd considering my take on try/catch logic branching.

What would you prefer I do?

…that is faster to access and supports Fields and backing fields for automatic properties."

This reverts commit 531dee2.
Also added an experimental version of fast reflection which deviates from the original in several ways.  It supports getting/setting fields in addition to properties.  For properties that have backing fields (automatic properties) those are leveraged to avoid the overhead that comes with C# properties.  Added sample unit tests  for display purposes.
@deipax
Copy link
Author

deipax commented Aug 29, 2016

Have you had a chance to look at the changes?

@jhgbrt
Copy link
Owner

jhgbrt commented Sep 7, 2016

I'm sorry, been extremely busy lately with other things. But I will get to
it, I promise ;-)

Jeroen Haegebaert
jeroen@haegebaert.com - Mobile: +32 (0)486 284272

http://www.dertiendester.be - helpt kinderen in Ethiopië

On 29 August 2016 at 18:09, deipax notifications@github.com wrote:

Have you had a chance to look at the changes?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHsUv37FAgOdiuQYwG4xDapvbjIApUtks5qkwQegaJpZM4Jc61r
.

@jhgbrt
Copy link
Owner

jhgbrt commented Nov 1, 2020

This was merged outside of the PR

@jhgbrt jhgbrt closed this Nov 1, 2020
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