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

chore: remove batch reply statistics #2490

Merged
merged 1 commit into from
Jan 29, 2024
Merged

chore: remove batch reply statistics #2490

merged 1 commit into from
Jan 29, 2024

Conversation

romange
Copy link
Collaborator

@romange romange commented Jan 27, 2024

  1. average batch latency will always be 0, because even in cases we have outliers they will be dominated by small CPU only copies that take dozens of ns.
  2. Measuring an operation like kBatch, which is solely CPU-based, necessitated the use of a clock. According to the CPU profiler, this contributed to approximately 5% of the CPU usage.

@romange romange requested a review from chakaz January 27, 2024 08:22
@dranikpg
Copy link
Contributor

dranikpg commented Jan 27, 2024

I don't understand why instead of unconditionally adding (obviously not zero cost) tracing and then removing it completely, we didn't introduce a proper macro for enabling or disabling all kinds of tracing during builds.

What do we do next time we want to get reply batching stats to analyze a specific scenario? 😄(🤨)

@romange
Copy link
Collaborator Author

romange commented Jan 27, 2024

Batching measures only the cpu work. It never worked as intended and that's what we are removing. We still keep the part that measures the actual socket latency

1. average batch latency will always be 0, because even in cases we have outliers they will be dominated by
   small CPU only copies that take dozens of ns.
2. Measuring an operation like kBatch, which is solely CPU-based, necessitated the use of a clock. According to the CPU profiler,
   this contributed to approximately 5% of the CPU usage.

Signed-off-by: Roman Gershman <roman@dragonflydb.io>
@romange romange merged commit d7604c1 into main Jan 29, 2024
10 checks passed
@romange romange deleted the RemoveReplyStat branch January 29, 2024 01:23
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.

None yet

3 participants