schema/gen/go: avoid Maybe pointers for small types #160
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
warpfork
reviewed
Apr 6, 2021
If we know that a schema type can be represented in Go with a small amount of bytes, using a pointer to store its "maybe" is rarely a good idea. For example, an optional string only weighs twice as much as a pointer, so a pointer adds overhead and will barely ever save any memory. Add a function to work out the byte size of a schema.TypeKind, relying on reflection and the basicnode package. Debug prints are also present if one wants to double-check the numbers. As of today, they are: sizeOf(small): 32 (4x pointer size) sizeOf(Bool): 1 sizeOf(Int): 8 sizeOf(Float): 8 sizeOf(String): 16 sizeOf(Bytes): 24 sizeOf(List): 24 sizeOf(Map): 32 sizeOf(Link): 16 Below is the result on go-merkledag's BenchmarkRoundtrip after re-generating go-codec-dagpb with this change. Note that the dag-pb schema contains multiple optional fields, such as strings. name old time/op new time/op delta Roundtrip-8 4.24µs ± 3% 3.78µs ± 0% -10.87% (p=0.004 n=6+5) name old alloc/op new alloc/op delta Roundtrip-8 6.38kB ± 0% 6.24kB ± 0% -2.26% (p=0.002 n=6+6) name old allocs/op new allocs/op delta Roundtrip-8 103 ± 0% 61 ± 0% -40.78% (p=0.002 n=6+6) Schema typekinds which don't directly map to basicnode prototypes, such as structs and unions, are left as a TODO for now. I did not do any measurements to arrive at the magic number of 4x, which is documented in the code. We might well increase it in the future, with more careful benchmarking. For now, it seems like a conservative starting point that should cover all basic types. Finally, re-generate within this repo.
|
I've re-done this with reflection and basicnode, which means it's no longer an estimation, and will stay up to date as the internals change. If there is a better place to grab Go values/types than basicnode for these measurements, I'm all ears. Perhaps some code already generated by schema/gen/go? Anyway, there's zero behavioral change compared to before. Another |
|
Ahh, right. We can't readily sizeof on the types before they're generated. This does still seem like a reasonable heuristic though, and certainly does do the right thing right now for practical values. 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
(see commit message)