Skip to content

Use shared mutex for global stacktrace cache#62453

Merged
serxa merged 2 commits intomasterfrom
lower-contention-on-stacktrace-cache
Apr 10, 2024
Merged

Use shared mutex for global stacktrace cache#62453
serxa merged 2 commits intomasterfrom
lower-contention-on-stacktrace-cache

Conversation

@serxa
Copy link
Copy Markdown
Member

@serxa serxa commented Apr 9, 2024 •
edited
Loading

Avoid locking stacktrace_cache_mutex exclusively as much as possible to lower contention even further. Follow up for #62332

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Modify your CI run:

NOTE: If your merge the PR with modified CI you MUST KNOW what you are doing
NOTE: Checked options will be applied if set before CI RunConfig/PrepareRunConfig step

Include tests (required builds will be added automatically):

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Unit tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with Analyzer
  • Add your option here

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All with Aarch64
  • Add your option here

Extra options:

  • do not test (only style check)
  • disable merge-commit (no merge from master before tests)
  • disable CI cache (job reuse)

Only specified batches in multi-batch jobs:

  • 1
  • 2
  • 3
  • 4

@clickhouse-ci
Copy link
Copy Markdown

clickhouse-ci bot commented Apr 9, 2024

This is an automatic comment. The PR descriptions does not match the template.

Please, edit it accordingly.

The error is: Changelog entry required for category 'Improvement'

@clickhouse-ci
Copy link
Copy Markdown

clickhouse-ci bot commented Apr 9, 2024

This is an automatic comment. The PR descriptions does not match the template.

Please, edit it accordingly.

The error is: Category 'Not for change log' is not valid

@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Apr 9, 2024
@robot-ch-test-poll4
Copy link
Copy Markdown
Contributor

robot-ch-test-poll4 commented Apr 9, 2024 •
edited by robot-clickhouse-ci-1
Loading

This is an automated comment for commit 5d576bb with description of existing statuses. It's updated for the latest CI running

⏳ Click here to open a full report in a separate page

Check nameDescriptionStatus
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR⏳ pending
Successful checks
Check nameDescriptionStatus
Fast testNormally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here✅ success
Mergeable CheckChecks if all other necessary checks are successful✅ success
PR CheckThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS✅ success
Style checkRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success

@nickitat nickitat self-assigned this Apr 9, 2024
nickitat
nickitat approved these changes Apr 10, 2024
View reviewed changes
src/Common/StackTrace.cpp
/// Calculation of stack trace text is extremely slow.
/// We use simple cache because otherwise the server could be overloaded by trash queries.
/// We use cache because otherwise the server could be overloaded by trash queries.
/// Note that this cache can grow unconditionally, but practically it should be small.
Copy link
Copy Markdown
Member

@nickitat nickitat Apr 10, 2024

Choose a reason for hiding this comment

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

irrelevant to the current PR.
though I see no benefit from caching whole stack as a single entry instead of storing each frame separately.

Copy link
Copy Markdown
Member Author

@serxa serxa Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I was also thinking about it. We can lower memory consumption by a factor equal to the stack average depth. And a bit less work for the frames appearing in many stacktraces. Ideally we need to move our cache to StackTrace::forEachFrame/toStringEveryLineImpl function. Though, it will require some effort.

@serxa serxa enabled auto-merge April 10, 2024 12:01
@serxa serxa added this pull request to the merge queue Apr 10, 2024
Merged via the queue into master with commit 027c8a8 Apr 10, 2024
@serxa serxa deleted the lower-contention-on-stacktrace-cache branch April 10, 2024 12:24
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 10, 2024
@alexey-milovidov alexey-milovidov mentioned this pull request Jul 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickitat nickitat nickitat approved these changes

Assignees

@nickitat nickitat

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@serxa @robot-ch-test-poll4 @nickitat @robot-clickhouse-ci-2