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

LAC::consumeResource may introduce performance regression #8270

Closed
guo-shaoge opened this issue Oct 30, 2023 · 5 comments · Fixed by #8271
Closed

LAC::consumeResource may introduce performance regression #8270

guo-shaoge opened this issue Oct 30, 2023 · 5 comments · Fixed by #8271
Assignees
Labels
affects-7.5 This bug affects the 7.5.x(LTS) versions. component/compute severity/critical type/bug The issue is confirmed as a bug.

Comments

@guo-shaoge
Copy link
Contributor

Bug Report

Please answer these questions before submitting your issue. Thanks!

https://github.com/pingcap/tiflash/blob/master/dbms/src/Storages/DeltaMerge/SkippableBlockInputStream.h#L224 and https://github.com/pingcap/tiflash/blob/master/dbms/src/Storages/DeltaMerge/ColumnFile/ColumnFileSetReader.cpp#L194 will consume resource for storage layer, but there are situations that the calling of LAC::consumeResource are too frequent, which may introduce significant regression.

1. Minimal reproduce step (Required)

2. What did you expect to see? (Required)

3. What did you see instead (Required)

4. What is your TiFlash version? (Required)

@Yui-Song
Copy link

As the issue causes a 17%- 31% performance regression in CH benchmark, which will block the release of v7.5.0. Adjust its severity to critical.
/remove-severity major
/severity critical

@Yui-Song
Copy link

/remove-label may-affects-5.3
/remove-label may-affects-5.4
/remove-label may-affects-6.1
/remove-label may-affects-6.5
/remove-label may-affects-7.1

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Oct 30, 2023

@Yui-Song: The label(s) may-affects-5.3, may-affects-5.4, may-affects-6.1, may-affects-6.5, may-affects-7.1 cannot be applied. These labels are supported: tide/merge-method-rebase, tide/merge-method-squash, tide/merge-method-merge, duplicate, first-time-contributor, good first issue, ok-to-test, needs-ok-to-test, help wanted, invalid, question, wontfix, make-local-great-again, needs-cherry-pick-release-5.3, needs-cherry-pick-release-5.4, needs-cherry-pick-release-6.1, needs-cherry-pick-release-6.5, needs-cherry-pick-release-7.1, needs-cherry-pick-release-7.5, needs-rebase.

In response to this:

/remove-label may-affects-5.3
/remove-label may-affects-5.4
/remove-label may-affects-6.1
/remove-label may-affects-6.5
/remove-label may-affects-7.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@Yui-Song
Copy link

/remove-label may-affects-5.3

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Oct 30, 2023

@Yui-Song: The label(s) may-affects-5.3 cannot be applied. These labels are supported: tide/merge-method-rebase, tide/merge-method-squash, tide/merge-method-merge, duplicate, first-time-contributor, good first issue, ok-to-test, needs-ok-to-test, help wanted, invalid, question, wontfix, make-local-great-again, needs-cherry-pick-release-5.3, needs-cherry-pick-release-5.4, needs-cherry-pick-release-6.1, needs-cherry-pick-release-6.5, needs-cherry-pick-release-7.1, needs-cherry-pick-release-7.5, needs-rebase.

In response to this:

/remove-label may-affects-5.3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-7.5 This bug affects the 7.5.x(LTS) versions. component/compute severity/critical type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants