Improve the performance of Go generated marshaling/encoding code #65

leighmcculloch · 2021-09-22T18:06:22Z

What

Change how the generated Go code marshals objects to binary to move logic from runtime to compile time. Specifically:

Add an EncodeTo function to all types that takes an xdr.Encoder and uses the encoder to encode itself.
Have the EncodeTo functions call the appropriate lower level encode function for their type, such as EncodeUint, instead of Encode that will use reflection to figure out the type.
Have the EncodeTo functions of complex types (structs, unions) call the EncodeTo function of each of their fields.

For any types not generated, such as time.Time, encoding falls back to the current reflection based approach.

This change affects marshaling/encoding only, and not unmarshaling/decoding.

Example

For an example of how this changes generated Go code, see stellar/go#3957.

Why

I noticed while working on stellar-deprecated/starlight#40 that the application was spending as much time building transactions as it was ed25519 verifying which is a bit of a red flag given that XDR's simple and non-self-descriptive format should be performant.

On inspection the code generated relies on reflection for encoding all types. Reflection is known to not be very performant and results in allocations that would be otherwise unnecessary.

A simple benchmark of calling MarshalBinary on xdr.TransactionEnvelope from github.com/stellar/go/xdr demonstrates this well. The same benchmark running against code generated by this pull request significantly reduces encoding time and allocations.

Before:

goos: darwin
goarch: amd64
pkg: github.com/stellar/go
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkXDRUnmarshal-8           107223             10231 ns/op            3536 B/op        149 allocs/op
BenchmarkGXDRUnmarshal-8           71019             16009 ns/op           54185 B/op        172 allocs/op
BenchmarkXDRMarshal-8             162579              6754 ns/op            3280 B/op        118 allocs/op
BenchmarkGXDRMarshal-8            252350              4398 ns/op            1312 B/op         95 allocs/op
PASS
ok      github.com/stellar/go   5.010s

After:

goos: darwin
goarch: amd64
pkg: github.com/stellar/go
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkXDRUnmarshal-8           104326             10106 ns/op            3536 B/op        149 allocs/op
BenchmarkGXDRUnmarshal-8           67959             16021 ns/op           54185 B/op        172 allocs/op
BenchmarkXDRMarshal-8            1287020               932.7 ns/op          1360 B/op         34 allocs/op
BenchmarkGXDRMarshal-8            288548              4161 ns/op            1312 B/op         95 allocs/op
PASS
ok      github.com/stellar/go   7.053s

Benchmark code and some additional tests run inside stellar/go

package benchmarks

import (
	"bytes"
	"encoding/base64"
	"testing"

	"github.com/stellar/go/gxdr"
	"github.com/stellar/go/strkey"
	"github.com/stellar/go/xdr"
	"github.com/stretchr/testify/require"
	goxdr "github.com/xdrpp/goxdr/xdr"
)

const input64 = "AAAAAgAAAADy2f6v1nv9lXdvl5iZvWKywlPQYsZ1JGmmAfewflnbUAAABLACG4bdAADOYQAAAAEAAAAAAAAAAAAAAABhSLZ9AAAAAAAAAAEAAAABAAAAAF8wDgs7+R5R2uftMvvhHliZOyhZOQWsWr18/Fu6S+g0AAAAAwAAAAJHRE9HRQAAAAAAAAAAAAAAUwsPRQlK+jECWsJLURlsP0qsbA/aIaB/z50U79VSRYsAAAAAAAAAAAAAAYMAAA5xAvrwgAAAAAAAAAAAAAAAAAAAAAJ+WdtQAAAAQCTonAxUHyuVsmaSeGYuVsGRXgxs+wXvKgSa+dapZWN4U9sxGPuApjiv/UWb47SwuFQ+q40bfkPYT1Tff4RfLQe6S+g0AAAAQBlFjwF/wpGr+DWbjCyuolgM1VP/e4ubfUlVnDAdFjJUIIzVakZcr5omRSnr7ClrwEoPj49h+vcLusagC4xFJgg="

var input = func() []byte {
	input, err := base64.StdEncoding.DecodeString(input64)
	if err != nil {
		panic(err)
	}
	return input
}()

func BenchmarkXDRUnmarshal(b *testing.B) {
	te := xdr.TransactionEnvelope{}

	// Make sure the input is valid.
	err := te.UnmarshalBinary(input)
	require.NoError(b, err)

	// Benchmark.
	for i := 0; i < b.N; i++ {
		_ = te.UnmarshalBinary(input)
	}
}

func BenchmarkGXDRUnmarshal(b *testing.B) {
	te := gxdr.TransactionEnvelope{}

	// Make sure the input is valid, note goxdr will panic if there's a
	// marshaling error.
	te.XdrMarshal(&goxdr.XdrIn{In: bytes.NewReader(input)}, "")

	// Benchmark.
	r := bytes.NewReader(input)
	for i := 0; i < b.N; i++ {
		r.Reset(input)
		te.XdrMarshal(&goxdr.XdrIn{In: r}, "")
	}
}

func BenchmarkXDRMarshal(b *testing.B) {
	te := xdr.TransactionEnvelope{}

	// Make sure the input is valid.
	err := te.UnmarshalBinary(input)
	require.NoError(b, err)
	output, err := te.MarshalBinary()
	require.NoError(b, err)
	require.Equal(b, input, output)

	// Benchmark.
	for i := 0; i < b.N; i++ {
		_, _ = te.MarshalBinary()
	}
}

func BenchmarkGXDRMarshal(b *testing.B) {
	te := gxdr.TransactionEnvelope{}

	// Make sure the input is valid, note goxdr will panic if there's a
	// marshaling error.
	te.XdrMarshal(&goxdr.XdrIn{In: bytes.NewReader(input)}, "")
	output := bytes.Buffer{}
	te.XdrMarshal(&goxdr.XdrOut{Out: &output}, "")

	// Benchmark.
	for i := 0; i < b.N; i++ {
		output.Reset()
		te.XdrMarshal(&goxdr.XdrOut{Out: &output}, "")
	}
}

func TestXDRMarshalLedgerEntryExtensionV1(t *testing.T) {
	te := xdr.LedgerEntryExtensionV1{}
	output, err := te.MarshalBinary()
	require.NoError(t, err)
	t.Logf(base64.StdEncoding.EncodeToString(output))

	address := "GBEUFD3PR6DY3JX3QI76GVNUZBOLDNAS6KODSXXKTLJ7TKIK5RF6HESR"
	accountID := xdr.AccountId{}
	accountID.SetAddress(address)

	te = xdr.LedgerEntryExtensionV1{SponsoringId: &accountID}
	output, err = te.MarshalBinary()
	require.NoError(t, err)
	t.Logf(base64.StdEncoding.EncodeToString(output))
}

func TestGXDRMarshalLedgerEntryExtensionV1(t *testing.T) {
	te := gxdr.LedgerEntryExtensionV1{}
	output := bytes.Buffer{}
	te.XdrMarshal(&goxdr.XdrOut{Out: &output}, "")
	t.Logf(base64.StdEncoding.EncodeToString(output.Bytes()))

	address := "GBEUFD3PR6DY3JX3QI76GVNUZBOLDNAS6KODSXXKTLJ7TKIK5RF6HESR"
	accountID := &gxdr.AccountID{Type: gxdr.PUBLIC_KEY_TYPE_ED25519}
	ed25519 := accountID.Ed25519()
	rawEd25519, err := strkey.Decode(strkey.VersionByteAccountID, address)
	require.NoError(t, err)
	copy(ed25519[:], rawEd25519)

	te = gxdr.LedgerEntryExtensionV1{SponsoringID: accountID}
	output = bytes.Buffer{}
	te.XdrMarshal(&goxdr.XdrOut{Out: &output}, "")
	t.Logf(base64.StdEncoding.EncodeToString(output.Bytes()))
}

func TestXDRMarshalAccountEntryExtensionV2(t *testing.T) {
	te := xdr.AccountEntryExtensionV2{}
	output, err := te.MarshalBinary()
	require.NoError(t, err)
	t.Logf(base64.StdEncoding.EncodeToString(output))

	address := "GBEUFD3PR6DY3JX3QI76GVNUZBOLDNAS6KODSXXKTLJ7TKIK5RF6HESR"
	accountID := xdr.AccountId{}
	accountID.SetAddress(address)

	te = xdr.AccountEntryExtensionV2{
		SignerSponsoringIDs: []xdr.SponsorshipDescriptor{
			nil,
			&accountID,
			nil,
		},
	}
	output, err = te.MarshalBinary()
	require.NoError(t, err)
	t.Logf(base64.StdEncoding.EncodeToString(output))
}

func TestGXDRMarshalAccountEntryExtensionV2(t *testing.T) {
	te := gxdr.AccountEntryExtensionV2{}
	output := bytes.Buffer{}
	te.XdrMarshal(&goxdr.XdrOut{Out: &output}, "")
	t.Logf(base64.StdEncoding.EncodeToString(output.Bytes()))

	address := "GBEUFD3PR6DY3JX3QI76GVNUZBOLDNAS6KODSXXKTLJ7TKIK5RF6HESR"
	accountID := &gxdr.AccountID{Type: gxdr.PUBLIC_KEY_TYPE_ED25519}
	ed25519 := accountID.Ed25519()
	rawEd25519, err := strkey.Decode(strkey.VersionByteAccountID, address)
	require.NoError(t, err)
	copy(ed25519[:], rawEd25519)

	te = gxdr.AccountEntryExtensionV2{
		SignerSponsoringIDs: []gxdr.SponsorshipDescriptor{
			nil,
			accountID,
			nil,
		},
	}
	output = bytes.Buffer{}
	te.XdrMarshal(&goxdr.XdrOut{Out: &output}, "")
	t.Logf(base64.StdEncoding.EncodeToString(output.Bytes()))
}

This change only focuses on marshaling/encoding because unmarshaling is significantly more complex and not required for the use cases I'm interested, building and hashing transactions.

Optional types such as SponsorshipDescriptor from the Stellar XDR complicate this encoding somewhat, although not significantly.

Known Limitations

N/A

#336) Update the version of stellar/go used in this repo to include the new commits that contain the xdr generated with stellar/xdrgen#65. stellar/xdrgen#65 improves the performance of xdr encoding.

bartekn

LGTM! It should improve ingestion speed in Horizon. Probably fixes stellar/go#2689 and stellar/go#3256.

bartekn · 2021-10-05T11:34:36Z

lib/xdrgen/generators/go.rb

+              out.puts "  }"
+            end
+          when :array
+            out.puts "  for i := 0; i < len(#{var}); i++ {"


Should we check max_length tag here? I'm not sure if the previous code checks this but if it does we probably should have it here too.

I don't think it is necessary because the type of var in the :array case is a fixed size array, and in Go we will represent it as an array, which has a fixed size at compile time. For example:

https://github.com/stellar/go/blob/a1db2a6b1fd3a444cdc9ac7250144e28d5de5354/xdr/xdr_generated.go#L18912-L18916

However, we could check max_length for the :var_array case which we represent as a Go slice and therefore has an unbounded size. The code that does encoding of variable length arrays today (src: https://github.com/stellar/go-xdr/blob/b95df30963cd0a4fcd12f2d65db0f9aeba987f73/xdr3/encode.go#L404-L427) doesn't check the max length, so that would be a functional change. I think if we are to pursue that we should pursue that separately to this PR.

ire-and-curses · 2021-10-05T15:37:22Z

LGTM! It should improve ingestion speed in Horizon. Probably fixes stellar/go#2689 and stellar/go#3256.

Note that this PR doesn't improve unmarshaling, so there may be more we can squeeze out in the future.

bartekn · 2021-10-05T15:38:23Z

Right, but TransactionProcessor is only marshaling.

leighmcculloch · 2021-10-05T16:18:42Z

TransactionProcessor is only marshaling

I'm wondering if this is an opportunity to test this new marshaling code. How would we test that the marshaling is consistent in Horizon with this change? If Horizon marshals XDR during ingestion, if we rebuilt Horizon using xdr generated by this PR, and ran a full history ingestion, would that be sufficient at identifying any problems marshaling that data? Would Horizon error if the marshaling resulted in different XDR?

bartekn · 2021-10-05T16:23:09Z

We can easily confirm this by verify-range test that we run before every release (almost - we don't if there are no ingestion changes). We can run a limited (like 100k ledgers) test against a branch (like this PR branch) to confirm the result are the same.

bartekn · 2021-10-05T22:53:04Z

Also, it looks like staticcheck found some issues in generated code: https://github.com/stellar/go/pull/3957/checks?check_run_id=3705045608

leighmcculloch · 2021-10-05T22:54:42Z

Also, it looks like staticcheck found some issues in generated code: https://github.com/stellar/go/pull/3957/checks?check_run_id=3705045608

Yup, and this is really annoying, because it is much easier to generate the code if we write it this way. I think we should probably stop running linters on generated code, or at least disable some linters like this that really have zero utility for generated code.

leighmcculloch · 2021-10-06T00:06:14Z

@bartekn I got stellar/go#3957 lint errors fixed. Could you help with running a verify-range test using that PR?

bartekn · 2021-10-18T15:29:40Z

@leighmcculloch the verify-range did not find any issues. Do you think we can merge this and stellar/go#3957? I'm looking forward for this performance bump.

2opremio · 2021-10-18T16:04:31Z

@leighmcculloch just to clarify, we will happily make a Horizon release just for this. The sooner we merge it the sooner we will release it :)

leighmcculloch · 2021-10-18T16:16:43Z

@bartekn @2opremio I'm good to merge this, however @bartekn left a comment stellar/go#3957 (comment) recommending we also use randxdr to fuzz the generated code. I think that could be a good idea, but I don't have capacity to handle that immediately. Is that something you could help out with?

2opremio · 2021-10-20T16:36:29Z

@leighmcculloch where is the BenchmarkXDRMarshal code you are referring to in the PR description?

leighmcculloch · 2021-10-20T20:54:28Z

@2opremio It is embedded in the PR description. Click the little arrow next to Benchmark code and some additional tests run inside stellar/go.

2opremio · 2021-11-08T18:41:46Z

I know this arrives very late ... but wouldn't it be better to use:

type xdrEncodable interface {
	EncodeTo(e *xdr.Encoder) error
}

// Marshal writes an xdr element `v` into `w`.
func Marshal(w io.Writer, v interface{}) (int, error) {
	if encodable, ok := v.(xdrEncodable); ok {
		b := bytes.Buffer{}
		e := xdr.NewEncoder(&b)
		err := encodable.EncodeTo(e)
		return w.Write( b.Bytes())
	}
	// delegate to xdr package's Marshal
	return xdr.Marshal(w, v)
}

instead of:

type xdrType interface {
	xdrType()
}

// Marshal writes an xdr element `v` into `w`.
func Marshal(w io.Writer, v interface{}) (int, error) {
	if _, ok := v.(xdrType); ok {
		if bm, ok := v.(encoding.BinaryMarshaler); ok {
			b, err := bm.MarshalBinary()
			if err != nil {
				return 0, err
			}
			return w.Write(b)
		}
	}
	// delegate to xdr package's Marshal
	return xdr.Marshal(w, v)
}

Using xdrType is artificial an noisy

leighmcculloch · 2021-11-08T18:47:04Z

That looks cleaner.

My only hesistancy is I wouldn't want to move the bytes.Buffer up to a generic function disconnected from the type, because I hope to replace bytes.Buffer with fixed sized buffers based on max size of the types, and we can only do that if the buffer is defined inside the MarshalBinary, and if xdr.Marshal calls MarshalBinary.

We could still test on xdr.xdrEncoderable, but then call .MarshalBinary, but that doesn't completely get rid of the oddity.

2opremio · 2021-11-08T18:51:42Z

I don't think we need fixed buffers, see stellar/go#4056

Update XDR using stellar/xdrgen#65 that improves the encoding cpu and memory usage.

leighmcculloch added 29 commits September 18, 2021 02:43

Skip Go generated binary interface on optional typedefs

7e8f088

fix

c3561d4

fix

a272f90

perf: xdrgen go generator

96555eb

wip

13692d0

wip

aa17e1f

wip

1c85e6d

wip

ca17398

wip

1abeadb

wip

b0cd537

wip

cade9f9

Merge branch 'master' into perf-xdrgen

9b89f2e

wip

40a3b1c

wip

95ce3ff

wip

152b800

wip

30f4263

wip

3b17d20

wip

210bb99

it works

8c52177

do not marshalbinary types outside the package

df7cdbb

safer

d8c3cb1

organize

b35ac8a

consistency

b64aaf3

wip

b4c719d

wip

c270798

handle edge

705dd0c

wip optional types

179f002

Fix optionals

2bd3d00

fix optional types inside arrays

5005b98

leighmcculloch mentioned this pull request Sep 24, 2021

xdr: update using xdrgen#65 stellar/go#3957

Merged

7 tasks

leighmcculloch requested a review from ire-and-curses September 24, 2021 22:12

leighmcculloch assigned tamirms and unassigned tamirms Sep 24, 2021

leighmcculloch requested a review from tamirms September 24, 2021 22:12

bartekn approved these changes Oct 5, 2021

View reviewed changes

leighmcculloch self-assigned this Oct 19, 2021

Merge branch 'master' into perf-xdrgen

e200027

leighmcculloch mentioned this pull request Nov 2, 2021

Profile Horizon stellar/go#4022

Closed

2opremio mentioned this pull request Nov 2, 2021

Add stellar/go-xdr#16 (reduction on XDR encoding/decoding allocations) stellar/go#4042

Merged

leighmcculloch merged commit 92a28e1 into stellar:master Nov 6, 2021

leighmcculloch deleted the perf-xdrgen branch November 6, 2021 00:53

leighmcculloch added a commit to stellar/go that referenced this pull request Nov 8, 2021

xdr: update using xdrgen#65 (#3957)

7a3ca3a

Update XDR using stellar/xdrgen#65 that improves the encoding cpu and memory usage.

This was referenced Nov 9, 2021

Create xdr.EncodingBuffer, which reduces buffer allocations stellar/go#4056

Merged

Generate DecodeFrom() methods for Golang #71

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the performance of Go generated marshaling/encoding code #65

Improve the performance of Go generated marshaling/encoding code #65

leighmcculloch commented Sep 22, 2021 •

edited

Loading

bartekn left a comment

bartekn Oct 5, 2021

leighmcculloch Oct 5, 2021

ire-and-curses commented Oct 5, 2021

bartekn commented Oct 5, 2021

leighmcculloch commented Oct 5, 2021

bartekn commented Oct 5, 2021

bartekn commented Oct 5, 2021

leighmcculloch commented Oct 5, 2021

leighmcculloch commented Oct 6, 2021 •

edited

Loading

bartekn commented Oct 18, 2021 •

edited

Loading

2opremio commented Oct 18, 2021 •

edited

Loading

leighmcculloch commented Oct 18, 2021

2opremio commented Oct 20, 2021

leighmcculloch commented Oct 20, 2021

2opremio commented Nov 8, 2021 •

edited

Loading

leighmcculloch commented Nov 8, 2021

2opremio commented Nov 8, 2021

Improve the performance of Go generated marshaling/encoding code #65

Improve the performance of Go generated marshaling/encoding code #65

Conversation

leighmcculloch commented Sep 22, 2021 • edited Loading

What

Example

Why

Known Limitations

bartekn left a comment

Choose a reason for hiding this comment

bartekn Oct 5, 2021

Choose a reason for hiding this comment

leighmcculloch Oct 5, 2021

Choose a reason for hiding this comment

ire-and-curses commented Oct 5, 2021

bartekn commented Oct 5, 2021

leighmcculloch commented Oct 5, 2021

bartekn commented Oct 5, 2021

bartekn commented Oct 5, 2021

leighmcculloch commented Oct 5, 2021

leighmcculloch commented Oct 6, 2021 • edited Loading

bartekn commented Oct 18, 2021 • edited Loading

2opremio commented Oct 18, 2021 • edited Loading

leighmcculloch commented Oct 18, 2021

2opremio commented Oct 20, 2021

leighmcculloch commented Oct 20, 2021

2opremio commented Nov 8, 2021 • edited Loading

leighmcculloch commented Nov 8, 2021

2opremio commented Nov 8, 2021

leighmcculloch commented Sep 22, 2021 •

edited

Loading

leighmcculloch commented Oct 6, 2021 •

edited

Loading

bartekn commented Oct 18, 2021 •

edited

Loading

2opremio commented Oct 18, 2021 •

edited

Loading

2opremio commented Nov 8, 2021 •

edited

Loading