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

xdr and horizon: Optimize compress-marshaling of ledger keys #4071

Merged
merged 4 commits into from
Nov 16, 2021

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Nov 13, 2021

15x CPU time improvement, all allocations were removed

Before:

goos: darwin
goarch: amd64
pkg: github.com/stellar/go/benchmarks
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
BenchmarkXDRMarshalCompress
BenchmarkXDRMarshalCompress-8   	  608031	      1970 ns/op	    2048 B/op	      59 allocs/op
PASS

After:

goos: darwin
goarch: amd64
pkg: github.com/stellar/go/benchmarks
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
BenchmarkXDRMarshalCompress
BenchmarkXDRMarshalCompress-8   	 8785383	       133.8 ns/op	       0 B/op	       0 allocs/op
PASS

One of the remaning allocations is, in fact, caused by a call to PoolID.Encode which I think can be removed.

xdr/ledger_key.go Outdated Show resolved Hide resolved
xdr/account_id.go Outdated Show resolved Hide resolved
xdr/asset.go Outdated Show resolved Hide resolved
@2opremio 2opremio requested review from tamirms and a team November 13, 2021 21:28
@2opremio
Copy link
Contributor Author

2opremio commented Nov 13, 2021

When writing this PR I started to wonder why don't we simply marshal the keys normally and hash them?

This would:

  1. Save a lot more space
  2. Save us from maintaining this compression hack

In other words, do we really need this sort of compression if the result is opaque?

xdr/account_id.go Outdated Show resolved Hide resolved
xdr/account_id.go Outdated Show resolved Hide resolved
xdr/asset.go Outdated Show resolved Hide resolved
xdr/ledger_key.go Show resolved Hide resolved
xdr/ledger_key.go Outdated Show resolved Hide resolved
xdr/account_id.go Outdated Show resolved Hide resolved
@2opremio 2opremio force-pushed the optimize-compress-marshaling branch 3 times, most recently from a4be372 to 0373aa9 Compare November 15, 2021 18:56
@2opremio
Copy link
Contributor Author

@bartekn PTAL

@2opremio 2opremio requested a review from a team November 15, 2021 22:00
xdr/trust_line_asset.go Outdated Show resolved Hide resolved
@2opremio 2opremio force-pushed the optimize-compress-marshaling branch from 0373aa9 to c517dad Compare November 16, 2021 00:08
m = append(m, account...)
dataName := []byte(strings.TrimRight(string(key.Data.DataName), "\x00"))
m = append(m, dataName...)
dataName := trimRightZeros([]byte(key.Data.DataName))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am surprised this casting to []byte from string didn't cause an allocation. The escape analysis is probably getting better in Go (it wasn't nearly as good as I would had liked to)

@2opremio 2opremio merged commit 2fcd985 into stellar:master Nov 16, 2021
@2opremio 2opremio deleted the optimize-compress-marshaling branch November 16, 2021 00:30
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.

2 participants