types,consensus: Add hashAll helper #129
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a slightly bigger change, so I figured I'd PR it to allow for some discussion instead of just pushing it.
Sia computes a lot of hashes. When I started implementing v2, I took care to make hashing as fast as possible. In particular, I wanted hashing operations to be zero-alloc. That meant accepting some ugliness like
hasherPool
and eschewing helper functions that might not be inlined.This PR essentially reverses that decision. Instead of every
ID
function grabbing a hasher and directly callingEncodeTo
on all the stuff we want to encode, we now pass those things to a magichashAll
helper function. The resulting code is definitely more readable; you can see at a glance exactly what's being hashed. The downside is that all of our hashes now allocate, making them a bit slower. How much slower? Anywhere from 1% to 15%, depending on how little data is being hashed. Is that acceptable? Well, when I run our test suite 100x, it takes basically the same amount of time -- definitely nowhere near 15% slower. Regardless -- preventable allocations make me sad. At least ~all of our hashing will be performed insidehashAll
now, which means CPU profiles will show us how much time we spend hashing in general...?Another consequence is that the exact data we're hashing has changed slightly. Basically, instead of omitting signatures, we now hash empty signatures. Why? Because it means instead of writing this:
we can write this:
Having a helper like
hashAll
doesn't force us to make this change, but it sure makes it a lot more compelling, doesn't it?Lemme know if you have strong feelings about this either way. Frankly, I'm pretty confident that this is the right choice; writing a big post about it is probably just my way of coping with the loss of my precious zero-alloc code 😭