Skip to content

[DO NOT MERGE] build: post-process .pb.go to not copy []byte in Unmarshal#6208

Closed
petermattis wants to merge 2 commits intocockroachdb:masterfrom
petermattis:pmattis/no-copy-bytes
Closed

[DO NOT MERGE] build: post-process .pb.go to not copy []byte in Unmarshal#6208
petermattis wants to merge 2 commits intocockroachdb:masterfrom
petermattis:pmattis/no-copy-bytes

Conversation

@petermattis
Copy link
Copy Markdown
Collaborator

@petermattis petermattis commented Apr 21, 2016

Fix the one case where this wasn't safe: rocksDBIterator.Unmarshal() now
unmarshals rocksDBIterator.Value() instead of unsafeValue().

This change is Reviewable

@petermattis
Copy link
Copy Markdown
Collaborator Author

petermattis commented Apr 21, 2016

Not sure how I feel about this change. I wanted to see how irritating it would be to make (it wasn't), but it is changing the semantics of Unmarshal in a way that makes me nervous. Might be cleaner (though more work) to add a new proto-tag and unmarshaling code upstream. I'll add benchmark numbers later.

@petermattis petermattis changed the title build: post-process .pb.go to not copy []byte in Unmarshal [DO NOT MERGE] build: post-process .pb.go to not copy []byte in Unmarshal Apr 21, 2016
@petermattis petermattis force-pushed the pmattis/no-copy-bytes branch from 41619a3 to 3c85585 Compare April 21, 2016 19:01
@bdarnell
Copy link
Copy Markdown
Contributor

bdarnell commented Apr 21, 2016

I'm wary of changing the semantics too. It should be safe in most of our usage but I'd prefer to be able to opt-in only for those protos where we expect it to make a difference.

Note that for the message that started this discussion, raftpb.Entry, we don't control the generated code, so this doesn't actually change anything there. (we do, however, control the call sites to Entry.Unmarshal, so we could change them to point to our optimized version if we devised some way to generate one)

@petermattis
Copy link
Copy Markdown
Collaborator Author

petermattis commented Apr 21, 2016

Ah, unmarshaling raftpb.Entry makes another copy. Nice. Yes, this won't fix that. It does fix the copying in unmarshaling BatchRequest though, which is important even outside of raft.

@bdarnell bdarnell mentioned this pull request Apr 21, 2016
Merged
@petermattis
Copy link
Copy Markdown
Collaborator Author

petermattis commented Apr 22, 2016

Here are some numbers showing the allocation savings:

name                     old allocs/op  new allocs/op  delta
KVInsert1_Native-32           339 ± 0%       329 ± 0%   -2.95%        (p=0.000 n=16+17)
KVInsert1_SQL-32              311 ± 0%       302 ± 0%   -2.89%        (p=0.000 n=18+18)
KVInsert10_Native-32          635 ± 0%       580 ± 0%   -8.66%        (p=0.000 n=20+19)
KVInsert10_SQL-32             840 ± 0%       776 ± 0%   -7.52%        (p=0.000 n=20+20)
KVInsert100_Native-32       3.49k ± 0%     2.98k ± 0%  -14.49%        (p=0.000 n=20+20)
KVInsert100_SQL-32          5.94k ± 0%     5.34k ± 0%  -10.22%        (p=0.000 n=19+20)
KVInsert1000_Native-32      31.8k ± 0%     26.7k ± 0%  -15.96%        (p=0.000 n=20+20)
KVInsert1000_SQL-32         57.7k ± 1%     51.1k ± 1%  -11.56%        (p=0.000 n=20+20)
KVInsert10000_Native-32      324k ± 4%      266k ± 1%  -17.98%        (p=0.000 n=20+20)
KVInsert10000_SQL-32         684k ± 9%      550k ± 3%  -19.63%        (p=0.000 n=19+20)
KVUpdate1_Native-32           542 ± 0%       530 ± 0%   -2.21%        (p=0.000 n=20+20)
KVUpdate1_SQL-32              605 ± 0%       599 ± 0%   -1.02%        (p=0.000 n=20+20)
KVUpdate10_Native-32        1.05k ± 0%     0.97k ± 0%   -7.16%        (p=0.000 n=20+19)
KVUpdate10_SQL-32           1.20k ± 0%     1.17k ± 0%   -2.76%        (p=0.000 n=19+19)
KVUpdate100_Native-32       5.81k ± 0%     5.11k ± 0%  -12.10%        (p=0.000 n=19+20)
KVUpdate100_SQL-32          6.87k ± 0%     6.56k ± 0%   -4.43%        (p=0.000 n=20+20)
KVUpdate1000_Native-32      53.1k ± 0%     46.0k ± 0%  -13.44%        (p=0.000 n=20+20)
KVUpdate1000_SQL-32         63.1k ± 0%     60.1k ± 0%   -4.81%        (p=0.000 n=19+20)
KVUpdate10000_Native-32      551k ± 4%      466k ± 2%  -15.54%        (p=0.000 n=20+20)
KVUpdate10000_SQL-32         678k ± 4%      637k ± 1%   -6.05%        (p=0.000 n=20+20)
KVDelete1_Native-32           329 ± 0%       321 ± 0%   -2.43%        (p=0.000 n=20+20)
KVDelete1_SQL-32              450 ± 0%       443 ± 0%   -1.52%        (p=0.000 n=20+20)
KVDelete10_Native-32          600 ± 0%       565 ± 0%   -5.86%        (p=0.000 n=20+20)
KVDelete10_SQL-32             881 ± 0%       838 ± 0%   -4.88%        (p=0.000 n=18+20)
KVDelete100_Native-32       3.17k ± 0%     2.87k ± 0%   -9.60%        (p=0.000 n=17+19)
KVDelete100_SQL-32          4.98k ± 0%     4.58k ± 0%   -8.11%        (p=0.000 n=19+19)
KVDelete1000_Native-32      28.6k ± 1%     25.6k ± 0%  -10.69%        (p=0.000 n=18+20)
KVDelete1000_SQL-32         46.2k ± 1%     41.9k ± 1%   -9.17%        (p=0.000 n=19+20)
KVDelete10000_Native-32      289k ± 4%      254k ± 1%  -12.28%        (p=0.000 n=20+20)
KVDelete10000_SQL-32         514k ± 8%      448k ± 5%  -12.88%        (p=0.000 n=20+19)
KVScan1_Native-32             217 ± 0%       213 ± 0%   -1.84%        (p=0.000 n=20+20)
KVScan1_SQL-32                203 ± 0%       203 ± 0%     ~     (all samples are equal)
KVScan10_Native-32            262 ± 0%       240 ± 0%   -8.29%        (p=0.000 n=20+17)
KVScan10_SQL-32               277 ± 0%       277 ± 0%     ~           (p=0.741 n=20+20)
KVScan100_Native-32           630 ± 0%       428 ± 0%  -32.06%        (p=0.000 n=16+16)
KVScan100_SQL-32              937 ± 0%       937 ± 0%   +0.03%        (p=0.033 n=20+17)
KVScan1000_Native-32        4.27k ± 0%     2.27k ± 0%  -46.89%        (p=0.000 n=20+20)
KVScan1000_SQL-32           7.44k ± 0%     7.45k ± 0%   +0.02%        (p=0.013 n=16+18)
KVScan10000_Native-32       40.6k ± 0%     20.6k ± 0%  -49.28%        (p=0.000 n=20+20)
KVScan10000_SQL-32          72.5k ± 0%     72.5k ± 0%     ~           (p=0.128 n=19+20)

Note that because these benchmarks are running on a single node with the local-call optimization enabled, the benefits are underestimated. The time improvements are more modest, in the single digit range, mostly accruing to the Native versions presumably because this change will help GRPC performance.

@petermattis petermattis force-pushed the pmattis/no-copy-bytes branch from 3c85585 to 9603c00 Compare April 22, 2016 21:29
@petermattis
Copy link
Copy Markdown
Collaborator Author

petermattis commented Apr 22, 2016

Added another commit which replaces the unmarshaling of raftpb.Entry with a new roachpb.RaftEntry proto that is structurally equivalent. Doesn't seem to help much:

name                  old time/op    new time/op    delta
KVInsert1_SQL-32         495µs ± 1%     497µs ± 2%     ~     (p=0.461 n=19+18)
KVInsert10_SQL-32        945µs ± 2%     943µs ± 1%     ~     (p=0.478 n=20+20)
KVInsert100_SQL-32      5.19ms ± 4%    5.16ms ± 2%     ~     (p=0.242 n=20+20)
KVInsert1000_SQL-32     51.9ms ± 3%    51.3ms ± 3%     ~     (p=0.072 n=20+18)
KVInsert10000_SQL-32     693ms ± 6%     690ms ± 9%     ~     (p=0.369 n=20+20)

name                  old allocs/op  new allocs/op  delta
KVInsert1_SQL-32           309 ± 0%       300 ± 0%   -2.91%  (p=0.000 n=18+17)
KVInsert10_SQL-32          815 ± 0%       752 ± 0%   -7.76%  (p=0.000 n=20+16)
KVInsert100_SQL-32       5.73k ± 0%     5.13k ± 0%  -10.59%  (p=0.000 n=20+20)
KVInsert1000_SQL-32      55.5k ± 2%     49.0k ± 0%  -11.76%  (p=0.000 n=19+20)
KVInsert10000_SQL-32      654k ± 5%      527k ± 3%  -19.45%  (p=0.000 n=19+20)

@bdarnell
Copy link
Copy Markdown
Contributor

bdarnell commented Apr 22, 2016

Replacing raftpb.Entry may not be a big win for these SQL benchmarks, but @tamird found unmarshaling entries to be a large part of the cost of generating a snapshot.

Another idea for doing this without post-processing would be to make a custom type that wraps a []byte and implements Unmarshal (as we've done for UUID). Gogoproto gives Unmarshal a slice into the input data, which we could save. We could use this custom type for bytes fields in cases where we're confident that pointing into the input data is safe and where the savings are significant.

@petermattis
Copy link
Copy Markdown
Collaborator Author

petermattis commented Apr 23, 2016

Yeah, I also had the thought about using a customtype. We moved away from using customtype a while ago and I can't recall the exact reason why. Perhaps @tamird remembers. I'm also not sure that the encoding would be exactly same. Something to investigate.

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Apr 23, 2016

#671

Changes of []byte to casttype (e.g. roachpb.Key) were only done because
it required less code.

On Fri, Apr 22, 2016 at 8:36 PM, Peter Mattis notifications@github.com
wrote:

Yeah, I also had the thought about using a customtype. We moved away from
using customtype a while ago and I can't recall the exact reason why.
Perhaps @tamird https://github.com/tamird remembers. I'm also not sure
that the encoding would be exactly same. Something to investigate.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6208 (comment)

@petermattis
Copy link
Copy Markdown
Collaborator Author

petermattis commented Apr 25, 2016

Switch back to customtype for Key isn't quite seamless. Nullable []byte fields are stored in protos as themselves, but when using customtype they are pointers. For example, when using casttype we have

type Span struct {
         Key Key
         EndKey Key
}

When using customtype we have:

type Span struct {
         Key *Key
         EndKey *Key
}

If I make the fields non-nullable then the encoding changes such that we always output the field even if the field == nil.

@petermattis petermattis mentioned this pull request Apr 26, 2016
Merged
@tamird tamird self-assigned this Apr 26, 2016
@petermattis
build: post-process .pb.go to not copy []byte in Unmarshal
97e81a7 
Fix the one case where this wasn't safe: rocksDBIterator.Unmarshal() now
unmarshals rocksDBIterator.Value() instead of unsafeValue().
@petermattis
storage,roachpb: add roachpb.RaftEntry for more efficient unmarshaling
e40c7ac
@petermattis petermattis force-pushed the pmattis/no-copy-bytes branch from 9603c00 to e40c7ac Compare April 28, 2016 17:55
@petermattis
Copy link
Copy Markdown
Collaborator Author

petermattis commented Jun 2, 2016

Closing the PR as this isn't going to be merged any time soon. I'll keep the branch.

@petermattis petermattis closed this Jun 2, 2016
@petermattis petermattis deleted the pmattis/no-copy-bytes branch November 28, 2016 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@tamird tamird

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@petermattis @bdarnell @tamird