Skip to content
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

codec: implement in-place byte encoding #6061

Merged

Conversation

@wangwangwar
Copy link
Contributor

wangwangwar commented Nov 26, 2019

PCP #5703

Signed-off-by: wangwangwar wangwangwar@gmail.com

What have you changed?

  1. Implement in-place byte encoding for components/codec. Add functions MemComparableByteCodec::encode_all_in_place and MemComparableByteCodec::encode_all_in_place_desc.

  2. Some questions:

  • I provide in-place byte encoding function as MemComparableByteCodec::encode_all_in_place(src: &mut Vec<u8>) -> usize, but the parameter src's type is &mut Vec<u8>, which is different from parameter of MemComparableByteCodec::try_decode_first_in_place(buffer: &mut [u8]) -> Result<(usize, usize)>. I don't know whether it's OK.
  • Who is responsible to make sure the capacity of src is sufficient? Function encode_all_in_place or the caller, which one is better?

TODO: Implement in-place byte encoding for tikv_utils/codec.

What is the type of the changes?

Improvement

How is the PR tested?

Unit test

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No. It should not change the behavior.

Does this PR affect tidb-ansible?

No

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

test byte::benches::bench_memcmp_encode_all_asc_large                                 ... bench:         103 ns/iter (+/- 5)
test byte::benches::bench_memcmp_encode_all_asc_large_naive                           ... bench:         202 ns/iter (+/- 12)
test byte::benches::bench_memcmp_encode_all_asc_large_original                        ... bench:         574 ns/iter (+/- 23)
test byte::benches::bench_memcmp_encode_all_asc_small                                 ... bench:          14 ns/iter (+/- 1)
test byte::benches::bench_memcmp_encode_all_asc_small_naive                           ... bench:          21 ns/iter (+/- 1)
test byte::benches::bench_memcmp_encode_all_desc_large                                ... bench:         138 ns/iter (+/- 5)
test byte::benches::bench_memcmp_encode_all_desc_large_original                       ... bench:       1,444 ns/iter (+/- 199)
test byte::benches::bench_memcmp_encode_all_desc_small                                ... bench:          28 ns/iter (+/- 1)
test byte::benches::bench_memcmp_encode_all_in_place_asc_large                        ... bench:         204 ns/iter (+/- 7)
test byte::benches::bench_memcmp_encode_all_in_place_asc_small                        ... bench:          64 ns/iter (+/- 4)
test byte::benches::bench_memcmp_encode_all_in_place_desc_large                       ... bench:         235 ns/iter (+/- 11)
test byte::benches::bench_memcmp_encode_all_in_place_desc_small                       ... bench:          80 ns/iter (+/- 4)

Any examples? (optional)

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 26, 2019

Thanks for your contribution. If your PR get merged, you will be rewarded 100 points.

@wangwangwar wangwangwar force-pushed the wangwangwar:pcp/implement-in-place-byte-encoding branch 4 times, most recently from fd77a73 to 88e33ef Nov 27, 2019
components/codec/src/byte.rs Outdated Show resolved Hide resolved
components/codec/src/byte.rs Outdated Show resolved Hide resolved
@wangwangwar wangwangwar force-pushed the wangwangwar:pcp/implement-in-place-byte-encoding branch from 67fb474 to 5e3b0a9 Nov 28, 2019
components/codec/src/byte.rs Outdated Show resolved Hide resolved
wangwangwar and others added 3 commits Nov 26, 2019
Signed-off-by: wangwangwar <wangwangwar@gmail.com>
Signed-off-by: wangwangwar <wangwangwar@gmail.com>
…t [u8]`

Signed-off-by: wangwangwar <wangwangwar@gmail.com>
@wangwangwar wangwangwar force-pushed the wangwangwar:pcp/implement-in-place-byte-encoding branch from 1ccb13e to 2f8cf18 Nov 30, 2019
@breeswish breeswish removed the S: Waiting label Dec 1, 2019
@wangwangwar wangwangwar requested a review from TennyZhuang Dec 1, 2019
@wangwangwar wangwangwar force-pushed the wangwangwar:pcp/implement-in-place-byte-encoding branch from 2522725 to 961142f Dec 1, 2019
@wangwangwar

This comment has been minimized.

Copy link
Contributor Author

wangwangwar commented Dec 1, 2019

@sre-bot /test

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Dec 1, 2019

/test

Signed-off-by: wangwangwar <wangwangwar@gmail.com>
@wangwangwar wangwangwar force-pushed the wangwangwar:pcp/implement-in-place-byte-encoding branch from 961142f to e7e4ba4 Dec 1, 2019
Copy link
Member

breeswish left a comment

Good job!

components/codec/src/byte.rs Outdated Show resolved Hide resolved
components/codec/src/byte.rs Show resolved Hide resolved
components/codec/src/byte.rs Outdated Show resolved Hide resolved
components/codec/src/byte.rs Outdated Show resolved Hide resolved
components/codec/src/byte.rs Outdated Show resolved Hide resolved
wangwangwar added 2 commits Dec 3, 2019
Signed-off-by: wangwangwar <wangwangwar@gmail.com>
Signed-off-by: wangwangwar <wangwangwar@gmail.com>
@wangwangwar

This comment has been minimized.

Copy link
Contributor Author

wangwangwar commented Dec 3, 2019

New benchmarks:

test byte::benches::bench_memcmp_encode_all_asc_large                                 ... bench:         103 ns/iter (+/- 5)
test byte::benches::bench_memcmp_encode_all_asc_large_naive                           ... bench:         182 ns/iter (+/- 13)
test byte::benches::bench_memcmp_encode_all_asc_large_original                        ... bench:         572 ns/iter (+/- 12)
test byte::benches::bench_memcmp_encode_all_asc_small                                 ... bench:          16 ns/iter (+/- 6)
test byte::benches::bench_memcmp_encode_all_asc_small_naive                           ... bench:          23 ns/iter (+/- 8)
test byte::benches::bench_memcmp_encode_all_desc_large                                ... bench:         140 ns/iter (+/- 8)
test byte::benches::bench_memcmp_encode_all_desc_large_original                       ... bench:       1,445 ns/iter (+/- 92)
test byte::benches::bench_memcmp_encode_all_desc_small                                ... bench:          29 ns/iter (+/- 2)
test byte::benches::bench_memcmp_encode_all_in_place_asc_large                        ... bench:         172 ns/iter (+/- 36)
test byte::benches::bench_memcmp_encode_all_in_place_asc_small                        ... bench:          34 ns/iter (+/- 3)
test byte::benches::bench_memcmp_encode_all_in_place_desc_large                       ... bench:         205 ns/iter (+/- 32)
test byte::benches::bench_memcmp_encode_all_in_place_desc_small                       ... bench:          52 ns/iter (+/- 6)
Signed-off-by: wangwangwar <wangwangwar@gmail.com>
@wangwangwar wangwangwar requested review from niedhui and breeswish Dec 3, 2019
Copy link
Member

breeswish left a comment

Thanks a lot! LGTM

@breeswish breeswish added the S: LGT1 label Dec 4, 2019
@niedhui
niedhui approved these changes Dec 4, 2019
Copy link
Collaborator

niedhui left a comment

LGTM. Thanks

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Dec 4, 2019

/merge

@sre-bot sre-bot added the S: CanMerge label Dec 4, 2019
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Dec 4, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Dec 4, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Dec 4, 2019

@wangwangwar merge failed.

@wangwangwar

This comment has been minimized.

Copy link
Contributor Author

wangwangwar commented Dec 4, 2019

@sre-bot /run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Dec 4, 2019

/run-all-tests

@breeswish breeswish merged commit 5a5a08a into tikv:master Dec 4, 2019
5 of 6 checks passed
5 of 6 checks passed
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job failed
Details
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-copr-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Dec 4, 2019

@wangwangwar complete task #5703 and get 100 score, currerent score 200

@wangwangwar wangwangwar deleted the wangwangwar:pcp/implement-in-place-byte-encoding branch Dec 4, 2019
zhang555 added a commit to zhang555/tikv that referenced this pull request Dec 16, 2019
Signed-off-by: wangwangwar <wangwangwar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.