xdr: update using xdrgen#65 #3957
Conversation
|
@bartekn I've fixed the linter errors in this PR. |
|
Started verify-range on last 100k ledgers: [37581877, 37681877]. I'll have results in a few hours. |
|
@leighmcculloch good news, |
|
@leighmcculloch would you mind rebasing? I did so at https://github.com/stellar/go/tree/perf-xdr myself but I cannot force-push to your branch. |
|
@2opremio I've merged master into this PR, and rerun stellar/xdrgen#65 on the .x files that live in this repo. |
|
@stellar/horizon-committers do we want to include this in next Horizon release. This time we have full week of testing. |
|
I think so - if we can also do fuzzing in parallel to our usual testing this week. @leighmcculloch would you be able to do that? |
|
@ire-and-curses I can't dedicate anymore time to this in the immediate future, nor do I have the expertise the Horizon team does in fuzzing this in the context of Horizon's use cases. If your team can do that, that would be very helpful. |
|
OK, we're going to take care of this. @stellar/horizon-committers whoever is testing next week please merge this into release branch. |
|
I can get this branch updated to target the release branch. Just let me know which branch and when ready. I'll merge the xdrgen change that is still open. |
|
The release branch will be branched off |
There was a problem hiding this comment.
@bartekn The PR is updated with master now. Let me know if anything else needs changing.
There's also one thing I noticed that might be another opportunity for improvement I'll try at a later date.
| func (s TransactionV1Envelope) MarshalBinary() ([]byte, error) { | ||
| b := new(bytes.Buffer) | ||
| _, err := Marshal(b, s) | ||
| b := bytes.Buffer{} | ||
| e := xdr.NewEncoder(&b) | ||
| err := s.EncodeTo(e) | ||
| return b.Bytes(), err |
There was a problem hiding this comment.
@2opremio I've been thinking about the work you did recently in stellar/go-xdr#16 and wondering if we can apply the same pattern you came up with to use a predefined fixed size buffer here too.
One of the downsides of using bytes.Buffer is that it will start with a smallish sized buffer and keep growing the buffer, which is probably allocations on the heap.
Many of our XDRs structs have a well defined maximum size because most of our variable length fields have a small maximum set. We could replace bytes.Buffer with our own fixed size buffers. This might not work if the buffers are to big to fit on the stack.
For example, if the max size of a transaction is 1kb, we could define a type:
type FixedBuffer1024 struct {
buf [1024]byte
len int
}
func (b *FixedBuffer1024) Write(b []byte) (int, error) {
// ...
}For each sized buffer we need we define a new type. With Go generics one day we could probably reduce all these down to one type, but we don't need generics to do it since we're already code generating.
Do you think this would help? I can try it out when I come back to xdrgen to do the decoding.
cc @bartekn
There was a problem hiding this comment.
Do we need new structs, though? What about:
b := make([]byte, 0, 1024)We can then pass to a single struct working on a provided slice? I think it will work the same wrt to allocations and we won't need to have so many types.
There was a problem hiding this comment.
I think this would benefit even more from a variant of MarshalBinary() which writes the output to a buffer you pass:
func (s TransactionV1Envelope) MarshalBinaryToBuffer(b *bytes.Buffer) error {
e := xdr.NewEncoder(&b)
err := s.EncodeTo(e)
return b.Bytes(), err
}
// less performant, but convenient
func (s TransactionV1Envelope) MarshalBinary() ([]byte, error) {
b := bytes.NewBuffer(make([]byte, somenumber)) // as indicated by bartek, somenumber should (generously) approximate the estimated size of the output. We would rather consume a bit more memory, but reduce the allocations
err := s.MarshalBinaryToBuffer(&b)
return b.Bytes(), err
}In that way, a consumer which decodes continuously (which, I think we have a few) would be able to reuse the buffer across calls, considerably reducing allocations:
func ProcessTransactions() {
var b bytes.Buffer // TODO: It would be even better to use a buffer with a reasonable
// initial size, maybe we could generate EstimatedBinarySize()
// methods for the xdr types? (it's probably an overoptimization though)
for {
t := getTransactionEnvelope()
b.Reset() // make sure the existing underlying buffer is reused
t.MarshalBinaryToBuffer(&b)
... // do something with b.Bytes()
}
}We could even add a reset method to the Encoder/Decoder to replace the underlying reader and writer to reduce the allocations even further.
func ProcessTransactions() {
var b bytes.Buffer // TODO: It would be even better to use a buffer with a reasonable
// initial size, maybe we could generate EstimatedBinarySize()
// methods for the xdr types? (it's probably an overoptimization though)
var e xdr.Encoder
for {
t := getTransactionEnvelope()
b.Reset() // make sure the existing underlying buffer is reused
e.Reset(&b) // maybe not even necessary since the underlying reader didn't change
s.EncodeTo(e)
... // do something with b.Bytes()
}
}I think I will try doing something like this for functions in the critical path and see whether it makes a big difference. Then we can look into how to productize if necessary (it's probably fine to simply not use MarshalBinary for things in the critical path)
@leighmcculloch thanks for fostering brainstorming about this!
There was a problem hiding this comment.
Ah yes, those are better ideas. If we can share a buffer let's do it. I think your comment is correct, we don't need the reset function on the encoder since in your example there's already the reset being used on the buffer. The encoder stores no meaningful state. So I think we have everything we need already to do the reuse. 🎉
There was a problem hiding this comment.
Can we track this idea somewhere? New issue?
There was a problem hiding this comment.
I have something better. A PR implementing it :)
|
@leighmcculloch I rerun a spurious failing test in Circle and now everything passes. Could you rebase again (sorry :S) and merge right after it succeeds? |
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Update XDR using stellar/xdrgen#65 that improves the encoding cpu and memory usage.
Why
See discussion on stellar/xdrgen#65.