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

Switch to using the BCL HashCode type #16069

Merged
merged 1 commit into from Jun 13, 2019
Merged

Switch to using the BCL HashCode type #16069

merged 1 commit into from Jun 13, 2019

Conversation

roji
Copy link
Member

@roji roji commented Jun 13, 2019

Also remove allocations from GetHashCode() methods by refraining from using LINQ.

Notes:

  • I deliberately refrained from calling Aggregate() (or other LINQ) since we don't really want GetHashCode() to allocate.
  • In at least some cases we should consider caching the hash code to avoid recomputing, but am leaving that for some other time. This is precisely why
  • I've left out GetServiceProviderHashCode(), because it returns a long. Is there any specific reason we're return a long hash code rather than the standard int? Maybe we should change to int and use HashCode there as well?
  • In cases where a class calls base.GetHashCode(), I simply passed that value HashCode.Combine() in the subclass. That means double-hashing occurs, which I think should be OK (opinions/insights??). Otherwise we'd need to expose getting a HashCode instance from base classes (not even possible with base classes we don't own, e.g. Expression).

@ajcvickers
Copy link
Member

@roji Looks good to me. FYI @AndriySvyryd @smitpatel @bricelam @maumar Any concerns?

For the service provider hashing, see #13437. It's 64-bits to allow for more entropy, but it's unclear whether it really helps.

@roji
Copy link
Member Author

roji commented Jun 13, 2019

Ah, I think I understand now - the hash code is being used as the actual key, as opposed to the standard way of having the object as the key (IDbContextOptions in this case), in which case there's an Equal() check.

Was it done this way for perf reasons or something?

@ajcvickers
Copy link
Member

@roji It has a...history. ;-) It's certainly not something I am completely happy with, but it's also not clear how much value there is in re-working it.

@roji
Copy link
Member Author

roji commented Jun 13, 2019

😆

OK. If we did want to change this, now would be a good time as changing the return value from long to int would be a breaking change.

@ajcvickers
Copy link
Member

@roji That's true...but we could always postpone that change since we can still use a long type but with 32 unused bites. ;-)

@roji
Copy link
Member Author

roji commented Jun 13, 2019

Sure, but if we're OK with having only 32-bits, we might as well just change the type to int now and do the breaking change, no?

@ajcvickers
Copy link
Member

@roji I'm not okay with changing to 32-bits using the current design.

@roji
Copy link
Member Author

roji commented Jun 13, 2019

Yeah, very understandable if you guys have already had some collisions. I may think about this a bit, but nothing urgent obviously.

Copy link
Member

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Also remove allocations from GetHashCode() methods by
refraining from using LINQ.
@roji
Copy link
Member Author

roji commented Jun 13, 2019

Converted a few more foreach's to for's and squashed, will merge after build.

@roji roji merged commit 507ef21 into master Jun 13, 2019
@roji roji deleted the HashItUp branch June 13, 2019 19:17
roji added a commit to npgsql/efcore.pg that referenced this pull request Aug 7, 2019
roji added a commit to npgsql/efcore.pg that referenced this pull request Aug 7, 2019
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.

None yet

4 participants