Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Remove null checks before GetValueOrDefault - #2663

Closed
jamesqo wants to merge 1 commit into
dotnet:masterfrom
jamesqo:sum
Closed

Remove null checks before GetValueOrDefault#2663
jamesqo wants to merge 1 commit into
dotnet:masterfrom
jamesqo:sum

Conversation

@jamesqo

@jamesqo jamesqo commented Aug 6, 2015

Copy link
Copy Markdown
Contributor

Separated from PR #2318. Free perf.

@stephentoub

Copy link
Copy Markdown
Member

Free perf

Can you please share your perf tests and results? What's the impact when all values are null? When some are null? When none are null? Etc.

@stephentoub

Copy link
Copy Markdown
Member

cc: @VSadov

@mikedn

mikedn commented Aug 7, 2015

Copy link
Copy Markdown

Can you please share your perf tests and results? What's the impact when all values are null? When some are null? When none are null? Etc.

This approach should be faster if the number of nulls in the source is low. GetValueOrDefault "cheats" and returns the value field without actually checking hasValue, that avoids a bunch of things: a memory load, a 0 test and a branch.

If the number of nulls is high then this will likely be slower, especially in the decimal case. add operations that previously were simply skipped now how to be done.

Let's see what benchmarks will say :)

@jamesqo

jamesqo commented Aug 7, 2015

Copy link
Copy Markdown
Contributor Author

Just finished writing the test. Here is the test (ideone), here are the old results, here are the new ones. What are your opinions?

@stephentoub

Copy link
Copy Markdown
Member

Thanks for sharing. Based purely on the data you shared, I don't think this is a change we should take. Where there was an improvement, it was typically significantly less than 1%, and there were plenty of regressions, including several greater than 1%, and one almost 5%.

@jamesqo

jamesqo commented Aug 7, 2015

Copy link
Copy Markdown
Contributor Author

Alright, I understand. Thanks.

@jamesqo jamesqo closed this Aug 7, 2015
@jamesqo
jamesqo deleted the sum branch August 7, 2015 20:14
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants