Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create xdr.EncodingBuffer, which reduces buffer allocations #4056

Merged
merged 7 commits into from
Nov 10, 2021

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Nov 8, 2021

The improvement is considerable. It reduces the allocations (and CPU consumption) by roughly half.

goos: darwin
goarch: amd64
pkg: github.com/stellar/go/benchmarks
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
BenchmarkXDRMarshal
BenchmarkXDRMarshal-8                          	 1000000	      1050 ns/op	    1352 B/op	      14 allocs/op
BenchmarkXDRMarshalWithEncoder
BenchmarkXDRMarshalWithEncoder-8               	 1909111	       619.1 ns/op	     176 B/op	       9 allocs/op
BenchmarkGXDRMarshal
BenchmarkGXDRMarshal-8                         	  151929	      7880 ns/op	    2152 B/op	     157 allocs/op
BenchmarkXDRMarshalHex
BenchmarkXDRMarshalHex-8                       	  473662	      2199 ns/op	    3640 B/op	      19 allocs/op
BenchmarkXDRMarshalHexWithEncoder
BenchmarkXDRMarshalHexWithEncoder-8            	  846926	      1406 ns/op	    1072 B/op	      10 allocs/op
BenchmarkXDRUnsafeMarshalHexWithEncoder
BenchmarkXDRUnsafeMarshalHexWithEncoder-8      	 1000000	      1137 ns/op	     176 B/op	       9 allocs/op
BenchmarkXDRMarshalBase64
BenchmarkXDRMarshalBase64-8                    	  555267	      1918 ns/op	    3000 B/op	      19 allocs/op
BenchmarkXDRMarshalBase64WithEncoder
BenchmarkXDRMarshalBase64WithEncoder-8         	  998617	      1217 ns/op	     752 B/op	      10 allocs/op
BenchmarkXDRUnsafeMarshalBase64WithEncoder
BenchmarkXDRUnsafeMarshalBase64WithEncoder-8   	 1000000	      1048 ns/op	     176 B/op	       9 allocs/op
PASS

Note how xdr encoding now smashes the performance of the gxdr encoder (which used to be better than the xdr encoder).

@2opremio 2opremio mentioned this pull request Nov 8, 2021
7 tasks
@2opremio 2opremio force-pushed the optimize-unmarhsalling-buffers branch from 7e0e5e7 to ce099d4 Compare November 8, 2021 16:10
@2opremio
Copy link
Contributor Author

2opremio commented Nov 8, 2021

@bartekn I didn't replace every Marshaling invocation with the new xdr.Encoder, let me know if there are places you think I missed. (I didn't replace single-encoding invocations, since it's not worth it, but it may be worth it if we refactor the code to reuse an encoder)

@2opremio 2opremio marked this pull request as ready for review November 8, 2021 19:02
@2opremio 2opremio requested review from a team and leighmcculloch November 8, 2021 19:02
@2opremio 2opremio force-pushed the optimize-unmarhsalling-buffers branch from ce099d4 to 87d2cbe Compare November 8, 2021 19:39
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😎 Very cool, halving encoding time is great. One comment about where this new type lives.

I defer to @stellar/horizon-committers for the Horizon changes.

Note how xdr encoding now smashes the performance of the gxdr encoder (which used to be better than the xdr encoder).

I think the xdr package is already much faster than gxdr without this change, because of the stellar/xdrgen#65 changes, but this change makes it better, half is awesome.

xdr/main.go Outdated
Comment on lines 106 to 102
// Encoder reuses internal buffers between invocations
// to minimize allocations.
type Encoder struct {
encoder *xdr.Encoder
xdrEncoderBuf bytes.Buffer
otherEncodersBuf []byte
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 This type looks like something that should live in the go-xdr package, we could replace the existing Encoder there with this, or add it there as a new type ScratchEncoder, or something similar. Having it here be xdr.Encoder is a little confusing, there's no indication of when to use this over the one in the go-xdr package. So comments would benefit, and I think moving it to go-xdr would be best.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The encoder also takes care of bookeeping buffers for base64 and hex encoding which I don't think belongs to go-xdr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with @leighmcculloch. If we move the code to go-xdr we can still do base64/hex stuff here.

Copy link
Member

@leighmcculloch leighmcculloch Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can move the base64 to go-xdr too. Base64ing xdr is generally helpful.

Copy link
Contributor

@bartekn bartekn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2x speed improvement? WOW! 😮 LGreatTM!

@bartekn I didn't replace every Marshaling invocation with the new xdr.Encoder, let me know if there are place you think I missed

AFAIR, ChangeCompactor takes a bunch of time during ingestion.

otherEncodersBuf []byte
}

func growSlice(old []byte, newSize int) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we use bytes.Buffer for xdr encoder but this custom code for other encoders?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@2opremio I think we can get rid of the extra base64 buffer and this grow code by streaming through the base64 encoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The base64 (and hex) encoders need a buffer to write to. This would allow us to get rid of the grow code, but not the buffer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, base64 (and hex) the encoders don't have a reset function and we don't want to allocate a new encoder every time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure you can reuse them after calling Close. For example:

	b1 := strings.Builder{}
	e1 := base64.NewEncoder(base64.StdEncoding, &b1)
	e1.Write([]byte("Hello World"))
	e1.Close()
	fmt.Println(b1.String())
	
	b1.Reset()
	e1.Write([]byte("Hello World"))
	e1.Close()
	fmt.Println(b1.String())

Ref: https://play.golang.org/p/kDifIOFjblC

Note the use of a strings.Builder, which might be better than writing to a []byte and converting it into a string. So maybe you choose in the moment which type of buffer you use dependent on whether you want binary or base64. Use a bytes.Buffer with no base64 encoder when you want binary. Use strings.Builder with a base64 encoder when you want a string.

Copy link
Contributor Author

@2opremio 2opremio Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strings builder won't perform well because (from what I have read) resetting dereferences the intermediate buffer, causing a allocations in every encoding invocation.

xdr/main.go Outdated
Comment on lines 106 to 102
// Encoder reuses internal buffers between invocations
// to minimize allocations.
type Encoder struct {
encoder *xdr.Encoder
xdrEncoderBuf bytes.Buffer
otherEncodersBuf []byte
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with @leighmcculloch. If we move the code to go-xdr we can still do base64/hex stuff here.

xdr/main.go Outdated
// Subsequent calls to marshalling methods will overwrite the returned buffer.
func (e *Encoder) UnsafeMarshalBinary(v interface{}) ([]byte, error) {
e.xdrEncoderBuf.Reset()
if encodable, ok := v.(xdrEncodable); ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we move the logic in this function to go-xdr, this xdr.Encider type can disappear if instead of base64ing a byte slice, you store a bytes.Buffer in all the places you're storing the xdr.Encoder, and wrap it in a base64.Encoder whenever writing to it. This way you don't need two buffers, only one. Also we don't need any new xdr encoder types. We make the existing xdr encoder type better by using the generated EncodeTo functions when they're present.

@2opremio
Copy link
Contributor Author

2opremio commented Nov 9, 2021

I have been thinking about it and I disagree about incorporating hex and base64 decoding into go-xdr. It breaks the separation of concerns.

For instance, using base64 decoding there wouldn't allow us to implement #4052 easily.

I am happy to change the name of the encoder to InplaceEncoder (or similar) though.

@leighmcculloch I am fine with moving the EncodeTo interface to go-xdr though. That will also allow us to implement stellar/xdrgen#65 (comment) . I will create a PR soon but let's leave the code in this PR as is (I will fix it later on).

@leighmcculloch
Copy link
Member

I agree, we don't need to add hex or base64 encoding to go-xdr. If you move the EncodeTo use to go-xdr, this PR can change to simply embed a bytes.Buffer into the Horizon types, and use https://pkg.go.dev/encoding/base64#NewEncoder, and nothing needs to change in the xdr package?

@2opremio
Copy link
Contributor Author

2opremio commented Nov 9, 2021

this PR can change to simply embed a bytes.Buffer into the Horizon types

What types?

@2opremio
Copy link
Contributor Author

2opremio commented Nov 9, 2021

and nothing needs to change in the xdr package

I don't see how we can do this without changing the xdr package, please elaborate

@2opremio 2opremio force-pushed the optimize-unmarhsalling-buffers branch from d2ce1cd to 0d8c529 Compare November 9, 2021 21:08
@leighmcculloch
Copy link
Member

leighmcculloch commented Nov 9, 2021

I'm doing a poor job getting my suggestion across, so I'll try to detail it.

I think if we add EncodeTo support to go-xdr you get functionality via composition and you can avoid two buffers, growing the slice, etc.

import xdr3 "github.com/stellar/go-xdr/xdr3"

buf := strings.Builder{}
b64 := base64.NewEncoder(base64.StdEncoding, &buf)
enc := xdr3.NewEncoder(&b64)
enc.Encode(xdrObj)
b64.Close()
str := buf.String() // <<< Base64 XDR encoded as a string
buf.Reset()

You could store buf, b64, and enc in the Horizon types.

That does seem cumbersome though, so yeah I think you are correct, a type in the xdr package still makes sense. That type could be slimmer than what's here though, and use a single buffer like above.

type StringEncoder struct {
	w strings.Builder
	enc xdr3.Encoder
	flush func()
}

func NewBase64StringEncoder() *StringEncoder {
	enc := &ResetEncoder{}
	b64 := base64.NewEncoder(base64.StdEncoding, enc.w)
	enc.enc = xdr3.NewEncoder(&b64)
	enc.flush = b64.Close
	return enc
}

func NewHexStringEncoder() *StringEncoder {
	enc := &ResetEncoder{}
	h := hex.NewEncoder(enc.w)
	enc.enc = xdr3.NewEncoder(h)
	enc.flush = func() {}
	return enc
}

func (e *StringEncoder) EncodeToString(v interface{}) (string, error) {
	err := e.Encode(v)
	if err != nil {
		return "", err
	}
	str := e.String()
	e.Reset()
	return str, nil
}

func (e *StringEncoder) Encode(v interface{}) error {
	return e.enc.Encode(v)
}

func (e *StringEncoder) String() string {
	e.flush()
	return e.w.String()
}

func (e *StringEncoder) Reset() {
	e.flush()
	e.w.Reset()
}

Which can then be used like:

type ClaimableBalancesChangeProcessor struct {
	stringEncoder     *xdr.StringEncoder
	// ...
}

p := &ClaimableBalancesChangeProcessor{
	stringEncoder:     xdr.NewHexStringEncoder(),
	// ...
}

id, err := p.stringEncoder.EncodeToString(cBalance.BalanceId)

You could have a similar BufferEncoder too for binary data, but from what I could see in this PR we don't have a use for binary output.

I think something like this would work to reduce the buffers, etc. Let me know if I'm way off base.

Feel free to go with what you've got though if this doesn't address the problem.

@2opremio
Copy link
Contributor Author

2opremio commented Nov 9, 2021

OK, much more clear. I will take it into consideration!

@2opremio 2opremio force-pushed the optimize-unmarhsalling-buffers branch 5 times, most recently from 864e6a4 to f3e20d3 Compare November 10, 2021 04:54
@2opremio
Copy link
Contributor Author

2opremio commented Nov 10, 2021

@leighmcculloch thanks for the suggestions, but I don't think that a strings builder is usable since resetting it deallocates it's internal buffer, causing allocations in every encoding call.

@2opremio
Copy link
Contributor Author

See golang/go#24716

@leighmcculloch
Copy link
Member

The same pattern should work with a bytes.Buffer?

@2opremio
Copy link
Contributor Author

2opremio commented Nov 10, 2021

Yeah, but then I don't really see an advantage compared to the existing code. In fact, I suspect it may be less performant. I will give it a try though.

@2opremio 2opremio changed the title Create xdr.Encoder, which reduces buffer allocations Create xdr.EncodingBuffer, which reduces buffer allocations Nov 10, 2021
@2opremio
Copy link
Contributor Author

I moved the logic of EncodeTo checking to go-xdr. Please take a look at stellar/go-xdr#17

@2opremio
Copy link
Contributor Author

2opremio commented Nov 10, 2021

I will update xdrgen to generate comply with the new Marshaler interface as opposed to EncodeTo. Stay put :)

@2opremio
Copy link
Contributor Author

Actually, I don't think stellar/go-xdr#17 is worth the pain since it disallows unmarshaling by pointer in unaddressable values, impacting performance. I will merge as is.

@2opremio
Copy link
Contributor Author

See stellar/xdrgen#67 (comment) for details.

It reduces a few a allocations but it seems to reduce CPU consumption by ~7%

```
goos: darwin
goarch: amd64
pkg: github.com/stellar/go/benchmarks
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
BenchmarkXDRMarshal
BenchmarkXDRMarshal-8                          	  102685	     11346 ns/op	    5128 B/op	     156 allocs/op
BenchmarkXDRMarshalWithEncoder
BenchmarkXDRMarshalWithEncoder-8               	  106569	     10636 ns/op	    3920 B/op	     150 allocs/op
BenchmarkXDRMarshalHex
BenchmarkXDRMarshalHex-8                       	   94836	     12340 ns/op	    6920 B/op	     158 allocs/op
BenchmarkXDRMarshalHexWithEncoder
BenchmarkXDRMarshalHexWithEncoder-8            	  104115	     11393 ns/op	    4816 B/op	     151 allocs/op
BenchmarkXDRUnsafeMarshalHexWithEncoder
BenchmarkXDRUnsafeMarshalHexWithEncoder-8      	  104821	     11024 ns/op	    3920 B/op	     150 allocs/op
BenchmarkXDRMarshalBase64
BenchmarkXDRMarshalBase64-8                    	   96364	     12162 ns/op	    6280 B/op	     158 allocs/op
BenchmarkXDRMarshalBase64WithEncoder
BenchmarkXDRMarshalBase64WithEncoder-8         	  104962	     11236 ns/op	    4496 B/op	     151 allocs/op
BenchmarkXDRUnsafeMarshalBase64WithEncoder
BenchmarkXDRUnsafeMarshalBase64WithEncoder-8   	  102901	     11042 ns/op	    3920 B/op	     150 allocs/op
```
@2opremio 2opremio force-pushed the optimize-unmarhsalling-buffers branch from f3e20d3 to b434d7d Compare November 10, 2021 15:33
@2opremio 2opremio force-pushed the optimize-unmarhsalling-buffers branch from d0e0784 to 8494f90 Compare November 10, 2021 16:03
@2opremio 2opremio merged commit 5db7e40 into stellar:master Nov 10, 2021
@2opremio 2opremio deleted the optimize-unmarhsalling-buffers branch November 10, 2021 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants