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

Optimize PersistentGenericSet snapshot #2394

Merged
merged 7 commits into from
May 31, 2020

Conversation

maca88
Copy link
Contributor

@maca88 maca88 commented May 21, 2020

An alternative approach of #2393, which uses HashSet<T>.TryGetValue for .Net Core and a dictionary for .NET Framework

}
if (_syncRoot == null)
{
Interlocked.CompareExchange<object>(ref _syncRoot, new object(), null);
Copy link
Contributor Author

@maca88 maca88 May 21, 2020

Choose a reason for hiding this comment

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

Used the same logic as List<> has.

@hazzik
Copy link
Member

hazzik commented May 25, 2020

Maybe we stick to the dictionary to avoid using conditional compilation?

@hazzik
Copy link
Member

hazzik commented May 25, 2020

Also, hibernate just uses Map without any additional wrappers.

maca88 and others added 2 commits May 25, 2020 14:26
Co-authored-by: Frédéric Delaporte <12201973+fredericDelaporte@users.noreply.github.com>
Co-authored-by: Frédéric Delaporte <12201973+fredericDelaporte@users.noreply.github.com>
@maca88
Copy link
Contributor Author

maca88 commented May 25, 2020

Hibernate is using HashMap, which supports null keys, so I think we should too.

Maybe we stick to the dictionary to avoid using conditional compilation?

To me using a dictionary is only a temporary solution until we upgrade to .NET Standard 2.1 or higher as HashSet is more memory efficent and a simpler solution for the problem (less code).

@hazzik
Copy link
Member

hazzik commented May 26, 2020

Is it possible to add unit tests for SetSnapshot (but no reflection please)?

@maca88
Copy link
Contributor Author

maca88 commented May 26, 2020

Tests added.

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

Successfully merging this pull request may close these issues.

4 participants