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

100% readonly fields and ReadOnlySpan #47

Merged
merged 11 commits into from
Nov 28, 2021

Conversation

daiplusplus
Copy link
Contributor

@daiplusplus daiplusplus commented Nov 16, 2021

I'll admit this PR is a bit busy, I had a bad case of "one more thing...".


My main objective was to change class Hashids such that all of its fields were readonly, as currently the (somewhat twisted) initialization logic for _alphabet and _seps happens in a separate non-static method which mutates this fields, which isn't permitted by readonly.

The main commit that matters is c0564ad which moves that init logic into a new static void InitCharArrays.


Another significant change is in 99bd19f which replaces occurrences of char[] and long[] with ReadOnlySpan<> to signal intent that a parameter is not mutated, which is always a concern when passing around arrays as parameters (given C# lacks C++-style const-correctness or first-class support for immutability).

For compatibility with pre-.NET Standard 2.1 consumers (.NET Framework, etc) the library includes a thin internal struct ReadOnlySpan<T> reimplementation, which should be zero-cost (so long as it isn't moved to the GC heap or boxed), and the JIT should also inline all or most of its members.

I actually didn't put much (any?) effort into other optimizations, though I'm happy with the benchmark results below.

I'm tempted to add another commit with C# 8.0 nullability too...

UPDATE: I successfully implemented C# <Nullable>enabled</Nullable> as well as other performance improvements (such as eliminating array allocations in the various Encode/Decode overloads), but things snowballed and it became far too much to put into a PR, so I've moved those changes to a separate branch I'll submit a PR for only after this is approved (and hopefully merged).


Benchmark results:

  • For .NET Framework 4.8 there is an insignificant performance cost, I assume from the extra indirection from the ReadOnlySpan implementation. I don't believe that the .NET 4.8 runtime and JIT has any optimizations for readonly fields though.
  • For .NET 5.0 there's an insignificant performance boost, which I assume is from readonly fields and the fact that ReadOnlySpan is heavily optimized in .NET Core.

.NET Framework 4.8

Before

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
RoundtripInts 4.316 us 0.0326 us 0.0272 us 0.1297 - - 859 B
RoundtripLongs 5.077 us 0.0193 us 0.0161 us 0.1450 - - 971 B
RoundtripHex 5.418 us 0.0225 us 0.0210 us 0.3052 - - 1966 B

After

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
RoundtripInts 4.321 us 0.0360 us 0.0337 us 0.1297 - - 859 B
RoundtripLongs 5.129 us 0.0357 us 0.0316 us 0.1450 - - 971 B
RoundtripHex 5.350 us 0.0389 us 0.0364 us 0.3052 - - 1966 B

.NET 5.0

Before

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
RoundtripInts 3.949 us 0.0262 us 0.0245 us 0.0687 - - 576 B
RoundtripLongs 4.515 us 0.0198 us 0.0185 us 0.0687 - - 576 B
RoundtripHex 3.783 us 0.0118 us 0.0092 us 0.1793 - - 1512 B

After

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
RoundtripInts 3.910 us 0.0134 us 0.0125 us 0.0610 - - 512 B
RoundtripLongs 4.461 us 0.0200 us 0.0187 us 0.0610 - - 512 B
RoundtripHex 3.774 us 0.0153 us 0.0136 us 0.1602 - - 1344 B

@manigandham manigandham self-assigned this Nov 18, 2021
@manigandham
Copy link
Collaborator

manigandham commented Nov 28, 2021

Hey @Jehoel
Thanks for this, finally got a chance to review it all and this is some great work!

One small change noted there but I'll go ahead and merge to get this in. Looking forward to the other changes, but please keep the PRs as small as you can. Easier to review multiple PRs if needed.

@manigandham manigandham merged commit 8c398de into ullmark:master Nov 28, 2021
@daiplusplus
Copy link
Contributor Author

@manigandham Uhhh, I wasn't prepared for it to be merged as-is - there's a bunch of TODOs and Questions left in comments....

@manigandham
Copy link
Collaborator

@Jehoel

I only see 3 TODOs; - One about short variable names and the others about the seps/alphabet characters. We can take care of that after your other changes before doing a release.

This project was ported directly from the Javascript version originally so it followed the same layout and naming. Feel free to create a new branch or just push to this one and we can make another PR.

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