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

WiP stats: add stats sink speed test with histogram updater to try to repro #37831 #37859

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Jan 3, 2025

Commit Message: Repro of contention bottleneck (#37831) in stats sinks with lots of threads, and a possibly remedy, implemented within a test sink.

Note that in its current form there are several issues:

  • the sample remedy has a bug -- it holds a pointer in its map to a histogram which might not survive a config update, so we need to take a reference to it. This is possible (the ref-count is internal) but is not made convenient via the API currently.
  • the remedy still has a contention bottleneck on the shared map, it's just a little better than serialzing the names under lock. But we don't need to contend; we could add another TLS object. This needs to be designed.
  • The remedy is implemented in a single sink, and should probably be shared across any sinks that require capturing all the histogram updates. This is not needed for sinks where the aggregated histogram data csn be used instead, thus such sinks should not need to pay for any new buffering. We probably need two Sink variants: one that provides per-change histogram updates, and one that only provides aggregated histogram data in the snapshot.

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #37859 was opened by jmarantz.

see: more, trace.

Signed-off-by: Joshua Marantz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant