node/bindnode: more minor improvements for graphsync #331
Conversation
|
Note to self: revisit bindnode's Next key being valid only until the next call. |
There was a problem hiding this comment.
Most of these make sense.
I concede on the TypeStruct.Fields accessor and TypeEnum.Members accessor for sure. (I keep wanting Golang to have immutablity. It doesn't. I should stop wishing I can have nice things; and paying extra for them isn't worth it.)
I'm fairly nervous about the iterators returning something as a Node that's mutable. Maybe with enough documentation, it's going to be survivable. Still nerve-wracking and a fairly large divergence from the usual contract.
I wish we could make the iterators for structs and unions (e.g. what's wishing for these string wrappers) just grab pointers to "constant" strings that the schema.Type values already contain. That's hard at present because that package also spent effort on trying to be immutable in a language that's inimically hostile to it... but maybe it's worth drilling there instead.
Querying the fields and members of structs and enums is common. I get the intent of trying to keep them read-only, but the current mechanism is just too expensive. name old time/op new time/op delta MessageEncodingRoundtrip/DagCbor-16 21.4µs ± 1% 16.9µs ± 0% -20.66% (p=0.029 n=4+4) name old alloc/op new alloc/op delta MessageEncodingRoundtrip/DagCbor-16 32.8kB ± 0% 23.2kB ± 0% -29.28% (p=0.029 n=4+4) name old allocs/op new allocs/op delta MessageEncodingRoundtrip/DagCbor-16 451 ± 0% 396 ± 0% -12.20% (p=0.029 n=4+4) An alternative could be to provide a slice, and then we'd append into it and reuse its space. However, note that that would still copy memory. An alternative could be to provide iterator-like methods, such as: Length() int FieldAt(int) StructField However, that would get relatively clunky, and doesn't seem particularly worth it right now. Ultimately, we don't have read-only slices, so we shouldn't try to invent our own. The fact that we return the direct slice does allow replacing elements, but the user can't append or re-slice, so that should be enough signal to most Go developers.
|
After talking to @warpfork about it yesterday, I ended up backing out my All that doesn't seem worth it for a minor speed-up right now. I might investigate other avenues which don't lose us safety in the near future. |
Taking this to mean that you're OK with that one commit, and given that the other two were removed, I'm going ahead and merging this :) |
(see commit messages - please do not squash)