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: Some small improvements #4664

Merged
merged 10 commits into from May 10, 2019

Conversation

Projects
None yet
5 participants
@breeswish
Copy link
Member

commented May 9, 2019

Signed-off-by: Breezewish breezewish@pingcap.com

What have you changed? (mandatory)

This PR makes some small improvements to the codec component, followed by 7a8fdf9

  • Support incomplete buffers when calculating the bytes length
  • Support calculating VarInt length (so that we can now skip a VarInt without decoding)
  • Add likely and unlikely hints for if statements
  • Refined error type to shrink its size. size_of(Error) is now 8.

What are the type of the changes? (mandatory)

  • Engineering (engineering change which doesn't change any feature or fix any issue)

How has this PR been tested? (mandatory)

New unit test.

Improve codec
Signed-off-by: Breezewish <breezewish@pingcap.com>

@breeswish breeswish added the C: Perf label May 9, 2019

Show resolved Hide resolved components/codec/src/buffer.rs Outdated
Show resolved Hide resolved components/codec/src/buffer.rs Outdated

breeswish added some commits May 9, 2019

Address comments to simplify the code
Signed-off-by: Breezewish <breezewish@pingcap.com>
Show resolved Hide resolved components/codec/src/byte.rs Outdated
Show resolved Hide resolved components/codec/src/number.rs Outdated
Show resolved Hide resolved components/codec/src/number.rs Outdated
@ngaut

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Could you show the benchmark results?

@breeswish

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

@ngaut

Decode VarInt takes 2ns in fastest path, takes 4ns in normal path, takes 5ns in slow path.
Skip VarInt takes 1ns in fast path, 2ns in slow path.

@breeswish breeswish force-pushed the breeswish:improve-codec branch from 06aaec8 to 3c1bbd1 May 10, 2019

breeswish added some commits May 10, 2019

Improve error handling
Signed-off-by: Breezewish <breezewish@pingcap.com>

@breeswish breeswish force-pushed the breeswish:improve-codec branch from 3c1bbd1 to 0ddac43 May 10, 2019

Show resolved Hide resolved components/codec/src/byte.rs Outdated
Show resolved Hide resolved components/codec/src/byte.rs Outdated
while ptr != ptr_end && *ptr >= 0x80 {
ptr = ptr.add(1);
}
// When we got here, we are either `ptr == ptr_end`, or `*ptr < 0x80`.

This comment has been minimized.

Copy link
@lonng

lonng May 10, 2019

Contributor

The assert is not always true, e.g [0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x7f](9 bytes) both ptr == ptr_end and *ptr < 0x80 are true.

This comment has been minimized.

Copy link
@breeswish

breeswish May 10, 2019

Author Member
[0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x7f]
                                             ^ptr_end

this is ptr_end. Your case is only *ptr < 0x80

breeswish added some commits May 10, 2019

Update comments for the new error
Signed-off-by: Breezewish <breezewish@pingcap.com>
@lonng
Copy link
Contributor

left a comment

LGTM

Move Box to outside
Signed-off-by: Breezewish <breezewish@pingcap.com>

@breeswish breeswish force-pushed the breeswish:improve-codec branch from 010e100 to d14b238 May 10, 2019

@lonng

lonng approved these changes May 10, 2019

Copy link
Contributor

left a comment

LGTM

@breeswish breeswish merged commit bedea35 into tikv:master May 10, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@breeswish breeswish deleted the breeswish:improve-codec branch May 10, 2019

H-ZeX added a commit to H-ZeX/tikv that referenced this pull request May 11, 2019

codec: Some small improvements (tikv#4664)
Signed-off-by: Breezewish <breezewish@pingcap.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.