This repository has been archived by the owner on Feb 9, 2021. It is now read-only.
Improve performance of compact utxo conversions #761
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We're going to make AbstractHash a proper ADT and stop exporting the constructor, so we will need to add a few more converison functions. We start simply by rearranging and clarifying some things.
To/from bytes, and from Digest. These will be needed when we make AbstractHash actually abstract (in the ADT sense).
Internally within the Hashing module. It's neater and will mean fewer things to change when we change the representation.
Use the conversion functions elsewhere.
Either unused, or unnecessary and should not be used.
It exposes too much and constraints the choice of representation. It was only used by the ByteArrayAccess instance for MerkleRoot which itself was unused after the recent changes to use hash conversion utility functions.
The instance To/FromCBOR ShortByteString.
Change AbstractHash from being a newtype over Digest to a newtype over ShortByteString and adjust the conversion functions. The ShortByteString type has efficient Eq & Ord instances whereas the Digest type has very inefficient ones. Previously the Digest comparison loop was near the top of the profile while doing a bulk sync. This representation change is an improvement of ~7% in the time to validate the whole chain.
Fewer intermediate conversions in the abstractHashFromBytes and the FromCBOR instances.
The Attributes type can contain UnparsedFields but the generator was relying on the smart constructors mkAttributes and makeAddress which always used empty UnparsedFields. We need UnparsedFields for proper test coverage. Two tests rely on Attributes with no UnparsedFields: tests that are checking the size of the Attributes. This is reasonable. So they now use a new generator variant genAttributesNoUnparsedFields.
Add a criterion microbenchmark for the conversions to/from the
CompactUTxO types: CompactTxId, CompactTxIn, CompactAddress and
CompactTxOut.
These representation conversion functions are used for every single tx
intput and output in the chain, so they are heavily used. Before we
try to make any performance improvements to these conversion functions
we need to be able to measure the differences.
Also make the TxIn constructor fields strict so that the benchmark
measures the full evaluation. And it should be strict anyway.
Results on a AMD Ryzen 7 3700X:
benchmarking CompactUTxO/toCompactTxId
time 116.4 ns (116.1 ns .. 117.0 ns)
1.000 R² (1.000 R² .. 1.000 R²)
mean 117.6 ns (117.1 ns .. 118.0 ns)
std dev 1.466 ns (1.234 ns .. 1.778 ns)
variance introduced by outliers: 13% (moderately inflated)
benchmarking CompactUTxO/fromCompactTxId
time 113.3 ns (113.1 ns .. 113.5 ns)
1.000 R² (1.000 R² .. 1.000 R²)
mean 113.3 ns (113.1 ns .. 113.9 ns)
std dev 1.116 ns (462.6 ps .. 2.294 ns)
benchmarking CompactUTxO/toCompactTxIn
time 117.6 ns (117.2 ns .. 118.1 ns)
1.000 R² (1.000 R² .. 1.000 R²)
mean 118.0 ns (117.5 ns .. 119.0 ns)
std dev 2.098 ns (1.135 ns .. 4.083 ns)
variance introduced by outliers: 23% (moderately inflated)
benchmarking CompactUTxO/fromCompactTxIn
time 122.6 ns (122.4 ns .. 122.7 ns)
1.000 R² (1.000 R² .. 1.000 R²)
mean 122.6 ns (122.5 ns .. 122.8 ns)
std dev 599.7 ps (479.5 ps .. 801.0 ps)
benchmarking CompactUTxO/toCompactAddress
time 575.7 ns (573.4 ns .. 578.0 ns)
1.000 R² (1.000 R² .. 1.000 R²)
mean 577.9 ns (576.5 ns .. 578.9 ns)
std dev 4.143 ns (3.356 ns .. 5.098 ns)
benchmarking CompactUTxO/fromCompactAddress
time 820.8 ns (818.8 ns .. 823.1 ns)
1.000 R² (1.000 R² .. 1.000 R²)
mean 821.1 ns (819.4 ns .. 822.7 ns)
std dev 5.573 ns (4.501 ns .. 7.004 ns)
benchmarking CompactUTxO/toCompactTxOut
time 587.2 ns (585.7 ns .. 588.7 ns)
1.000 R² (1.000 R² .. 1.000 R²)
mean 587.1 ns (586.1 ns .. 588.1 ns)
std dev 3.179 ns (2.633 ns .. 4.010 ns)
benchmarking CompactUTxO/fromCompactTxOut
time 846.7 ns (841.9 ns .. 851.0 ns)
1.000 R² (1.000 R² .. 1.000 R²)
mean 842.3 ns (840.2 ns .. 844.9 ns)
std dev 8.043 ns (6.333 ns .. 10.14 ns)
intricate
reviewed
Apr 2, 2020
Comment on lines
+165
to
+168
| (indexPrimArray arr 0) | ||
| (indexPrimArray arr 1) | ||
| (indexPrimArray arr 2) | ||
| (indexPrimArray arr 3) |
There was a problem hiding this comment.
Is it worth checking the size of arr before doing this? I haven't worked with PrimArray before, so I'm not sure whether dereferencing at an out-of-bounds index will result in undefined behaviour or some kind of exception.
There was a problem hiding this comment.
It would indeed fail. But we know these are always length 4, because we made them with the functions in this module. And we don't export the constructors.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
No description provided.