-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
changefeedccl: Improve avro encoder performance #63829
Conversation
@ajwerner I realized that after reverting an accidental merge, I never re-reverted this performance improvement PR. |
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.
Having never read this code before, I'm not having an easy time reviewing it without understanding the surrounding context. Can you clarify when the maps are being allocated now vs. before and explain how it's safe?
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @miretskiy, and @stevendanna)
pkg/ccl/changefeedccl/avro.go, line 105 at r2 (raw file):
typ *types.T native map[string]interface{}
native is not very descriptive to me, want to add some commentary here and in avroDataRecord
pkg/ccl/changefeedccl/avro_test.go, line 714 at r2 (raw file):
} // BenchmarkEncodeInt-16 2006964 666.0 ns/op 73 B/op 5 allocs/op
This is going to rot very fast. Rather than checking this into the code, I think it'd be better to have it in the commit messages.
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.
Ack. Added comments.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @stevendanna)
pkg/ccl/changefeedccl/avro.go, line 105 at r2 (raw file):
Previously, ajwerner wrote…
native is not very descriptive to me, want to add some commentary here and in
avroDataRecord
I agree. I used native
because that's what goavro uses. adding a comment here is helpful.
Let me know if that answers your question above re "maps"
pkg/ccl/changefeedccl/avro_test.go, line 714 at r2 (raw file):
Previously, ajwerner wrote…
This is going to rot very fast. Rather than checking this into the code, I think it'd be better to have it in the commit messages.
Okay.
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.
Reviewed 1 of 2 files at r3.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @miretskiy and @stevendanna)
pkg/ccl/changefeedccl/avro.go, line 139 at r4 (raw file):
colIdxByFieldIdx map[int]int fieldIdxByName map[string]int native map[string]interface{}
nit: comment to indicate that this field exists for reuse to avoid allocating a new map.
pkg/ccl/changefeedccl/avro.go, line 592 at r4 (raw file):
func (r *avroDataRecord) nativeFromRow(row rowenc.EncDatumRow) (interface{}, error) { if r.native == nil { r.native = make(map[string]interface{}, len(r.Fields))
note something like:
// Note that it's safe to reuse r.native without clearing it because all records will
// contain the same complete set of fields
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.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @stevendanna)
pkg/ccl/changefeedccl/avro.go, line 139 at r4 (raw file):
Previously, ajwerner wrote…
nit: comment to indicate that this field exists for reuse to avoid allocating a new map.
Done.
pkg/ccl/changefeedccl/avro.go, line 592 at r4 (raw file):
Previously, ajwerner wrote…
note something like:
// Note that it's safe to reuse r.native without clearing it because all records will // contain the same complete set of fields
Ooops -- I even forgot I did this one too.
Thanks. Comment added.
tftr |
bors r- |
Canceled. |
tftr |
Build failed (retrying...): |
Build failed (retrying...): |
Build failed: |
bors r+ |
Build succeeded: |
Avoid expensive allocations (maps) when encoding datums.
Improve encoder performance by ~40%, and significantly reduce
memory allocations per op.
Release Notes: None