-
Notifications
You must be signed in to change notification settings - Fork 59
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
Improve float serialization, add .NET 5 Half support. #77
base: master
Are you sure you want to change the base?
Conversation
.NET Core 3.0 is unsupported now and targeting it actively throws a warning on the .NET 5 SDK.
For future commit to add Half support.
Float serialization used the same serialization code as normal integers, which writes less bytes if the integer is of sufficiently small size. The problem however is that a float is basically *never* so small due to how the bits are laid out and this almost always resulted in a wasted extra byte vs just writing the 4/8 bytes of the float directly. ("basically never" is everything except 0/subnormals, which I'd say is not worth adding an extra byte to almost all other floats over. I even tested all possible floats just to make sure.) The new code is also faster on Core because it writes the 4/8 bytes at once instead of individual WriteByte/ReadByte() calls (see benchmarks in PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single PR is fine. It's the commits themselves which are important (doing just a single thing in a single commit).
I had some minor comments, but otherwise this looks good.
} | ||
|
||
return a; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these two methods be part of the next commit ("Add support for .NET 5 Half type.")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was kind of debating that as well but figured this would make the diffs cleaner since now all these uint read/write methods are in the same commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasons why I think it should not be part of the commit:
- This commit is "improve float serialization", i.e. improve what we already have
- The new methods are not used (so they cannot even improve the float serialization)
- The commit message doesn't mention adding new feature
- If you do add a note to the commit message, it would probably be something like this at the end of the message "Also, add xyz", which is a clear hint that the code is probably really not part of the commit.
The "Fix indentation" looks fine, but please don't add "fix an earlier commit in this pull request" commits to a pull request. Just fix the earlier commits, and push the new branch over the old pull request. Or if there are a lot of changes, perhaps create a new pull request. Github is broken and doesn't support the above model properly, but I don't want such fixes merged. They just make the history more confusing, and in some cases (not here, though) break bisect. |
Any plan to release new version of NetSerializer with .NET 5.0 and higher? Do you have any timeline? |
Apologies for doing this all in one PR. Doing it separately would've produced three conflicting PRs which is even less ideal.
Optimized float serialization to be faster and write less bytes in almost all cases. Add .NET 5
Half
support. See commits for details.Benchmarks for writing/reading code mentioned
Span code is used on .NET Core/.NET 5 for 32/64 bit since it's faster.
Benchmark code:
Proof that compressed int writing for floats does not help