kv: add simple bulk put routines for vectorized insert#96237
kv: add simple bulk put routines for vectorized insert#96237craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
27f55d3 to
540f632
Compare
540f632 to
b9b5d2a
Compare
|
@nvanbenschoten i think this PR is down your alley |
nvb
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @msirek)
pkg/kv/batch.go line 430 at r1 (raw file):
} // PutBytes allows multiple Bytes value type puts to be added to the batch.
nit: s/Bytes/[]byte/g
pkg/kv/batch.go line 430 at r1 (raw file):
} // PutBytes allows multiple Bytes value type puts to be added to the batch.
Can we add testing for these new methods?
pkg/kv/batch.go line 435 at r1 (raw file):
b.bulkRequest(keys, values, func(i int, k roachpb.Key, v []byte) (roachpb.Request, int) { pr := &reqs[i] pr.RequestHeader.Key = k
nit: Isn't RequestHeader embedded?
pkg/kv/batch.go line 436 at r1 (raw file):
pr := &reqs[i] pr.RequestHeader.Key = k pr.Value.SetBytes(v)
Should we be setting Value checksums here? Like we do in roachpb.NewPut.
pkg/kv/batch.go line 441 at r1 (raw file):
} // InitPutBytes allows multiple Bytes value type puts to be added to the batch.
nit: could you move the two batch InitPut methods below (*Batch).InitPut?
pkg/kv/batch.go line 577 at r1 (raw file):
b.Results = make([]Result, 0, len(keys)) } count := 0
Why can't we use bulkRequests in this case? Is the contract too rigid? Can we fix that?
pkg/kv/batch.go line 996 at r1 (raw file):
} func (b *Batch) bulkRequest(
Should we assert that len(keys) == len(values)?
pkg/kv/batch.go line 999 at r1 (raw file):
keys []roachpb.Key, values [][]byte, valFactory func(int, roachpb.Key, []byte) (roachpb.Request, int),
reqFactory?
pkg/kv/batch.go line 999 at r1 (raw file):
keys []roachpb.Key, values [][]byte, valFactory func(int, roachpb.Key, []byte) (roachpb.Request, int),
Let's give the int return value a name so that the contract is clear.
Along the same lines, let's name the params. The role of the int param isn't obvious.
pkg/kv/batch.go line 1007 at r1 (raw file):
count := 0 for i, key := range keys { // Ignore nil keys
This necessitates the count tracking, separate from i. Is it needed? Why would we ever pass in a nil key?
pkg/kv/batch.go line 1014 at r1 (raw file):
reqs[count], numBytes = valFactory(count, key, values[i]) b.approxMutationReqBytes += len(key) + numBytes b.initResult(1, 1, notRaw, nil)
Should we be calling this once per batch of Puts? Like we do in (*Batch).Del?
b9b5d2a to
00d96f7
Compare
cucaroach
left a comment
There was a problem hiding this comment.
Thanks for all the helpful review comments! I think I addressed everything, let me know if you think we more tests.
Reviewable status:
pkg/kv/batch.go line 430 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Can we add testing for these new methods?
Yes! Looks like db_test.go would be a good place for them.
pkg/kv/batch.go line 435 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: Isn't
RequestHeaderembedded?
I'm not sure what this means. Oh you're saying I can just leave off the RequestHeader yes?
pkg/kv/batch.go line 436 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Should we be setting Value checksums here? Like we do in
roachpb.NewPut.
I assume we should, is there something I can use to test that the checksums are set and correct?
pkg/kv/batch.go line 577 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Why can't we use
bulkRequestsin this case? Is the contract too rigid? Can we fix that?
Good idea, I gave up to easily.
pkg/kv/batch.go line 996 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Should we assert that
len(keys) == len(values)?
We could, I'm still unclear if we
pkg/kv/batch.go line 1007 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
This necessitates the
counttracking, separate fromi. Is it needed? Why would we ever pass in a nil key?
So basically we re-use slices across each index and due to partial indexes there are "holes" where we elide KVs that fail the partial index predicate. Basically nil keys is my way of avoiding doing this separate count tracking and compaction at the encoder level and letting it happen here at the KV API boundary where I think its cheaper because we're reformulating everything anyways. Its a little sleazy but I think its kinda the "vector" way, akin to how coldata.Batches utilize selection vectors.
pkg/kv/batch.go line 1014 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Should we be calling this once per batch of Puts? Like we do in
(*Batch).Del?
I don't think so, at least not without changing all the result processing code since, Del doesn't seem to care about the result of each individual delete so it has one result with N calls? Maybe I'm missing something but I think I had to do it this way to get a result per request.
cucaroach
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/batch.go line 996 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
We could, I'm still unclear if we
Finishing thought ... I'm unsure where its a good idea to add assertion checks for things that should never happen when Go will generate good enough error messages to clue in future developers who introduce breakages. Sometimes I see checks like this in crdb_test blocks. Curious what your thoughts are.
00d96f7 to
ffca888
Compare
ffca888 to
ef7d6fc
Compare
cucaroach
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/batch.go line 1014 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
I don't think so, at least not without changing all the result processing code since, Del doesn't seem to care about the result of each individual delete so it has one result with N calls? Maybe I'm missing something but I think I had to do it this way to get a result per request.
I do think there's room for improvement here for multi back to back bulk puts which will be common. I need to understand the result code a little better.
msirek
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/batch.go line 440 at r3 (raw file):
pr.Value.InitChecksum(k) pr.Value.SetBytes(values[keyIdx]) return pr, len(pr.Value.RawBytes)
It looks like all of the functions passed to bulkRequest share the same code besides the next to last line, so there are only 2 versions. Should these be refactored out into defined functions to pass as parameters? Or, if only the SetBytes or SetTuple line is expected to change, two layers of function calls could be defined. One with the main template, and the a function call parameter that calls either SetBytes or SetTuple.
pkg/kv/batch.go line 1017 at r3 (raw file):
if reqIdx > 0 { reqs = reqs[:reqIdx] b.appendReqs(reqs...)
All other calls to appendReqs are immediately followed by a call to initResult. I'm not sure why this is, but I'm wondering if there's a reason for it that applies here too. It looks like the 2 functions don't touch the same data structures, so I guess it's ok to have the order of the calls swapped. Just pointing out the difference in case you want to take a closer look.
nvb
left a comment
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
pkg/kv/batch.go line 435 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
I'm not sure what this means. Oh you're saying I can just leave off the RequestHeader yes?
Right. Looks like you made the change.
pkg/kv/batch.go line 436 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
I assume we should, is there something I can use to test that the checksums are set and correct?
These calls to InitChecksum need to come after the calls to SetBytes/SetTuple.
To test, you could verify that Value.Verify(key) does not return an error.
pkg/kv/batch.go line 996 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
Finishing thought ... I'm unsure where its a good idea to add assertion checks for things that should never happen when Go will generate good enough error messages to clue in future developers who introduce breakages. Sometimes I see checks like this in crdb_test blocks. Curious what your thoughts are.
I think in cases like these where we'd like to place constraints on a function's inputs, doing so explicitly with a helpful message is good. It makes it easier to debug in cases where the function is used incorrectly. It also ensures that bugs don't slip through the cracks. For instance, in these cases, len(keys) > len(values) would have caused a panic, but len(keys) < len(values) would have silently succeeded. If a caller is passing in len(keys) < len(values), we're probably doing them a disservice by silently letting that succeed.
pkg/kv/batch.go line 1007 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
So basically we re-use slices across each index and due to partial indexes there are "holes" where we elide KVs that fail the partial index predicate. Basically nil keys is my way of avoiding doing this separate count tracking and compaction at the encoder level and letting it happen here at the KV API boundary where I think its cheaper because we're reformulating everything anyways. Its a little sleazy but I think its kinda the "vector" way, akin to how coldata.Batches utilize selection vectors.
This doesn't feel like the kind of complexity that should be making its way into the KV API, especially if it makes code at the API boundary harder to read, or if it makes the contract of the API more subtle. Given how many times we iterate over the requests in a batch below this level, I'd be pretty surprised if this provided a measurable performance benefit vs. doing the compaction at the encoder level. Also, there's a performance cost to this, which is that we're now sizing b.Results larger than it needs to be in cases where compression is possible. So I'd suggest removing it unless you can demonstrate a performance improvement from the optimization.
7c27c57 to
a7137cb
Compare
cucaroach
left a comment
There was a problem hiding this comment.
Ready for another look, thanks!
Reviewable status:
pkg/kv/batch.go line 996 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I think in cases like these where we'd like to place constraints on a function's inputs, doing so explicitly with a helpful message is good. It makes it easier to debug in cases where the function is used incorrectly. It also ensures that bugs don't slip through the cracks. For instance, in these cases,
len(keys) > len(values)would have caused a panic, butlen(keys) < len(values)would have silently succeeded. If a caller is passing inlen(keys) < len(values), we're probably doing them a disservice by silently letting that succeed.
👍
pkg/kv/batch.go line 1007 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
This doesn't feel like the kind of complexity that should be making its way into the KV API, especially if it makes code at the API boundary harder to read, or if it makes the contract of the API more subtle. Given how many times we iterate over the requests in a batch below this level, I'd be pretty surprised if this provided a measurable performance benefit vs. doing the compaction at the encoder level. Also, there's a performance cost to this, which is that we're now sizing
b.Resultslarger than it needs to be in cases where compression is possible. So I'd suggest removing it unless you can demonstrate a performance improvement from the optimization.
Sounds good, partial indexes are more the exception than the rule I think so no problem to do the compaction for the exceptional case.
pkg/kv/batch.go line 1014 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
I do think there's room for improvement here for multi back to back bulk puts which will be common. I need to understand the result code a little better.
Okay I think I figured out the right way to do this.
pkg/kv/batch.go line 1017 at r3 (raw file):
Previously, msirek (Mark Sirek) wrote…
All other calls to
appendReqsare immediately followed by a call toinitResult. I'm not sure why this is, but I'm wondering if there's a reason for it that applies here too. It looks like the 2 functions don't touch the same data structures, so I guess it's ok to have the order of the calls swapped. Just pointing out the difference in case you want to take a closer look.
Thanks, the request->result mapping is a little funny because of the dynamic number of calls/rows per request but I'm pretty sure the order doesn't matter.
a7137cb to
f2ca4b2
Compare
024d849 to
d72d962
Compare
cucaroach
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/batch.go line 1007 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
Okay so I'd like to revisit this. I don't just have empty keys for partial indexes, they also come up with column families where if the value ends up being empty we drop that KV pair. However I reuse the key slices across families, the part of the key up to and including the family ID is saved across families and we just overwrite the rest of the key. If I have to compress the slices I can't do that, so I'd end up having to reallocate the slices of keys which would end up generating a bunch of garbage. Admittedly these extra allocations would only end up hurting those cases but extra memory allocations are one of those hidden costs that end up affecting everything and I'd like to avoid it. What if I allowed empty keys and fixed the extra sizing so we only end up allocating results for the number of real keys? In terms of the batch encoder logic allowing empty keys means much simpler code there for a pretty small complexity hit at the bulk API layer. But the real win for me is the reduction in throw away allocations.
Actually I think we can have a simple clean API AND avoid any copying/throw away allocations if we go with a iterator/generator model instead of mega slices, might come in handy for other things, let me give that a whirl.
07627c6 to
673572a
Compare
|
@nvanbenschoten this is ready for another look! Thanks! |
nvb
left a comment
There was a problem hiding this comment.
@cucaroach do we have benchmark results that demonstrate the speedup provided by this change?
Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status:
pkg/kv/batch.go line 91 at r8 (raw file):
// BulkSourceIterator is the iterator interface for bulk put operations. type BulkSourceIterator[T GValue] interface { Next() (roachpb.Key, T)
What is the contract of this method for when the iterator is exhausted?
pkg/kv/batch.go line 572 at r8 (raw file):
func (b *Batch) CPutTuples(bs BulkSource[[]byte]) { numKeys := bs.Len() reqs := make([]kvpb.ConditionalPutRequest, numKeys)
Conditional put requests require an expected value. We don't seem to be setting one for any request in CPutTuples or CPutValues.
pkg/kv/batch.go line 1051 at r8 (raw file):
req, numBytes := requestFactory() b.approxMutationReqBytes += numBytes newReqs[i].MustSetInner(req)
MustSetInner heap allocates a ResponseUnion_... struct. It's unfortunate that we can't batch allocate all of these structs for the bulk request, but I don't see a good way to support this generically over the request types.
pkg/kv/batch.go line 1062 at r8 (raw file):
if idx < r.calls { if idx < len(r.Rows) { return &r, r.Rows[idx], nil
This will cause r (the loop iteration variable) to escape to the heap. I think we want the following instead:
for i := range b.Results {
r := &b.Results[i]
...
return r, ...
}
cucaroach
left a comment
There was a problem hiding this comment.
No but I whipped one up, how's this:
❯❯❯ go test ./pkg/kv -benchmem -run - -bench BenchmarkBulk
goos: darwin
goarch: arm64
pkg: github.com/cockroachdb/cockroach/pkg/kv
BenchmarkBulkBatchAPI/single-10 3561 358131 ns/op 824200 B/op 5069 allocs/op
BenchmarkBulkBatchAPI/bulk-10 10000 174356 ns/op 797713 B/op 2008 allocs/op
PASS
ok github.com/cockroachdb/cockroach/pkg/kv 4.929s
Reviewable status:
pkg/kv/batch.go line 440 at r3 (raw file):
Previously, msirek (Mark Sirek) wrote…
It looks like all of the functions passed to
bulkRequestshare the same code besides the next to last line, so there are only 2 versions. Should these be refactored out into defined functions to pass as parameters? Or, if only theSetBytesorSetTupleline is expected to change, two layers of function calls could be defined. One with the main template, and the a function call parameter that calls eitherSetBytesorSetTuple.
They refer to a variable in the surrounding function so I don't think so but I'm open to ideas...
pkg/kv/batch.go line 91 at r8 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
What is the contract of this method for when the iterator is exhausted?
I would say the contract is calling Next more than Len() times is undefined, does that pass muster?
pkg/kv/batch.go line 572 at r8 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Conditional put requests require an expected value. We don't seem to be setting one for any request in CPutTuples or CPutValues.
I was just copying inserter.go which uses insertCPutFn and always passes nil for expValue (well for PK it does, for secondary index it switches to InitPut). Presumably theres a good reason for this and we're going for this behavior: "To conditionally set a value only if the key doesn't currently exist, pass an empty expValue." Maybe I should append "IfEmpty" to CPutValues and CPutTuples? I admit to being confused by the CPut API's menu of expValue, allowNotExist and inline flavors.
pkg/kv/batch.go line 1051 at r8 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
MustSetInnerheap allocates aResponseUnion_...struct. It's unfortunate that we can't batch allocate all of these structs for the bulk request, but I don't see a good way to support this generically over the request types.
Maybe we need first class "bulk" operations so they all live under one Request object but not sure about that.
pkg/kv/batch.go line 1062 at r8 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
This will cause
r(the loop iteration variable) to escape to the heap. I think we want the following instead:for i := range b.Results { r := &b.Results[i] ... return r, ... }
So the way I have it we heap allocate a kv.Result and copy b.Results[i] into it (00078) and the way you have it a copy is allocated with runtime.convTslice (00063). If I'm reading it right there's an allocation either way. Very annoying, maybe I should just return i and if the caller wants the Result object they can get themselves?
Your way:
# /Users/treilly/vec-encoding/pkg/kv/batch.go
00000 (1058) TEXT github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult(SB), ABIInternal
00001 (1058) FUNCDATA $0, gclocals·bb+LSCCik3x40Cn8eFqL9w==(SB)
00002 (1058) FUNCDATA $1, gclocals·KJo/W9EYQ0eq9CmHhpGqcA==(SB)
00003 (1058) FUNCDATA $2, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.stkobj(SB)
00004 (1058) FUNCDATA $5, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.arginfo1(SB)
00005 (1058) FUNCDATA $6, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.argliveinfo(SB)
b1
00006 (1058) PCDATA $3, $1
v107
00007 (+1060) MOVD 8(R0), R7
v98
00008 (1060) MOVD 16(R0), R8
v139
00009 (1058) MOVD R1, R2
v136
00010 (1058) MOVD $0, R3
b1
00011 (1060) JMP 14
v79
00012 (+1060) ADD $1, R3, R3
v76
00013 (+1069) SUB R10, R1, R1
v134
00014 (+1060) CMP R8, R3
b2
00015 (1060) BGE 45
v106
00016 (+1061) SUB R3<<2, R3, R9
v35
00017 (1061) SUB R9<<5, R7, R9
v40
00018 (+1062) MOVD (R9), R10
v137
00019 (1062) CMP R10, R1
b6
00020 (1062) BGE 12
v73
00021 (+1063) MOVD 24(R9), R7
v38
00022 (1063) MOVD 32(R9), R8
v112
00023 (1063) CMP R8, R1
b9
00024 (1063) BGE 37
b11
00025 (+1064) BHS 84
v28
00026 (+1064) LSL $5, R1, R8
v53
00027 (1064) MOVD (R7)(R8), R8
v61
00028 (1064) ADD R1<<5, R7, R7
v99
00029 (1064) MOVD 8(R7), R2
v74
00030 (1064) MOVD 16(R7), R3
v129
00031 (1064) MOVD 24(R7), R4
v116
00032 (1064) MOVD R9, R0
v124
00033 (1064) MOVD R8, R1
v58
00034 (1064) MOVD $0, R5
v115
00035 (1064) MOVD $0, R6
b13
00036 (1064) RET
v29
00037 (+1066) MOVD R9, R0
v108
00038 (1066) MOVD $0, R1
v95
00039 (1066) MOVD $0, R2
v83
00040 (1066) MOVD R2, R3
v121
00041 (1066) MOVD $0, R4
v122
00042 (1066) MOVD $0, R5
v30
00043 (1066) MOVD R1, R6
b12
00044 (1066) RET
v133
00045 (1058) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv.b(RSP)
v133
00046 (1058) PCDATA $3, $2
v109
00047 (+1071) STP (ZR, ZR), github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-32(RSP)
v84
00048 (1071) STP (ZR, ZR), github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-16(RSP)
v75
00049 (1071) MOVD R2, R0
v88
00050 (1071) PCDATA $1, $1
v88
00051 (1071) CALL runtime.convT64(SB)
v128
00052 (1071) MOVD $type.int(SB), R1
v42
00053 (1071) MOVD R1, github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-32(RSP)
v48
00054 (1071) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-24(RSP)
v141
00055 (1071) MOVD github.com/cockroachdb/cockroach/pkg/kv.b(RSP), R1
v126
00056 (1071) MOVD 8(R1), R0
v123
00057 (1071) MOVD 16(R1), R2
v114
00058 (1071) MOVD 24(R1), R1
v57
00059 (1071) MOVD R1, R3
v27
00060 (1071) MOVD R2, R1
v49
00061 (1071) MOVD R3, R2
v102
00062 (1071) PCDATA $1, $2
v102
00063 (1071) CALL runtime.convTslice(SB)
v45
00064 (1071) MOVD $type.[]github.com/cockroachdb/cockroach/pkg/kv.Result(SB), R1
v140
00065 (1071) MOVD R1, github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-16(RSP)
v43
00066 (1071) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-8(RSP)
v118
00067 (?) NOP
# /Users/treilly/go/pkg/mod/github.com/cockroachdb/errors@v1.9.1/errutil_api.go
v51
00068 (+148) MOVD $1, R0
v41
00069 (148) MOVD $go.string."index %d outside of results: %+v"(SB), R1
v39
00070 (148) MOVD $32, R2
v23
00071 (148) MOVD $github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-32(RSP), R3
v33
00072 (148) MOVD $2, R4
v93
00073 (148) MOVD R4, R5
v120
00074 (+148) PCDATA $1, $3
v120
00075 (+148) CALL github.com/cockroachdb/errors/errutil.AssertionFailedWithDepthf(SB)
# /Users/treilly/vec-encoding/pkg/kv/batch.go
v127
00076 (+1071) MOVD $0, R2
v50
00077 (1071) MOVD R2, R3
v18
00078 (1071) MOVD $0, R4
v100
00079 (1071) MOVD R0, R5
v96
00080 (1071) MOVD R1, R6
v92
00081 (1071) MOVD $0, R0
v69
00082 (1071) MOVD $0, R1
b17
00083 (1071) RET
00084 (1071) PCDATA $1, $-1
00085 (1071) PCDATA $3, $1
v66
00086 (1064) MOVD R1, R0
v55
00087 (1064) MOVD R8, R1
v59
00088 (1064) PCDATA $1, $3
v59
00089 (1064) CALL runtime.panicIndex(SB)
00090 (1064) HINT $0
00091 (?) END
My way:
# /Users/treilly/vec-encoding/pkg/kv/batch.go
00000 (1058) TEXT github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult(SB), ABIInternal
00001 (1058) FUNCDATA $0, gclocals·bb+LSCCik3x40Cn8eFqL9w==(SB)
00002 (1058) FUNCDATA $1, gclocals·s4fpErQgTyW87J+tkvIERg==(SB)
00003 (1058) FUNCDATA $2, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.stkobj(SB)
00004 (1058) FUNCDATA $5, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.arginfo1(SB)
00005 (1058) FUNCDATA $6, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.argliveinfo(SB)
b1
00006 (1058) PCDATA $3, $1
v80
00007 (1058) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv.b(RSP)
v140
00008 (1058) MOVD R1, github.com/cockroachdb/cockroach/pkg/kv.idx+8(RSP)
v140
00009 (1058) PCDATA $3, $-1
v58
00010 (+1060) MOVD $type.github.com/cockroachdb/cockroach/pkg/kv.Result(SB), R0
v17
00011 (1060) PCDATA $1, $0
v17
00012 (1060) CALL runtime.newobject(SB)
v33
00013 (1060) MOVD github.com/cockroachdb/cockroach/pkg/kv.b(RSP), R1
v112
00014 (1060) MOVD 8(R1), R2
v109
00015 (1060) MOVD 16(R1), R3
b1
00016 (1060) CBZ R3, 23
v21
00017 (1060) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv.&r-40(RSP)
v95
00018 (1060) MOVD R3, github.com/cockroachdb/cockroach/pkg/kv..autotmp_29-56(RSP)
v60
00019 (1060) MOVD github.com/cockroachdb/cockroach/pkg/kv.idx+8(RSP), R7
v108
00020 (1058) MOVD R7, R4
v137
00021 (1058) MOVD $0, R5
b3
00022 (1060) JMP 62
v26
00023 (+1070) STP (ZR, ZR), github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-32(RSP)
v86
00024 (1070) STP (ZR, ZR), github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-16(RSP)
v110
00025 (1070) MOVD github.com/cockroachdb/cockroach/pkg/kv.idx+8(RSP), R0
v90
00026 (1070) PCDATA $1, $1
v90
00027 (1070) CALL runtime.convT64(SB)
v97
00028 (1070) MOVD $type.int(SB), R1
v27
00029 (1070) MOVD R1, github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-32(RSP)
v119
00030 (1070) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-24(RSP)
v85
00031 (1070) MOVD github.com/cockroachdb/cockroach/pkg/kv.b(RSP), R1
v138
00032 (1070) MOVD 8(R1), R0
v143
00033 (1070) MOVD 16(R1), R2
v133
00034 (1070) MOVD 24(R1), R1
v136
00035 (1070) MOVD R1, R3
v24
00036 (1070) MOVD R2, R1
v64
00037 (1070) MOVD R3, R2
v104
00038 (1070) PCDATA $1, $2
v104
00039 (1070) CALL runtime.convTslice(SB)
v117
00040 (1070) MOVD $type.[]github.com/cockroachdb/cockroach/pkg/kv.Result(SB), R1
v44
00041 (1070) MOVD R1, github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-16(RSP)
v79
00042 (1070) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-8(RSP)
v120
00043 (?) NOP
# /Users/treilly/go/pkg/mod/github.com/cockroachdb/errors@v1.9.1/errutil_api.go
v130
00044 (+148) MOVD $1, R0
v134
00045 (148) MOVD $go.string."index %d outside of results: %+v"(SB), R1
v141
00046 (148) MOVD $32, R2
v52
00047 (148) MOVD $github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-32(RSP), R3
v69
00048 (148) MOVD $2, R4
v45
00049 (148) MOVD R4, R5
v122
00050 (+148) PCDATA $1, $3
v122
00051 (+148) CALL github.com/cockroachdb/errors/errutil.AssertionFailedWithDepthf(SB)
# /Users/treilly/vec-encoding/pkg/kv/batch.go
v40
00052 (+1070) MOVD $0, R2
v98
00053 (1070) MOVD R2, R3
v75
00054 (1070) MOVD $0, R4
v102
00055 (1070) MOVD R0, R5
v126
00056 (1070) MOVD R1, R6
v36
00057 (1070) MOVD $0, R0
v101
00058 (1070) MOVD $0, R1
b17
00059 (1070) RET
v81
00060 (1060) ADD $96, R2, R2
v135
00061 (1060) MOVD R10, R5
v50
00062 (1060) PCDATA $0, $-2
v50
00063 (+1060) MOVD $runtime.writeBarrier(SB), R8
v78
00064 (1060) MOVWU (R8), R9
b5
00065 (1060) CBNZW R9, 74
v43
00066 (+1060) ADD $88, R2, R9
v99
00067 (1060) MOVD R0, R17
v100
00068 (1060) MOVD R2, R16
v111
00069 (1060) MOVD.P 8(R16), R27
v111
00070 (1060) MOVD.P R27, 8(R17)
v111
00071 (1060) CMP R9, R16
v111
00072 (1060) BLE 69
b16
00073 (1060) JMP 88
v32
00074 (1060) MOVD R5, github.com/cockroachdb/cockroach/pkg/kv..autotmp_30-64(RSP)
v51
00075 (1060) MOVD R2, github.com/cockroachdb/cockroach/pkg/kv..autotmp_31-48(RSP)
v42
00076 (1061) MOVD R7, github.com/cockroachdb/cockroach/pkg/kv..autotmp_32-72(RSP)
v28
00077 (+1060) MOVD R0, R1
v129
00078 (1060) MOVD $type.github.com/cockroachdb/cockroach/pkg/kv.Result(SB), R0
v123
00079 (+1060) CALL runtime.typedmemmove(SB)
v132
00080 (1061) MOVD github.com/cockroachdb/cockroach/pkg/kv.&r-40(RSP), R0
v68
00081 (1070) MOVD github.com/cockroachdb/cockroach/pkg/kv.b(RSP), R1
v66
00082 (1060) MOVD github.com/cockroachdb/cockroach/pkg/kv..autotmp_31-48(RSP), R2
v65
00083 (1060) MOVD github.com/cockroachdb/cockroach/pkg/kv..autotmp_29-56(RSP), R3
v74
00084 (1070) MOVD github.com/cockroachdb/cockroach/pkg/kv.idx+8(RSP), R4
v71
00085 (1060) MOVD github.com/cockroachdb/cockroach/pkg/kv..autotmp_30-64(RSP), R5
v77
00086 (1061) MOVD github.com/cockroachdb/cockroach/pkg/kv..autotmp_32-72(RSP), R7
v76
00087 (1060) MOVD $runtime.writeBarrier(SB), R8
v37
00088 (+1061) PCDATA $0, $-1
v37
00089 (+1061) MOVD (R0), R9
v55
00090 (1061) CMP R9, R7
b15
00091 (1061) BLT 97
v73
00092 (+1060) ADD $1, R5, R10
v70
00093 (+1068) SUB R9, R7, R7
v67
00094 (1060) CMP R10, R3
b6
00095 (1060) BGT 60
b6
00096 (1060) JMP 23
v49
00097 (+1062) MOVD 24(R0), R8
v47
00098 (1062) MOVD 32(R0), R1
v94
00099 (1062) CMP R1, R7
b9
00100 (1062) BGE 112
b11
00101 (+1063) PCDATA $1, $-1
b11
00102 (+1063) BHS 119
v144
00103 (+1063) LSL $5, R7, R9
v128
00104 (1063) MOVD (R8)(R9), R1
v56
00105 (1063) ADD R7<<5, R8, R7
v125
00106 (1063) MOVD 8(R7), R2
v116
00107 (1063) MOVD 16(R7), R3
v131
00108 (1063) MOVD 24(R7), R4
v22
00109 (1063) MOVD $0, R5
v82
00110 (1063) MOVD $0, R6
b13
00111 (1063) RET
v62
00112 (+1065) MOVD $0, R1
v15
00113 (1065) MOVD $0, R2
v13
00114 (1065) MOVD R2, R3
v10
00115 (1065) MOVD $0, R4
v39
00116 (1065) MOVD $0, R5
v142
00117 (1065) MOVD R1, R6
b12
00118 (1065) RET
v139
00119 (1063) MOVD R7, R0
v54
00120 (1063) PCDATA $1, $3
v54
00121 (1063) CALL runtime.panicIndex(SB)
00122 (1063) HINT $0
00123 (?) END
cucaroach
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/batch.go line 1062 at r8 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
So the way I have it we heap allocate a kv.Result and copy b.Results[i] into it (00078) and the way you have it a copy is allocated with runtime.convTslice (00063). If I'm reading it right there's an allocation either way. Very annoying, maybe I should just return i and if the caller wants the Result object they can get themselves?
Your way:
# /Users/treilly/vec-encoding/pkg/kv/batch.go 00000 (1058) TEXT github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult(SB), ABIInternal 00001 (1058) FUNCDATA $0, gclocals·bb+LSCCik3x40Cn8eFqL9w==(SB) 00002 (1058) FUNCDATA $1, gclocals·KJo/W9EYQ0eq9CmHhpGqcA==(SB) 00003 (1058) FUNCDATA $2, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.stkobj(SB) 00004 (1058) FUNCDATA $5, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.arginfo1(SB) 00005 (1058) FUNCDATA $6, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.argliveinfo(SB) b1 00006 (1058) PCDATA $3, $1 v107 00007 (+1060) MOVD 8(R0), R7 v98 00008 (1060) MOVD 16(R0), R8 v139 00009 (1058) MOVD R1, R2 v136 00010 (1058) MOVD $0, R3 b1 00011 (1060) JMP 14 v79 00012 (+1060) ADD $1, R3, R3 v76 00013 (+1069) SUB R10, R1, R1 v134 00014 (+1060) CMP R8, R3 b2 00015 (1060) BGE 45 v106 00016 (+1061) SUB R3<<2, R3, R9 v35 00017 (1061) SUB R9<<5, R7, R9 v40 00018 (+1062) MOVD (R9), R10 v137 00019 (1062) CMP R10, R1 b6 00020 (1062) BGE 12 v73 00021 (+1063) MOVD 24(R9), R7 v38 00022 (1063) MOVD 32(R9), R8 v112 00023 (1063) CMP R8, R1 b9 00024 (1063) BGE 37 b11 00025 (+1064) BHS 84 v28 00026 (+1064) LSL $5, R1, R8 v53 00027 (1064) MOVD (R7)(R8), R8 v61 00028 (1064) ADD R1<<5, R7, R7 v99 00029 (1064) MOVD 8(R7), R2 v74 00030 (1064) MOVD 16(R7), R3 v129 00031 (1064) MOVD 24(R7), R4 v116 00032 (1064) MOVD R9, R0 v124 00033 (1064) MOVD R8, R1 v58 00034 (1064) MOVD $0, R5 v115 00035 (1064) MOVD $0, R6 b13 00036 (1064) RET v29 00037 (+1066) MOVD R9, R0 v108 00038 (1066) MOVD $0, R1 v95 00039 (1066) MOVD $0, R2 v83 00040 (1066) MOVD R2, R3 v121 00041 (1066) MOVD $0, R4 v122 00042 (1066) MOVD $0, R5 v30 00043 (1066) MOVD R1, R6 b12 00044 (1066) RET v133 00045 (1058) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv.b(RSP) v133 00046 (1058) PCDATA $3, $2 v109 00047 (+1071) STP (ZR, ZR), github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-32(RSP) v84 00048 (1071) STP (ZR, ZR), github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-16(RSP) v75 00049 (1071) MOVD R2, R0 v88 00050 (1071) PCDATA $1, $1 v88 00051 (1071) CALL runtime.convT64(SB) v128 00052 (1071) MOVD $type.int(SB), R1 v42 00053 (1071) MOVD R1, github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-32(RSP) v48 00054 (1071) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-24(RSP) v141 00055 (1071) MOVD github.com/cockroachdb/cockroach/pkg/kv.b(RSP), R1 v126 00056 (1071) MOVD 8(R1), R0 v123 00057 (1071) MOVD 16(R1), R2 v114 00058 (1071) MOVD 24(R1), R1 v57 00059 (1071) MOVD R1, R3 v27 00060 (1071) MOVD R2, R1 v49 00061 (1071) MOVD R3, R2 v102 00062 (1071) PCDATA $1, $2 v102 00063 (1071) CALL runtime.convTslice(SB) v45 00064 (1071) MOVD $type.[]github.com/cockroachdb/cockroach/pkg/kv.Result(SB), R1 v140 00065 (1071) MOVD R1, github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-16(RSP) v43 00066 (1071) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-8(RSP) v118 00067 (?) NOP # /Users/treilly/go/pkg/mod/github.com/cockroachdb/errors@v1.9.1/errutil_api.go v51 00068 (+148) MOVD $1, R0 v41 00069 (148) MOVD $go.string."index %d outside of results: %+v"(SB), R1 v39 00070 (148) MOVD $32, R2 v23 00071 (148) MOVD $github.com/cockroachdb/cockroach/pkg/kv..autotmp_16-32(RSP), R3 v33 00072 (148) MOVD $2, R4 v93 00073 (148) MOVD R4, R5 v120 00074 (+148) PCDATA $1, $3 v120 00075 (+148) CALL github.com/cockroachdb/errors/errutil.AssertionFailedWithDepthf(SB) # /Users/treilly/vec-encoding/pkg/kv/batch.go v127 00076 (+1071) MOVD $0, R2 v50 00077 (1071) MOVD R2, R3 v18 00078 (1071) MOVD $0, R4 v100 00079 (1071) MOVD R0, R5 v96 00080 (1071) MOVD R1, R6 v92 00081 (1071) MOVD $0, R0 v69 00082 (1071) MOVD $0, R1 b17 00083 (1071) RET 00084 (1071) PCDATA $1, $-1 00085 (1071) PCDATA $3, $1 v66 00086 (1064) MOVD R1, R0 v55 00087 (1064) MOVD R8, R1 v59 00088 (1064) PCDATA $1, $3 v59 00089 (1064) CALL runtime.panicIndex(SB) 00090 (1064) HINT $0 00091 (?) ENDMy way:
# /Users/treilly/vec-encoding/pkg/kv/batch.go 00000 (1058) TEXT github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult(SB), ABIInternal 00001 (1058) FUNCDATA $0, gclocals·bb+LSCCik3x40Cn8eFqL9w==(SB) 00002 (1058) FUNCDATA $1, gclocals·s4fpErQgTyW87J+tkvIERg==(SB) 00003 (1058) FUNCDATA $2, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.stkobj(SB) 00004 (1058) FUNCDATA $5, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.arginfo1(SB) 00005 (1058) FUNCDATA $6, github.com/cockroachdb/cockroach/pkg/kv.(*Batch).GetResult.argliveinfo(SB) b1 00006 (1058) PCDATA $3, $1 v80 00007 (1058) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv.b(RSP) v140 00008 (1058) MOVD R1, github.com/cockroachdb/cockroach/pkg/kv.idx+8(RSP) v140 00009 (1058) PCDATA $3, $-1 v58 00010 (+1060) MOVD $type.github.com/cockroachdb/cockroach/pkg/kv.Result(SB), R0 v17 00011 (1060) PCDATA $1, $0 v17 00012 (1060) CALL runtime.newobject(SB) v33 00013 (1060) MOVD github.com/cockroachdb/cockroach/pkg/kv.b(RSP), R1 v112 00014 (1060) MOVD 8(R1), R2 v109 00015 (1060) MOVD 16(R1), R3 b1 00016 (1060) CBZ R3, 23 v21 00017 (1060) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv.&r-40(RSP) v95 00018 (1060) MOVD R3, github.com/cockroachdb/cockroach/pkg/kv..autotmp_29-56(RSP) v60 00019 (1060) MOVD github.com/cockroachdb/cockroach/pkg/kv.idx+8(RSP), R7 v108 00020 (1058) MOVD R7, R4 v137 00021 (1058) MOVD $0, R5 b3 00022 (1060) JMP 62 v26 00023 (+1070) STP (ZR, ZR), github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-32(RSP) v86 00024 (1070) STP (ZR, ZR), github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-16(RSP) v110 00025 (1070) MOVD github.com/cockroachdb/cockroach/pkg/kv.idx+8(RSP), R0 v90 00026 (1070) PCDATA $1, $1 v90 00027 (1070) CALL runtime.convT64(SB) v97 00028 (1070) MOVD $type.int(SB), R1 v27 00029 (1070) MOVD R1, github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-32(RSP) v119 00030 (1070) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-24(RSP) v85 00031 (1070) MOVD github.com/cockroachdb/cockroach/pkg/kv.b(RSP), R1 v138 00032 (1070) MOVD 8(R1), R0 v143 00033 (1070) MOVD 16(R1), R2 v133 00034 (1070) MOVD 24(R1), R1 v136 00035 (1070) MOVD R1, R3 v24 00036 (1070) MOVD R2, R1 v64 00037 (1070) MOVD R3, R2 v104 00038 (1070) PCDATA $1, $2 v104 00039 (1070) CALL runtime.convTslice(SB) v117 00040 (1070) MOVD $type.[]github.com/cockroachdb/cockroach/pkg/kv.Result(SB), R1 v44 00041 (1070) MOVD R1, github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-16(RSP) v79 00042 (1070) MOVD R0, github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-8(RSP) v120 00043 (?) NOP # /Users/treilly/go/pkg/mod/github.com/cockroachdb/errors@v1.9.1/errutil_api.go v130 00044 (+148) MOVD $1, R0 v134 00045 (148) MOVD $go.string."index %d outside of results: %+v"(SB), R1 v141 00046 (148) MOVD $32, R2 v52 00047 (148) MOVD $github.com/cockroachdb/cockroach/pkg/kv..autotmp_17-32(RSP), R3 v69 00048 (148) MOVD $2, R4 v45 00049 (148) MOVD R4, R5 v122 00050 (+148) PCDATA $1, $3 v122 00051 (+148) CALL github.com/cockroachdb/errors/errutil.AssertionFailedWithDepthf(SB) # /Users/treilly/vec-encoding/pkg/kv/batch.go v40 00052 (+1070) MOVD $0, R2 v98 00053 (1070) MOVD R2, R3 v75 00054 (1070) MOVD $0, R4 v102 00055 (1070) MOVD R0, R5 v126 00056 (1070) MOVD R1, R6 v36 00057 (1070) MOVD $0, R0 v101 00058 (1070) MOVD $0, R1 b17 00059 (1070) RET v81 00060 (1060) ADD $96, R2, R2 v135 00061 (1060) MOVD R10, R5 v50 00062 (1060) PCDATA $0, $-2 v50 00063 (+1060) MOVD $runtime.writeBarrier(SB), R8 v78 00064 (1060) MOVWU (R8), R9 b5 00065 (1060) CBNZW R9, 74 v43 00066 (+1060) ADD $88, R2, R9 v99 00067 (1060) MOVD R0, R17 v100 00068 (1060) MOVD R2, R16 v111 00069 (1060) MOVD.P 8(R16), R27 v111 00070 (1060) MOVD.P R27, 8(R17) v111 00071 (1060) CMP R9, R16 v111 00072 (1060) BLE 69 b16 00073 (1060) JMP 88 v32 00074 (1060) MOVD R5, github.com/cockroachdb/cockroach/pkg/kv..autotmp_30-64(RSP) v51 00075 (1060) MOVD R2, github.com/cockroachdb/cockroach/pkg/kv..autotmp_31-48(RSP) v42 00076 (1061) MOVD R7, github.com/cockroachdb/cockroach/pkg/kv..autotmp_32-72(RSP) v28 00077 (+1060) MOVD R0, R1 v129 00078 (1060) MOVD $type.github.com/cockroachdb/cockroach/pkg/kv.Result(SB), R0 v123 00079 (+1060) CALL runtime.typedmemmove(SB) v132 00080 (1061) MOVD github.com/cockroachdb/cockroach/pkg/kv.&r-40(RSP), R0 v68 00081 (1070) MOVD github.com/cockroachdb/cockroach/pkg/kv.b(RSP), R1 v66 00082 (1060) MOVD github.com/cockroachdb/cockroach/pkg/kv..autotmp_31-48(RSP), R2 v65 00083 (1060) MOVD github.com/cockroachdb/cockroach/pkg/kv..autotmp_29-56(RSP), R3 v74 00084 (1070) MOVD github.com/cockroachdb/cockroach/pkg/kv.idx+8(RSP), R4 v71 00085 (1060) MOVD github.com/cockroachdb/cockroach/pkg/kv..autotmp_30-64(RSP), R5 v77 00086 (1061) MOVD github.com/cockroachdb/cockroach/pkg/kv..autotmp_32-72(RSP), R7 v76 00087 (1060) MOVD $runtime.writeBarrier(SB), R8 v37 00088 (+1061) PCDATA $0, $-1 v37 00089 (+1061) MOVD (R0), R9 v55 00090 (1061) CMP R9, R7 b15 00091 (1061) BLT 97 v73 00092 (+1060) ADD $1, R5, R10 v70 00093 (+1068) SUB R9, R7, R7 v67 00094 (1060) CMP R10, R3 b6 00095 (1060) BGT 60 b6 00096 (1060) JMP 23 v49 00097 (+1062) MOVD 24(R0), R8 v47 00098 (1062) MOVD 32(R0), R1 v94 00099 (1062) CMP R1, R7 b9 00100 (1062) BGE 112 b11 00101 (+1063) PCDATA $1, $-1 b11 00102 (+1063) BHS 119 v144 00103 (+1063) LSL $5, R7, R9 v128 00104 (1063) MOVD (R8)(R9), R1 v56 00105 (1063) ADD R7<<5, R8, R7 v125 00106 (1063) MOVD 8(R7), R2 v116 00107 (1063) MOVD 16(R7), R3 v131 00108 (1063) MOVD 24(R7), R4 v22 00109 (1063) MOVD $0, R5 v82 00110 (1063) MOVD $0, R6 b13 00111 (1063) RET v62 00112 (+1065) MOVD $0, R1 v15 00113 (1065) MOVD $0, R2 v13 00114 (1065) MOVD R2, R3 v10 00115 (1065) MOVD $0, R4 v39 00116 (1065) MOVD $0, R5 v142 00117 (1065) MOVD R1, R6 b12 00118 (1065) RET v139 00119 (1063) MOVD R7, R0 v54 00120 (1063) PCDATA $1, $3 v54 00121 (1063) CALL runtime.panicIndex(SB) 00122 (1063) HINT $0 00123 (?) END
Nevermind, you're right, the convTslice allocation in your way is for the error path argument to AssertionFailedF.
9626573 to
0de779e
Compare
nvb
left a comment
There was a problem hiding this comment.
No but I whipped one up, how's this:
Sorry, I meant do you have benchmarks that demonstrate that this new API extension can be used to provide a meaningful speedup for its expected use cases?
On that topic, what are the expected use cases where we'll use this API?
Reviewed 1 of 2 files at r9.
Reviewable status:
pkg/kv/batch.go line 91 at r8 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
I would say the contract is calling Next more than Len() times is undefined, does that pass muster?
It seems fine, though maybe better to say that it will panic. Up to you.
pkg/kv/batch.go line 1051 at r8 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
Maybe we need first class "bulk" operations so they all live under one Request object but not sure about that.
This realization and the question above about benchmarking are giving me pause about whether introducing new semi-typed, semi-specialized APIs on the Batch object is the right path, vs. using the lower-level BatchRequest API and txn.Send directly like we do in txnKVFetcher.fetch. It sounds like this new API improves perf in some areas but also leaves some on the table. Switching to the BatchRequest API would give us full control over memory allocations to facilitate batching and re-use, and it also avoids unnecessary wrapping on the response path. If we're saying that the inserter.go logic is so performance critical that it can no longer tolerate the overhead of KV's well-typed API (it's good that we're getting to this point!), should we first evaluate the approach of using its existing raw API?
cucaroach
left a comment
There was a problem hiding this comment.
So the closest thing I have is the BuildFromDatum and BuildFromCol rows here:
https://docs.google.com/spreadsheets/d/10mayXq3ICsRD__UuacT1UFT5I0I7anWzrNjNjqqFEvc/edit#gid=0
BuildFromDatum measures the speed of building the kv.Batch from datums row by row and BuildFromCol builds them from a coldata.Batch. I can't tease out how much is due to datum overhead vs the new vectorized bulk encoder vs using the bulk APIs w/o further experiments but I can do that if you'd like, I suspect the majority of the speedup comes from the new bulk encoder. I do know that we've seen surprising speedups from removing allocations so I suspect going back to the individual CPut/InitPut calls would definitely have a measurable affect and drive a lot more GC activity.
The expected use cases are COPY today, prepared batch inserts tomorrow and in the future maybe other types of mutations.
Reviewable status:
pkg/kv/batch.go line 1051 at r8 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
This realization and the question above about benchmarking are giving me pause about whether introducing new semi-typed, semi-specialized APIs on the
Batchobject is the right path, vs. using the lower-levelBatchRequestAPI andtxn.Senddirectly like we do intxnKVFetcher.fetch. It sounds like this new API improves perf in some areas but also leaves some on the table. Switching to theBatchRequestAPI would give us full control over memory allocations to facilitate batching and re-use, and it also avoids unnecessary wrapping on the response path. If we're saying that theinserter.gologic is so performance critical that it can no longer tolerate the overhead of KV's well-typed API (it's good that we're getting to this point!), should we first evaluate the approach of using its existing raw API?
Well I think having high level first class support for batch mutations is a good thing but I also agree what's the point if the API is wonky and still has in-efficiencies. I'm also intrigued by the this BatchRequest idea and hadn't considered it before. Although I feel like I could get rid of the MustSetInner cost by allocating a slice of request/union pair structs like the txnKVFetcher does. Let me give that a whirl.
What does "unnecessary wrapping on the response path" mean?
|
Okay that went well, I got rid of the per key result allocation (which should have died when I figured out the calls/numRows machinery) and I got the unions bulk allocated too ala Latest benchmark results: |
|
Another thought that occurred to me is that we only really need the bulkRequest function to live in Batch, we could make it public and move all the ergonomic wrappers around it to the vector encoding package. Would that be better for now? |
nvb
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r9, 1 of 1 files at r11, all commit messages.
Reviewable status:
pkg/kv/batch.go line 614 at r11 (raw file):
union kvpb.RequestUnion_ConditionalPut }, numKeys)
nit: stray line.
These are simple analogs to existing APIs that do all the memory allocations in bulk. Epic: CRDB-18892 Informs: cockroachdb#91831 Release note: None
|
bors r=nvanbenschoten |
|
Build failed: |
|
bors retry |
|
Build succeeded: |
These are simple analogs to existing APIs that do all the memory
allocations in bulk.
Epic: CRDB-18892
Informs: #91831
Release note: None