Skip to content

Convert EntityKey to struct to reduce GC #1975

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

Open
hazzik opened this issue Jan 14, 2019 · 1 comment · May be fixed by #1976
Open

Convert EntityKey to struct to reduce GC #1975

hazzik opened this issue Jan 14, 2019 · 1 comment · May be fixed by #1976

Comments

@hazzik
Copy link
Member

hazzik commented Jan 14, 2019

As shown in #1972 (comment) it gives about 6 megabytes improvement (down to 167Mb overall).

@bill-poole
Copy link

I’m interested in understanding the performance gained by this. I imagine it would be substantial. That is, I suspect the primary benefit to be had from this improvement is the performance gain more than the memory saving.

Using structs also improves memory locality, which increases the likelihood of CPU cache hits. Although, I suspect the standard .NET dictionary class is pretty heap allocation heavy anyway. But still, one level of indirection is better than two.

A .NET Swisstable implementation would be much better, if it existed.

I assume the entity key is used as the key in a dictionary, which is passed by value, meaning there could be a overhead in passing the struct parameter.

That being said, a lot of work has gone into RyuJIT lately to “keep structs in registers” (there is a user story by that name in the .NET runtime repo), so it’s possible that will mitigate the overhead.

If EntityKey.Equals is inlined and EntityKey.GetHashCode is devirtualised and inlined, then no problem.

In the end, it comes down to benchmarking the different design options - e.g. using BenchmarkDotNet.

Has any performance benchmarking been done?

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

Successfully merging a pull request may close this issue.

2 participants