Skip to content

General performance improvements on data ingestion - #67

Draft
gsora wants to merge 19 commits into
mainfrom
gsora/perfimpro-ingest
Draft

General performance improvements on data ingestion#67
gsora wants to merge 19 commits into
mainfrom
gsora/perfimpro-ingest

Conversation

@gsora

@gsora gsora commented Mar 14, 2022

Copy link
Copy Markdown
Contributor

This PR attempts to optimize some rough corners on the ingestion side.

This is a deviation from @akhilkumarpilli proposed benchmarks, which were insanely helpful during the profiling and execution the code contained in this pull request.

A breakdown of what I did follows:

  • Got rid of the intermediate traceOperationInter struct
    That struct was used to obtain a """cuter""" TraceOperation struct, with BlockHeight and TxHash top-level fields. In the end, I figured we could just have a top-level Metadata struct which enhances type safety by specifying BlockHeight and TxHash types directly, without requiring implementing the json.UnmarshalJSON interface implementation.

  • Operation type is now a string, not a byte slice
    Having Operation as a byte slice was useful but it's incredibly resource-intensive when having it in a tight loop, because it requires converting it to string back and forth.

  • Usage of a data allocation pool
    Since TraceOperation's are used a lot during the ingestion process, using a sync.Pool helps with memory pressure by re-using structs already allocated previously. Each struct gets carefully wiped before usage. When data is selected to be parsed (e.g. the Operation matches what we want) we copy the TraceOperation content, otherwise it's discarded.

  • Zero-copy string to []byte conversion
    This is sorta controversial, I personally don't like this method at all but sometimes you gotta be flexible and bend to the Go runtime. The row that we read off the FIFO is potentially very big:tm:, and since each string to []byte conversion involves an allocation + copy we risk wasting precious time just doing that. This PR introduces the unsafeGetBytes() function which does a zero-copy, zero-allocation conversion to []byte, alleviating the tight loop penalty we were paying before. Keep in mind that in Go strings are immutable and that slices are passed by reference, so the output of this function effectively creates a mutable string. I ended up using this method anyway because the source string is discarded after use.
    I did not came up with this function, Ian Lance Taylor wrote it.

TL;DR: on my M1 Pro ARM machine, this branch produces the following numbers

gsora@smallBFG ~/D/T/t/t/t/benchmarks (gsora/perfimpro-ingest)> benchstat bench_stdlib_noperf bench_stdlib_perf
name                        old time/op    new time/op    delta
TracelistenerRealTraces-10     11.0s ± 2%      8.7s ± 6%  -21.20%  (p=0.000 n=8+10)

name                        old alloc/op   new alloc/op   delta
TracelistenerRealTraces-10    5.04GB ± 0%    3.17GB ± 4%  -37.09%  (p=0.000 n=8+10)

name                        old allocs/op  new allocs/op  delta
TracelistenerRealTraces-10     42.7M ± 0%     43.4M ± 5%     ~     (p=0.515 n=8+10)

So in a nutshell, it's about 21% quicker in reading and parsing traces while allocating less total memory, with a slight trade-off in terms of total allocations.

Source data for those results is here.

akhilkumarpilli and others added 9 commits March 3, 2022 14:21
`tracestats` reads cosmos-sdk traces written to a file by
the`--trace-store` command flag, and retrieves
 - block height
 - key length
 - value length
 - total trace length
off each and every trace line, then
creates a CSV file with this data.

Useful for data analysis purposes.
Make it work with the new no-middleman
type.

@DeshErBojhaa DeshErBojhaa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please clarify the race case comment!!

Comment thread cmd/tracestats/main.go Outdated
Comment thread tracelistener/benchmark_test.go Outdated
Comment thread tracelistener/tracelistener.go Outdated
Comment thread tracelistener/tracelistener.go
Comment thread tracelistener/tracelistener.go Outdated
@gsora
gsora requested a review from DeshErBojhaa March 14, 2022 13:02
Comment thread tracelistener/trace.go
Comment on lines +7 to 10
func (to *TraceOperation) Copy() TraceOperation {
ret := *to
return ret
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we consider a deep copy here.

I know we're assigning new slice in the Reset, but doing a shallow copy of a struct containing slice is risky in a sense later someone else can abuse the data integrity.

If we don't introduce deep copy, at least leave some comments on that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a deep copy: https://go.dev/play/p/zEQGXtnVsHa.
Since we're dealing with a simple struct, assigning a dereferenced pointer to a variable effectively copies the whole object, including slices.

Comment thread tracelistener/trace.go Outdated
Comment thread tracelistener/trace.go Outdated
Comment on lines +21 to +22
func (to TraceOperation) String() string {
return fmt.Sprintf(`[%s] "%v" -> "%v"`, to.Operation, string(to.Key), string(to.Value))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please consider to *TraceOperator as receiver. Not a fan of mixing value and pointer receiver when not strictly necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since nobody uses this method I was actually thinking of getting rid of it. WDYT?

Comment thread tracelistener/trace.go
Comment on lines +7 to 10
func (to *TraceOperation) Copy() TraceOperation {
ret := *to
return ret
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we consider a deep copy here.

I know we're assigning new slice in the Reset, but doing a shallow copy of a struct containing slice is risky in a sense later someone else can abuse the data integrity.

If we don't introduce deep copy, at least leave some comments on that.

Also added missing godoc strings, removed `TraceOperation.String()`
method, it wasn't needed anymore.
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