Skip to content

Move perf tests for Aarch64 from PRs to master#43623

Merged
tavplubix merged 6 commits intomasterfrom
move_perf_tests_aarch_to_master
Nov 28, 2022
Merged

Move perf tests for Aarch64 from PRs to master#43623
tavplubix merged 6 commits intomasterfrom
move_perf_tests_aarch_to_master

Conversation

@tavplubix
Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@alexey-milovidov alexey-milovidov self-assigned this Nov 24, 2022
@nickitat
Copy link
Copy Markdown
Member

  • what is the motivation? 4 checks is nothing compared to ~200 in total, also perf tests are not the longest checks: they are faster than tidy and take roughly the same time as integration tests and stress tests.
  • is there a way to request arm perf tests for the specific pr?

@tavplubix
Copy link
Copy Markdown
Member Author

what is the motivation? 4 checks is nothing compared to ~200 in total, also perf tests are not the longest checks: they are faster than tidy and take roughly the same time as integration tests and stress tests.

AFAIK we use quite powerful (and therefore quite expensive) machines for perf tests. Also seems like it just does not make sense to run perf tests on Aarch64 for each commit in each PR. Perf tests on x86 should be enough to catch performance degradation in 99% of PRs (except some architecture-specific changes). Also almost nobody looks at flaky performance tests, especially on Aarch64.

~200 in total

~110 actually, because most checks in the list are duplicated

they are faster than tidy

It's another major issue. We still have to investigate why clang-tidy build takes more than 5 (FIVE!!!) hours sometimes: https://s3.amazonaws.com/clickhouse-builds/43442/d9975c9d13fd8215706226912dd6bf1c37fd4f77/clickhouse_special_build_check/report.html
It may be related to extra checks/warnings that were enabled recently, cc: @rschu1ze

is there a way to request arm perf tests for the specific pr?

Unfortunately, no. Another issue is that we always run only some sample of perf tests. We should run all perf tests (including tests on Aarch64) for PRs labeled as "performance improvement". I'll convert this PR to draft for now.

@tavplubix tavplubix marked this pull request as draft November 24, 2022 16:08
@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Nov 25, 2022
@tavplubix tavplubix marked this pull request as ready for review November 26, 2022 16:43
@tavplubix
Copy link
Copy Markdown
Member Author

is there a way to request arm perf tests for the specific pr?

Now there is: arm perf tests will start if pr has pr-performance label

@tavplubix tavplubix merged commit a82fec8 into master Nov 28, 2022
@tavplubix tavplubix deleted the move_perf_tests_aarch_to_master branch November 28, 2022 14:56
@rschu1ze
Copy link
Copy Markdown
Member

About clang-tidy performance: Yes, it's terribly slow in some builds. No new checks were added recently afaik. In my experience, some checks can be much more expensive than others. Unfortunately, there is no good way of finding these except with "bisecting". I am currently doing that locally ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants