Add Benchmark COM-RPC interfaces by smanes0213 · Pull Request #492 · rdkcentral/ThunderInterfaces

smanes0213 · 2026-04-15T05:08:53Z

Add IBenchmark.h: trigger/collectdata/setthreshold APIs with nanosecond latency stats, memory stats, pass/fail thresholds, and PerformanceCheckCompleted event notification
Add IBenchmarkPayloadCOMRPC.h: payload interface for benchmarking various COM-RPC parameter types (uint32, string, structs, buffers)
Update QAIds.h: register interface IDs for IBenchmark, IBenchmarkPayload, and associated iterators

- Add IBenchmark.h: trigger/collectdata/setthreshold APIs with nanosecond latency stats, memory stats, pass/fail thresholds, and PerformanceCheckCompleted event notification - Add IBenchmarkPayloadCOMRPC.h: payload interface for benchmarking various COM-RPC parameter types (uint32, string, structs, buffers) - Update QAIds.h: register interface IDs for IBenchmark, IBenchmarkPayload, and associated iterators Signed-off-by: smanes0213 <sankalpmaneshwar46@outlook.com>

Copilot

Pull request overview

Adds new QualityAssurance COM-RPC interfaces to support benchmarking round-trip latency/memory behavior and to exercise a variety of COM-RPC parameter shapes, with corresponding QA interface ID allocations.

Changes:

Added IBenchmark interface for triggering benchmarks, collecting results, configuring thresholds, and emitting a completion event.
Added IBenchmarkPayload interface to benchmark different COM-RPC parameter types (primitives, strings, structs, buffers).
Registered new QA interface IDs for the benchmark and payload interfaces (plus iterators/notification IDs).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
`qa_interfaces/QAIds.h`	Allocates ID ranges for `IBenchmark`, notification, result iterator, and payload interfaces.
`qa_interfaces/IBenchmark.h`	Introduces benchmark control/results interface plus completion notification sink.
`qa_interfaces/IBenchmarkPayloadCOMRPC.h`	Introduces payload-sending interface to benchmark various parameter/return patterns.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

bramoosterhuis

In general:

Missing trailing newline. POSIX defines a text file as a sequence of newline-terminated lines, without it the last line is unterminated and tools that process text files can behave unexpectedly.
Missing copyright headers.

bramoosterhuis · 2026-05-13T09:04:25Z

+
+        virtual uint32_t Add(const uint32_t a, const uint32_t b, uint32_t &result /* @out */) const = 0;
+
+        ~IBenchmarkPayload() override = default;


Nitpicking, but the destructor belongs right after enum { ID = ... } , that's the consistent ordering across all interfaces in this repo.

bramoosterhuis · 2026-05-13T09:06:11Z

+        ID_BENCHMARK_RESULT_ITERATOR                 = ID_BENCHMARK + 2,
+
+        ID_BENCHMARK_PAYLOAD                         = RPC::IDS::ID_EXTERNAL_QA_INTERFACE_OFFSET + 0x050,
+        ID_PAYLOADTYPE_ITERATOR                      = ID_BENCHMARK_PAYLOAD + 1,


Should be ID_BENCHMARK_PAYLOADTYPE_ITERATOR, every other iterator ID in this file is prefixed with its group name. IDs are permanent once shipped, an ambiguous name risks a silent collision if another interface adds a similarly named iterator in the future.

bramoosterhuis · 2026-05-13T09:11:00Z

+            virtual void PerformanceCheckCompleted() = 0;
+        };
+
+        virtual Core::hresult Register(const IBenchmark::INotification* sink) = 0;


Was there a reason to make the sink const here? Convention throughout the codebase is non-const for Register/Unregister. The practical consequence is that any implementation storing the sink and calling PerformanceCheckCompleted() through it needs a const_cast, since the method is non-const.

bramoosterhuis · 2026-05-13T09:13:07Z

+        // @brief Run the benchmark
+        // @param iterations: Denotes the number of iterations the benchmark should run
+        // @param success: True if the benchmark completed successfully
+        virtual Core::hresult Trigger(const uint32_t iterations, bool& success /* @out */) = 0;


IMHO: The success out-param is redundant, Core::hresult is already the success/failure signal. Having both leaves an ambiguous state if they disagree. The existing QA interfaces express failure conditions via @retval doc comments on the return value only.

bramoosterhuis · 2026-05-13T09:22:57Z

+        // @param maxLatencyDeviationPct: Maximum allowed % deviation in avg latency compared to first-run baseline (0 = no latency check)
+        // @param maxMemoryGrowthBytes: Maximum allowed RSS growth in bytes per method (0 = no memory check)
+        // @param success: True if thresholds were set
+        virtual Core::hresult SetThreshold(const float maxLatencyDeviationPct, const uint64_t maxMemoryGrowthBytes, bool& success /* @out */) = 0;


Using float for a percentage opens the door to NaN/Infinity arriving over the wire with no way to detect or reject them, and no other interface in this repo uses float at a COM-RPC boundary so this would be the odd one out convention-wise. A uint32_t in millipercent (where 1000 = 1%) gives the same precision, keeps the value range explicit, and is trivially range-checked on the receiving side. If a negative deviation makes sense semantically, int32_t in millipercent covers that too.

bramoosterhuis · 2026-05-13T09:59:06Z

+    namespace QualityAssurance {
+
+
+    struct SampleData


SampleData is a very generic name sitting loose in the QualityAssurance namespace, where anything can see it and accidentally clash with it. Every other supporting struct in this repo is defined inside the interface class that owns it, TestDetails inside ITestTextOptions, device inside IBluetooth::IBluetoothControl etc. Move it inside IBenchmarkPayload.

bramoosterhuis · 2026-05-13T10:01:39Z

+
+        typedef RPC::IIteratorType<PayloadType, ID_PAYLOADTYPE_ITERATOR> IPayloadTypeIterator;
+
+        virtual uint32_t GetPayloadTypes(IPayloadTypeIterator*& types /* @out */) const = 0;


Was the intent to benchmark the iterator type over COM-RPC alongside the scalar and buffer types? If so that should be documented, because as written it just looks like a runtime query for information that is already visible in the method signatures.

bramoosterhuis · 2026-05-13T10:10:55Z

The filename encodes the transport mechanism, no other file in qa_interfaces/ does this (ITestAutomation.h, ITestController.h, ITestUtility.h). The name should reflect what is being tested, not how.

bramoosterhuis · 2026-05-13T10:17:46Z

+
+        virtual uint32_t SendSampleData(const SampleData& data) = 0;
+
+        virtual uint32_t SendWithNoParameters() = 0;


Nitpicking, but SendNoPayload would fit the Send* naming scheme better, the current name describes the signature rather than the operation.

Copilot AI review requested due to automatic review settings April 15, 2026 05:08

Copilot started reviewing on behalf of smanes0213 April 15, 2026 05:09 View session

smanes0213 marked this pull request as draft April 15, 2026 05:10

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Comment thread qa_interfaces/IBenchmarkPayloadCOMRPC.h Outdated

Comment thread qa_interfaces/IBenchmarkPayloadCOMRPC.h Outdated

Comment thread qa_interfaces/IBenchmark.h Outdated

Potential fix for pull request finding

e1230c2

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

smanes0213 requested review from MFransen69, bramoosterhuis and sramani-metro May 12, 2026 10:15

bramoosterhuis requested changes May 13, 2026

View reviewed changes

Resolve review comments

d6d6207

smanes0213 requested a review from bramoosterhuis May 14, 2026 04:47


		virtual uint32_t Add(const uint32_t a, const uint32_t b, uint32_t &result /* @out */) const = 0;

		~IBenchmarkPayload() override = default;


		typedef RPC::IIteratorType<PayloadType, ID_PAYLOADTYPE_ITERATOR> IPayloadTypeIterator;

		virtual uint32_t GetPayloadTypes(IPayloadTypeIterator& types / @out */) const = 0;


		virtual uint32_t SendSampleData(const SampleData& data) = 0;

		virtual uint32_t SendWithNoParameters() = 0;

Conversation

smanes0213 commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bramoosterhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants