Skip to content

*: refine the attribute definition of types.Time and types.Duration#11672

Merged
alivxxx merged 18 commits into
pingcap:masterfrom
XuHuaiyu:refine_time
Aug 14, 2019
Merged

*: refine the attribute definition of types.Time and types.Duration#11672
alivxxx merged 18 commits into
pingcap:masterfrom
XuHuaiyu:refine_time

Conversation

@XuHuaiyu
Copy link
Copy Markdown
Contributor

@XuHuaiyu XuHuaiyu commented Aug 8, 2019

What problem does this PR solve?

  1. Change the type of types.MysqlTime.hour from int to uint32
  2. Change the type of types.Time.Fsp from int to int8
  3. Change the type of types.Duration.Fsp from int to int8

After 1 and 2, the memory usage of a Time struct will be reduced from 40bytes to 20 bytes.

What is changed and how it works?

Check List

Tests

  • No code

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change

Side effects

N/A

Related changes

@XuHuaiyu XuHuaiyu added type/enhancement The issue or PR belongs to an enhancement. status/WIP labels Aug 8, 2019
@XuHuaiyu
Copy link
Copy Markdown
Contributor Author

/run-all-tests

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 12, 2019

Codecov Report

Merging #11672 into master will decrease coverage by 0.6059%.
The diff coverage is 96.875%.

@@               Coverage Diff               @@
##             master     #11672       +/-   ##
===============================================
- Coverage   82.0336%   81.4276%   -0.606%     
===============================================
  Files           433        432        -1     
  Lines         96497      93090     -3407     
===============================================
- Hits          79160      75801     -3359     
- Misses        11744      11870      +126     
+ Partials       5593       5419      -174

Comment thread types/datum_test.go
Comment thread types/mytime.go Outdated
Comment thread types/mytime.go
Comment thread types/mytime.go Outdated
@XuHuaiyu XuHuaiyu requested review from qw4990 and zz-jason August 13, 2019 03:08
Copy link
Copy Markdown
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qw4990 qw4990 added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 13, 2019
Comment thread expression/helper.go Outdated
Comment thread types/time.go
Copy link
Copy Markdown
Contributor

@tiancaiamao tiancaiamao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no problem with the change of fsp from int to int8, and change hour to int16 seems ok.
But why mytime.go is modified?

Comment thread expression/builtin_time.go
Comment thread types/mytime.go Outdated
Comment thread types/mytime.go
Comment thread types/mytime.go Outdated
Comment thread types/mytime.go Outdated
Comment thread expression/distsql_builtin_test.go
@XuHuaiyu
Copy link
Copy Markdown
Contributor Author

/run-all-tests

Comment thread types/mytime_test.go Outdated
"time"

. "github.com/pingcap/check"
"unsafe"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put it with system packages?

@XuHuaiyu XuHuaiyu requested a review from alivxxx August 13, 2019 09:39
Copy link
Copy Markdown
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

alivxxx
alivxxx previously approved these changes Aug 13, 2019
Copy link
Copy Markdown
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alivxxx alivxxx added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 13, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 13, 2019

/run-all-tests

@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 13, 2019

@XuHuaiyu merge failed.

@XuHuaiyu
Copy link
Copy Markdown
Contributor Author

/run-common-test tidb-test=pr/874
/run-integration-common-test tidb-test=pr/874

@XuHuaiyu
Copy link
Copy Markdown
Contributor Author

/run-all-tests

@XuHuaiyu
Copy link
Copy Markdown
Contributor Author

/run-unit-test

1 similar comment
@XuHuaiyu
Copy link
Copy Markdown
Contributor Author

/run-unit-test

@alivxxx alivxxx merged commit adb3071 into pingcap:master Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants