-
Notifications
You must be signed in to change notification settings - Fork 25
chore: Batch writer benchmarks #1552
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
Conversation
⏱️ Benchmark results
|
} | ||
} | ||
|
||
func writerMatrix[T writers.Writer, C any, O ~func(T)](prefix string, constructor func(C, ...O) (T, error), client C, recordMaker func() func() arrow.Record, optsMatrix map[string][]O) []bCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have a problem...
writers/writers_test.go
Outdated
runtime.ReadMemStats(&mEnd) | ||
|
||
allocatedBytes := mEnd.Alloc - mStart.Alloc | ||
b.ReportMetric(float64(allocatedBytes)/float64(newN), "bytes/op") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't Go measure memory usage already if we provide -benchmem
? What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
benchmem
will report allocs/op
(probably the same number as ours but I didn't compare them) and won't report until invoked with -benchmem
or toggled from within the test. We also multiply the N
to get more stable benchmarks (and hit writer batching spots).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit complicated logic but since it's a benchmark it's probably okay as long as it helps you find memory-related improvements 😅
Helps with memory. Benched with #1552, old: ``` BenchmarkWriterMemory/MixedBatchWriter_batch10k_bytes100M BenchmarkWriterMemory/MixedBatchWriter_batch10k_bytes100M-10 826 1445595 ns/op 1393 bytes/op BenchmarkWriterMemory/MixedBatchWriter_defaults BenchmarkWriterMemory/MixedBatchWriter_defaults-10 670 1504799 ns/op 1393 bytes/op BenchmarkWriterMemory/MixedBatchWriter_wide_batch10k_bytes100M BenchmarkWriterMemory/MixedBatchWriter_wide_batch10k_bytes100M-10 8 128966609 ns/op 172785 bytes/op BenchmarkWriterMemory/MixedBatchWriter_wide_defaults BenchmarkWriterMemory/MixedBatchWriter_wide_defaults-10 9 118630315 ns/op 60355 bytes/op ``` new: ``` BenchmarkWriterMemory/MixedBatchWriter_batch10k_bytes100M BenchmarkWriterMemory/MixedBatchWriter_batch10k_bytes100M-10 690 1476479 ns/op 1401 bytes/op BenchmarkWriterMemory/MixedBatchWriter_defaults BenchmarkWriterMemory/MixedBatchWriter_defaults-10 687 1539707 ns/op 1401 bytes/op BenchmarkWriterMemory/MixedBatchWriter_wide_batch10k_bytes100M BenchmarkWriterMemory/MixedBatchWriter_wide_batch10k_bytes100M-10 8 129154755 ns/op 173592 bytes/op BenchmarkWriterMemory/MixedBatchWriter_wide_defaults BenchmarkWriterMemory/MixedBatchWriter_wide_defaults-10 9 111416773 ns/op 44588 bytes/op ```
for i := 0; i < numWideCols; i++ { | ||
bldr.Field(i + 1).(*array.Int64Builder).Append(randVals[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for i := 0; i < numWideCols; i++ { | |
bldr.Field(i + 1).(*array.Int64Builder).Append(randVals[i]) | |
for i := 1; i <= numWideCols; i++ { | |
bldr.Field(i).(*array.Int64Builder).Append(randVals[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'd need to do randVals[i-1]
, adds compexity and potential copy/paste errors, just to avoid an + 1
https://docs.google.com/spreadsheets/d/1NxKyOtvckW7yCbwLTL4VOu91XPNyd-Q0dYDMxjT-yH8/edit?usp=sharing
Current state (
1000 * N
)