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

copr: improve in performance using lookup set in metadata #6000

Merged
merged 10 commits into from Nov 27, 2019

Conversation

@TennyZhuang
Copy link
Contributor

TennyZhuang commented Nov 21, 2019

PCP #5713

Signed-off-by: TennyZhuang zty0826@gmail.com

What have you changed?

Please explain in detail what the changes are in this PR and why they are needed:

  • Implement Hash for Decimal and Time.
  • fuzz test Decimal hash
  • optimize compare_in performance using metadata.

Assume that the number of rows is n , and the number of constant args is m, then the vec_compare_in optimized from O(n*m) to O(n+m)

What is the type of the changes?

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

How is the PR tested?

Please select the tests that you ran to verify your changes:

  • Unit test

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

Does this PR affect tidb-ansible?

Refer to a related PR or issue link (optional)

#5713

Benchmark result if necessary (optional)

Add a benchmark with 1024 rows and 1024 constant args.

before optimization

test rpn_expr::impl_compare::tests::bench_compare_in                                      ... bench:   3,264,001 ns/iter (+/- 310,432)

after optimization

test rpn_expr::impl_compare::tests::bench_compare_in                                      ... bench:      21,425 ns/iter (+/- 3,039)

Any examples? (optional)

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 21, 2019

Thanks for your pull request. Pick up issue #5713 and reopen this PR

@sre-bot sre-bot closed this Nov 21, 2019
@you06 you06 reopened this Nov 21, 2019
@TennyZhuang TennyZhuang force-pushed the TennyZhuang:in-improve branch 2 times, most recently from 06cd21b to 696afe1 Nov 22, 2019
@TennyZhuang

This comment has been minimized.

Copy link
Contributor Author

TennyZhuang commented Nov 22, 2019

@TennyZhuang

This comment has been minimized.

Copy link
Contributor Author

TennyZhuang commented Nov 22, 2019

@sre-bot /test

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 22, 2019

/test

@TennyZhuang

This comment has been minimized.

Copy link
Contributor Author

TennyZhuang commented Nov 22, 2019

@sre-bot /dco

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 22, 2019

/dco

@TennyZhuang

This comment has been minimized.

Copy link
Contributor Author

TennyZhuang commented Nov 22, 2019

@sre-bot /复读机

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 22, 2019

/复读机

@TennyZhuang

This comment has been minimized.

Copy link
Contributor Author

TennyZhuang commented Nov 22, 2019

@sre-bot /merge

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 22, 2019

@TennyZhuang No command or invalid command

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 22, 2019

The dco server is not under pingcap's control, you may add a new commit to retrigger it.

@TennyZhuang

This comment has been minimized.

Copy link
Contributor Author

TennyZhuang commented Nov 24, 2019

I have updated the benchmark in the description, it seems that a huge improvement for some cases.

PTAL, @lonng @breeswish

@TennyZhuang TennyZhuang force-pushed the TennyZhuang:in-improve branch 2 times, most recently from 2a88707 to f842a33 Nov 25, 2019
Signed-off-by: TennyZhuang <zty0826@gmail.com>
@TennyZhuang TennyZhuang force-pushed the TennyZhuang:in-improve branch from f842a33 to 1c49435 Nov 26, 2019
@winkyao

This comment has been minimized.

Copy link
Contributor

winkyao commented Nov 27, 2019

@TennyZhuang Could you post the benchmark result in the cases that have huge improvements.

@TennyZhuang

This comment has been minimized.

Copy link
Contributor Author

TennyZhuang commented Nov 27, 2019

@winkyao i have already post the result in PR description.

Copy link
Member

breeswish left a comment

The rest LGTM

Signed-off-by: TennyZhuang <zty0826@gmail.com>
TennyZhuang added 3 commits Nov 27, 2019
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
@TennyZhuang TennyZhuang force-pushed the TennyZhuang:in-improve branch from e5a6b6a to a8752ba Nov 27, 2019
fix
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Copy link
Contributor

sticnarf left a comment

The rest implementation lgtm. Nice work.

Additionally, could you add a unit test for compare_in_by_compare? This function is not covered.

TennyZhuang added 2 commits Nov 27, 2019
Signed-off-by: TennyZhuang <zty0826@gmail.com>
typo
Signed-off-by: TennyZhuang <zty0826@gmail.com>
@TennyZhuang

This comment has been minimized.

Copy link
Contributor Author

TennyZhuang commented Nov 27, 2019

@sticnarf construct some Json case is complicated in test, how about add a new fn called

fn compare_in_by_compare_internal<T: InByCompare>(base_val: T, args: &[&Option<T>], default_ret: Option<Int>) -> Result<Option<Int>>)

then compare_in_by_hash will also call compare_in_by_compare_internal, so that compare_in_by_compare_internal is covered, and compare_in_by_compare will be simple enough so that we don't need to test it.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Nov 27, 2019

How about simply construct Json expr nodes? Then your compare_in_by_compare can be covered.

Signed-off-by: TennyZhuang <zty0826@gmail.com>
@TennyZhuang

This comment has been minimized.

Copy link
Contributor Author

TennyZhuang commented Nov 27, 2019

@breeswish @sticnarf PTAL at the latest commit, I add a fn_mapper to test compare_in_by_compare

@TennyZhuang

This comment has been minimized.

Copy link
Contributor Author

TennyZhuang commented Nov 27, 2019

@sre-bot /test

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 27, 2019

/test

Copy link
Contributor

sticnarf left a comment

LGTM

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Nov 27, 2019

/merge

@sre-bot sre-bot added the S: CanMerge label Nov 27, 2019
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 27, 2019

/run-all-tests

@sre-bot sre-bot merged commit 30e8d53 into tikv:master Nov 27, 2019
6 checks passed
6 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-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 Nov 27, 2019

Team .* complete task #5713 and get 2100 score, currerent score 4150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.