storage: avoid copying marshalled RaftCommand when encoding #36392
Conversation
|
This change is |
|
cc. @dt Do you have any benchmarks that stress |
There was a problem hiding this comment.
Nice
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
Informs cockroachdb#36347. This change avoids the unnecessary allocation and memory copy present in Raft command encoding. This extra work is expensive for large commands like `AddSSTable` requests. Even for smaller requests, this work was still a serious problem because it took place under heavily contended locks. For instance, the encoding in `defaultSubmitProposalLocked` took place under the Replica mutex, which serializes all Raft proposals for a Range. The other two locations fixed took place under the Raft mutex. While less heavily contended, this was still slowing down the Raft processing goroutine. This is a less dramatic version of a change I've been working on. The full change lifts the slice allocation and most of the RaftCommand proto marshalling all the way out of `defaultSubmitProposalLocked` and out of the `Replica.mu` critical section. This commit gets us part of the way there sets the stage for the rest of the change. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/avoidAllocCmd
branch
from
b38c984 to
91abab1
Compare
|
I don't think we have any well targeted |
|
Nice, this does show up on cc. @danhhz |
Hell yes. 🙌 I do have a local branch where I started added benchmarks to understand the costs at various levels of AddSSTable vs just writing the data to disk, etc. I just pushed it to my fork if you want to see if it applies cleanly. https://github.com/cockroachdb/cockroach/compare/master...danhhz:fast_addsstable?expand=1 |
|
Unfortunately, it didn't rebase cleanly. Are you planning on getting that in any time soon @danhhz? |
|
bors r+ |
It was worth a shot. Thanks for checking. Landing this is unfortunately quite low on my flex friday list right now |
36392: storage: avoid copying marshalled RaftCommand when encoding r=nvanbenschoten a=nvanbenschoten Informs #36347. This change avoids the unnecessary allocation and memory copy present in Raft command encoding. This extra work is expensive for large commands like `AddSSTable` requests. Even for smaller requests, this work was still a serious problem because it took place under heavily contended locks. For instance, the encoding in `defaultSubmitProposalLocked` took place under the Replica mutex, which serializes all Raft proposals for a Range. The other two locations fixed took place under the Raft mutex. While less heavily contended, this was still slowing down the Raft processing goroutine. This is a less dramatic version of a change I've been working on. The full change lifts the slice allocation and most of the RaftCommand proto marshalling all the way out of `defaultSubmitProposalLocked` and out of the `Replica.mu` critical section. This commit gets us part of the way there sets the stage for the rest of the change. Release note: None Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
|
@dt should I push to get this in for 19.1? |
|
@nvanbenschoten If you felt so inclined, sure! I don't think mem usage is on our current list of must-resolve 19.1 backfiller so wasn't planning on lobbying you to do so, but everyone likes faster and using less mem, so if you're comfortable backporting it, I would't complain. |
|
I did eventually get to merging that branch and ran it on master with and without this PR. |
Informs #36347.
This change avoids the unnecessary allocation and memory copy present in
Raft command encoding. This extra work is expensive for large commands
like
AddSSTablerequests. Even for smaller requests, this work wasstill a serious problem because it took place under heavily contended
locks. For instance, the encoding in
defaultSubmitProposalLockedtookplace under the Replica mutex, which serializes all Raft proposals for
a Range. The other two locations fixed took place under the Raft mutex.
While less heavily contended, this was still slowing down the Raft
processing goroutine.
This is a less dramatic version of a change I've been working on. The
full change lifts the slice allocation and most of the RaftCommand proto
marshalling all the way out of
defaultSubmitProposalLockedand out ofthe
Replica.mucritical section. This commit gets us part of the waythere sets the stage for the rest of the change.
Release note: None