Conversation
📝 WalkthroughWalkthroughThe changes update the hardware counter system by introducing new components and extending existing ones. A new structure, ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/common/common/src/counter/conditioned_counter.rs (1)
1-114: Consider adding multi-thread safety documentation.The code appears to handle synchronization through the use of atomic operations in the underlying counters, but it would be helpful to explicitly document whether
ConditionedCounteris thread-safe and any constraints on its usage in concurrent contexts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/common/common/src/counter/conditioned_counter.rs(1 hunks)lib/common/common/src/counter/hardware_accumulator.rs(1 hunks)lib/common/common/src/counter/hardware_counter.rs(1 hunks)lib/common/common/src/counter/hardware_data.rs(2 hunks)lib/common/common/src/counter/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
lib/common/common/src/counter/hardware_accumulator.rs (1)
lib/common/common/src/counter/hardware_counter.rs (6)
cpu_counter(123-125)payload_io_read_counter(128-130)payload_io_write_counter(143-145)vector_io_write_counter(153-155)payload_index_io_read_counter(133-135)payload_index_io_write_counter(138-140)
lib/common/common/src/counter/conditioned_counter.rs (2)
lib/common/common/src/counter/hardware_counter.rs (4)
new(42-55)disposable(60-73)from(202-206)cpu_counter(123-125)lib/common/common/src/counter/hardware_accumulator.rs (4)
new(111-117)disposable(122-128)get_cpu(22-24)get_cpu(161-163)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-low-resources
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test-consistency
- GitHub Check: test (macos-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: test (windows-latest)
- GitHub Check: test-consensus
- GitHub Check: test
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (13)
lib/common/common/src/counter/mod.rs (1)
1-1: LGTM: New module added for the ConditionedCounterThe counter module has been properly updated to export the new
conditioned_countermodule, making the new functionality accessible to other parts of the codebase.lib/common/common/src/counter/hardware_counter.rs (1)
201-207: LGTM: Efficient implementation of From trait for HardwareDataThe implementation correctly combines hardware counter data from both the counter itself and its accumulator, utilizing the new
hw_data()method. This simplifies conversion of counter data to a consolidatedHardwareDataobject.lib/common/common/src/counter/hardware_data.rs (2)
1-2: LGTM: Add the std::ops::Add importCorrectly imported the Add trait to implement field-wise addition for HardwareData.
15-29: LGTM: Well-implemented Add trait for HardwareDataThe implementation correctly performs field-wise addition of all hardware counters, which will be useful for combining data from multiple sources. This supports the new functionality needed by the ConditionedCounter.
lib/common/common/src/counter/hardware_accumulator.rs (1)
189-209: LGTM: Clean implementation of hw_data methodThe
hw_data()method efficiently retrieves all counter values from the request_drain in a single operation, returning a consolidatedHardwareDatastructure. This reduces code duplication compared to calling individual getters for each counter type.lib/common/common/src/counter/conditioned_counter.rs (8)
1-14: Structure and documentation look good.The struct is well-documented with clear comments explaining its purpose and usage. The inclusion of lifetime parameter
'ais appropriate for storing a reference to the parent counter.
16-24: Implementation is concise and correct.The constructor initializes the struct with a condition flag, parent reference, and a disposable temporary counter. The use of
HardwareCounterCell::disposable()is appropriate here as it creates a temporary counter that doesn't persist measurements on its own.
26-32: Deref implementation enables transparent usage.Good implementation of
Derefthat allowsConditionedCounterto be used where aHardwareCounterCellis expected. This makes the wrapper non-intrusive to existing code.
34-42: Drop implementation correctly handles conditional accumulation.The
Dropimplementation performs accumulation only when the condition is true, effectively implementing the conditional counting behavior. This ensures measurements are only transferred to the parent counter when needed.
44-61: Test cases verify non-accumulation when condition is false.This test correctly verifies that when the condition is false, no values are accumulated in the parent counter or its accumulator, which is the expected behavior.
63-75: Test cases verify accumulation when condition is true.This test correctly verifies that when the condition is true, values are properly accumulated in the parent's accumulator, not directly in the parent's counter cells. The comment on line 73 is helpful in clarifying this distinction.
77-94: Test verifies more complex indirect counting scenario.Good test case that verifies a more complex usage pattern of creating new accumulators and counter cells from the conditioned counter. This helps ensure the wrapper works correctly in real-world usage scenarios.
96-114: Test validates integration with HwMeasurementAcc.This final test confirms that the
ConditionedCounterworks correctly when its parent is obtained from aHwMeasurementAcc, ensuring broader compatibility across the measurement system.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new wrapper, ConditionedCounter, for HardwareCounterCell to enable conditioned accumulation of hardware counter values when counting must be optionally disabled. Key changes include:
- Adding a new module (conditioned_counter) to the counter module.
- Defining an Add implementation for HardwareData to support accumulation.
- Implementing ConditionedCounter with condition-based accumulation and associated tests.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/common/common/src/counter/mod.rs | Adds the conditioned_counter module entry to the counter module. |
| lib/common/common/src/counter/hardware_data.rs | Implements the Add trait for HardwareData to enable summing of hardware metrics. |
| lib/common/common/src/counter/hardware_counter.rs | Adds a conversion from HardwareCounterCell to HardwareData for accumulating counter values. |
| lib/common/common/src/counter/hardware_accumulator.rs | Adds a method to extract HardwareData from the hardware accumulator counters. |
| lib/common/common/src/counter/conditioned_counter.rs | Implements the ConditionedCounter wrapper with conditional accumulation and provides tests for various scenarios. |
Comments suppressed due to low confidence (1)
lib/common/common/src/counter/conditioned_counter.rs:13
- [nitpick] Consider renaming 'tmp' to a more descriptive name such as 'disposable_counter' to improve code clarity.
tmp: HardwareCounterCell,
Adds a new wrapper for
HwCounterCellthat allows conditioned counting.This is needed in places where we can't use a multiplier of 0 to disable counting, like mmap bool index.
An example of how this can be used can be found in #6296.