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

Reduce the size of `Duration` to 8 bytes #4858

Merged
merged 25 commits into from Jul 3, 2019

Conversation

Projects
None yet
10 participants
@iosmanthus
Copy link
Contributor

commented Jun 7, 2019

  • Reduce the size of Duration from 24 bytes to 8 bytes
  • Accelerate codec of Duration

What have you changed? (mandatory)

This a split PR of #4427. It tries to reduce the size of Duration to 8 bytes simulating bitfield in C/C++ so that it fits in just one register.

What are the type of changes? (mandatory)

  • Improvement (a change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

cargo test coprocessor

Reduce the size of `Duration` to 8 bytes
* Reduce the size of `Duration` from 24 bytes to 8 bytes
* Accelerate codec of `Duration`

Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>

@iosmanthus iosmanthus force-pushed the iosmanthus:refactor-duration branch from abcb34a to 086767f Jun 7, 2019

@iosmanthus

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

PTAL @breeswish

Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/time/mod.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated

@overvenus overvenus added the C: Copr label Jun 8, 2019

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

how about MySQL handles Duration type?

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

how about the performance gained?

@ngaut

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

Can tidb do the same thing? @zz-jason

@zz-jason

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Can tidb do the same thing? @zz-jason

Durtion in TiDB is stored with 8 bytes since this PR: pingcap/tidb#7043

@iosmanthus

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

how about MySQL handles Duration type?

https://dev.mysql.com/doc/internals/en/date-and-time-data-type-representation.html
And the memory layout is:

 1 bit sign    (1= non-negative, 0= negative)
 1 bit unused  (reserved for future extensions)
10 bits hour   (0-838)
 6 bits minute (0-59) 
 6 bits second (0-59) 
---------------------
24 bits = 3 bytes

@kennytm

@kennytm

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@iosmanthus ?

We don't need to completely copy MySQL's internal data representation.

@iosmanthus

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@iosmanthus ?

We don't need to completely copy MySQL's internal data representation.

OK. I will fix it. 😃

@iosmanthus

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

how about the performance gained?

Here is the benchmark: @siddontang

[I] ➜ cargo benchcmp before after
 name                                                                            before ns/iter  after ns/iter  diff ns/iter   diff %  speedup 
 coprocessor::codec::mysql::duration::benches::bench_check_add_and_sub_duration  161             288                     127   78.88%   x 0.56 
 coprocessor::codec::mysql::duration::benches::bench_codec                       628             145                    -483  -76.91%   x 4.33 
 coprocessor::codec::mysql::duration::benches::bench_hours                       3,001           2,062                  -939  -31.29%   x 1.46 
 coprocessor::codec::mysql::duration::benches::bench_parse                       417             355                     -62  -14.87%   x 1.17 
 coprocessor::codec::mysql::duration::benches::bench_round_frac                  18              11                       -7  -38.89%   x 1.64 
 coprocessor::codec::mysql::duration::benches::bench_to_decimal                  248             243                      -5   -2.02%   x 1.02

iosmanthus added some commits Jun 12, 2019

Add benchmark for `Duration`
* Rearrange bitfield of `Duration`
* Fix some spelling mistakes
* Fix `Display` for `Duration`

Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
Rewrite `checked_add/checked_sub` for `Duration`
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
@iosmanthus

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Latest benchmark:

[I] ➜ cargo benchcmp before latest
 name                                                                              before ns/iter  latest ns/iter  diff ns/iter   diff %  speedup 
 coprocessor::codec::mysql::duration::benches::bench_checked_add_and_sub_duration  159             121                      -38  -23.90%   x 1.31 
 coprocessor::codec::mysql::duration::benches::bench_codec                         537             140                     -397  -73.93%   x 3.84 
 coprocessor::codec::mysql::duration::benches::bench_hours                         2,995           1,988                 -1,007  -33.62%   x 1.51 
 coprocessor::codec::mysql::duration::benches::bench_parse                         416             349                      -67  -16.11%   x 1.19 
 coprocessor::codec::mysql::duration::benches::bench_round_frac                    17              11                        -6  -35.29%   x 1.55 
 coprocessor::codec::mysql::duration::benches::bench_to_decimal                    238             238                        0    0.00%   x 1.00 

iosmanthus added some commits Jun 12, 2019

Mask the unused bit
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
Add `to_bits/from_bits` for `Duration`
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
Set `subsec_nanos` to private
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
Set `subsec_nanos` back to public...
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
Show resolved Hide resolved components/tikv_util/src/time.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs

iosmanthus and others added some commits Jun 14, 2019

Rename `as_*` to `to_*`
* Rename `as_*` to `to_*`
* Rewrite `Duration::build`

Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
Remove `Duration::build`
* Add `Duration::round` to round `hours/minutes/seconds/micros`
* Rewrite `Duration::new` to construct `Duration` without validation
* Add `Duration::from_millis`

Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
@breeswish
Copy link
Member

left a comment

The rest looks good to me. @sticnarf PTAL

Show resolved Hide resolved src/coprocessor/dag/rpn_expr/impl_compare.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Show resolved Hide resolved src/coprocessor/codec/mysql/duration.rs Outdated
Reuse some code
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>

iosmanthus and others added some commits Jun 29, 2019

Fix unit test failing in rpn function module
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
@breeswish
Copy link
Member

left a comment

Awesome! The rest looks fine!

@breeswish

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

/run-integration-tests

@breeswish

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@sticnarf PTAL

iosmanthus added some commits Jul 1, 2019

Use `write!` to format `Duration`
* Modify some comment
* Add constant MICRO_WIDTH

Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
@sticnarf

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

/run-integration-tests

@sticnarf
Copy link
Contributor

left a comment

LGTM

Impl TryFrom<Duration> for Decimal
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
hours: &mut u32,
minutes: &mut u32,
secs: &mut u32,
micros: &mut u32,

This comment has been minimized.

Copy link
@H-ZeX

H-ZeX Jul 1, 2019

Contributor

Add comment and debug_assert! that the micros has 6 or 7 decimal number

@breeswish

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

/run-integration-tests

@iosmanthus iosmanthus merged commit 6a81268 into tikv:master Jul 3, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
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.