Latency

[DouglasAdler]

No description provided.

[DouglasAdler]

Add latency to rdkperf for tracking an event across processes.

[Copilot]

Pull request overview

This PR introduces a new cross-scope “latency sequence” measurement capability (recording ordered locations and reporting aggregated timings), backed by shared memory, and adds perf-test / unit-test hooks to exercise it.

Changes:

• Added latency tracking infrastructure: shared-memory block, sequence/location model, and a circular buffer for timestamps.
• Added a new public C API for latency (RDKLatency*) and a perf test scenario that exercises nested calls + fork.
• Updated a few existing components (msgqueue name buffer sizing, logging env var handling, timer shutdown behavior, minor test tweaks).

Reviewed changes

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

Show a summary per file

File	Description
test/unit_tests.cpp	Adds new perf timing subtask + node timing loops; adjusts threshold units.
test/perftest.cpp	Adds latency test sequence (incl. fork/wait) and enables latency tests via compile-time flag.
src/rdk_perf_shm_block.h	Introduces shared-memory block header/metadata definition and API.
src/rdk_perf_shm_block.cpp	Implements shared-memory creation/attach, mapping, and semaphore locking.
src/rdk_perf_sequence.h	Declares the shared-memory-backed sequence manager API.
src/rdk_perf_sequence.cpp	Implements sequence creation/lookup, location chaining, and recording.
src/rdk_perf_location.h	Declares PerfLocation and exposes timestamp/record APIs.
src/rdk_perf_location.cpp	Implements timestamp storage, aggregation, parent/child diff calculation.
src/rdk_perf_latency_data.h	Adds shared-memory data structures for sequences/locations/circular buffers.
src/rdk_perf_circularbuffer.h	Declares a circular buffer used for timestamp storage.
src/rdk_perf_circularbuffer.cpp	Implements circular buffer operations and logging.
src/rdk_perf_msgqueue.h	Renames message name length macro to MAX_MSG_NAME_LEN.
src/rdk_perf_msgqueue.cpp	Updates buffer-size usage in message send paths.
src/rdk_perf_logging.cpp	Changes env var name for verbose logging and adds debug prints.
src/rdk_perf_clock.cpp	Suppresses a log line when getrusage returns zeros.
rdkperf/rdk_perf.h	Exposes latency API through the main public header.
rdkperf/rdk_perf.cpp	Adjusts shutdown order for the timer thread/task and disables exit reporting.
rdkperf/rdk_perf_latency.h	Adds the public C latency API header.
rdkperf/rdk_latency.cpp	Implements RDKLatency* calls using PerfSequence.
rdkperf/rdk_mem_tracker.h	Adds a memory tracker public header.
rdkperf/rdk_mem_tracker.cpp	Adds memory tracker implementation + C wrapper functions.
rdkperf/Makefile	Adds an $ORIGIN rpath for runtime library resolution.
.github/workflows/cla.yml	Adds a CLA workflow using rdkcentral/cmf-actions.

---

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

[Copilot]

Pull request overview

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

---

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

[Copilot]

SharedMemoryBlock is managed as a process-wide singleton (_shared_memory_block), but the destructor does not reset _shared_memory_block to nullptr. If any code deletes the singleton (e.g., via a owning class destructor) and later calls get_instance(), it will return a dangling pointer. Consider either (a) making get_instance() return a non-owning pointer that is never deleted, or (b) adding explicit lifetime management + nulling the static on destruction.

[Copilot]

CircBufferObject::records is declared as records[sizeof(CircBufferRecord) * MAX_TIME_STAMPS], which makes the array length a byte-count multiple (e.g., 400 elements) instead of MAX_TIME_STAMPS elements. This inflates CircBufferObject (and therefore Sequence/Location) unexpectedly and breaks assumptions in code that treats maxRecords as the array capacity. It should be sized as records[MAX_TIME_STAMPS].

[Copilot]

CircBufferObject includes char* name, and Location/Sequence structs (and their circular buffers) are stored in shared memory. Persisting a process-local pointer in shared memory is unsafe across processes and will become invalid when another process maps the segment at a different address. Consider removing the pointer from the shared-memory layout (or storing a fixed-size name or an offset within the SHM region).

[Copilot]

Location stores Sequence* sequence, but Location/Sequence are placed in shared memory. A raw pointer to a SHM-mapped object is not stable across processes (different mapping base addresses), so sequence will be invalid when read from another process. Consider storing a sequence index/offset instead of a pointer, and resolve it relative to the mapped base in each process.

[Copilot]

strncpy(..., MAX_NAME_LEN) does not guarantee NUL-termination when the input is length >= MAX_NAME_LEN. Since these names are later treated as C strings (e.g., logged/compared), this can lead to unterminated strings. Consider using snprintf or explicitly setting name[MAX_NAME_LEN-1] = '\0' after strncpy.

Suggested change

	strncpy(_sequences[i].name, sequenceName, MAX_NAME_LEN);
	strncpy(_sequences[i].name, sequenceName, MAX_NAME_LEN - 1);
	_sequences[i].name[MAX_NAME_LEN - 1] = '\0';

[Copilot]

Pull request overview

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

---

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

[Copilot]

CircBufferObject::records is declared as CircBufferRecord records[sizeof(CircBufferRecord) * MAX_TIME_STAMPS], which allocates (sizeof(record)*MAX) records rather than MAX records. This mismatches how the buffer is indexed (0..maxRecords-1) and inflates the shared-memory layout. It should likely be records[MAX_TIME_STAMPS] (or change the type to a byte array if you intended raw storage).

Suggested change

	CircBufferRecord records[sizeof(CircBufferRecord) * MAX_TIME_STAMPS];
	CircBufferRecord records[MAX_TIME_STAMPS];

[Copilot]

RemoveSequence() can form invalid pointers when location_offset / child_offset is INVALID_OFFSET (-1): _locations + (-1) is UB. Add INVALID_OFFSET guards before computing location and before stepping to next.

Suggested change

	Location* location = _locations + _sequences[i].location_offset;
	while(location != nullptr) {
	Location* next = _locations + location->child_offset;
	LOG(eTrace, "Removing location %s", location->name);
	location->name[0] = '\0';
	location = next;
	auto location_offset = _sequences[i].location_offset;
	while(location_offset != INVALID_OFFSET) {
	Location* location = _locations + location_offset;
	auto next_offset = location->child_offset;
	LOG(eTrace, "Removing location %s", location->name);
	location->name[0] = '\0';
	location_offset = next_offset;

[Copilot]

This error log runs unconditionally, even when the sequence was successfully removed, which will mislead operators and tests. Only log this message when retVal is false (and fix the typo "ro remove" -> "to remove").

Suggested change

	LOG(eError, "Failed to find sequence %s ro remove\n", sequenceName);
	if(!retVal) {
	LOG(eError, "Failed to find sequence %s to remove\n", sequenceName);
	}

[Copilot]

When creating a new location, the circular buffer backing storage (timeStampCirBuffer) isn’t reset, and the CircularBuffer constructor currently does not clear head/tail/currentSize/records. This can leave new locations with garbage circular-buffer state. Ensure new buffers are zeroed and initialized (use CircularBuffer::initialize() for new allocations).

[Copilot]

GetLocationsDepth() does not guard against _current_sequence->location_offset == INVALID_OFFSET, so it can compute _locations + (-1) and dereference it. Return 0 early when location_offset is invalid.

[Copilot]

GetDataRecord() constructs PerfLocation from _current_sequence->location_offset without checking INVALID_OFFSET, which can dereference out-of-bounds memory for a sequence with no locations. Guard and return nullptr (or an empty record) when location_offset is invalid.

Suggested change

	if(_current_sequence->location_offset == INVALID_OFFSET) {
	LOG(eError, "No valid location to get data record\n");
	return nullptr;
	}

[Copilot]

RDKLatencyReport() prints a heap-allocated DataRecord chain returned by GetDataRecord() but never frees it (and advances pData so the original pointer is lost). This leaks on every report. Keep the root pointer and delete/free the chain after printing (or return an owning smart pointer/value type).

[Copilot]

Several log statements use [%s] with circBuffer->name (e.g., in the FULL-buffer log). If name hasn’t been set (or is an invalid SHM pointer), passing it to %s is undefined and can crash. Guard against null/invalid names (or avoid storing pointers in SHM).

[Copilot]

rootDiff is a uint64_t, but it’s logged with %lf (double) in LOG(eTrace, "Root timestamp ... root diff %lf"), which is undefined behavior. Use the correct integer format (or cast to double if that’s the intent).

[Copilot]

Pull request overview

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

---

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

[Copilot]

RemoveTimeStamp(count) calls _timeStamps.pop(...), but pop() removes the oldest (tail) entry and does not remove a specific key. This breaks the logic in AddTimeStamp() that tries to remove the matching parent timestamp for the same count. Add an erase-by-key API (or change storage) so the timestamp for the requested count is removed deterministically.

Suggested change

	uint64_t timeStamp = 0;
	return _timeStamps.pop(count, timeStamp);
	CircBufferObject tempBufferMemory = {};
	CircularBuffer retainedTimeStamps(&tempBufferMemory, MAX_TIME_STAMPS);
	bool removed = false;
	uint32_t poppedCount = 0;
	uint64_t timeStamp = 0;
	while(_timeStamps.pop(poppedCount, timeStamp)) {
	if(!removed && poppedCount == count) {
	removed = true;
	LOG(eTrace, "Removed timestamp %lu for count %u\n", timeStamp, count);
	continue;
	}
	if(!retainedTimeStamps.push(poppedCount, timeStamp)) {
	LOG(eError, "Failed to retain timestamp %lu for count %u while removing count %u\n", timeStamp, poppedCount, count);
	return false;
	}
	}
	while(retainedTimeStamps.pop(poppedCount, timeStamp)) {
	if(!_timeStamps.push(poppedCount, timeStamp)) {
	LOG(eError, "Failed to restore timestamp %lu for count %u after removing count %u\n", timeStamp, poppedCount, count);
	return false;
	}
	}
	if(!removed) {
	LOG(eError, "Failed to find timestamp for count %u\n", count);
	}
	return removed;

[Copilot]

These functions are declared static with constructor/destructor attributes, but the definitions are non-static. That’s a linkage mismatch and will typically fail to compile (“static declaration follows non-static” / multiple linkage). Make the definitions static to match the forward declarations (or drop static from both).

[Copilot]

The attached_intances decrement and == 0 decision are protected by the semaphore, but the actual cleanup (sem_destroy() / shm_unlink()) happens after unlocking. Another process can attach/detach in between, leading to premature destruction/unlink. Keep the lock held across decrement+check+final cleanup (or use a more robust refcounting strategy).

Suggested change

	// Decrement the number of attached instances
	lock();
	_shared_block->attached_intances--;
	if(_shared_block->attached_intances == 0) {
	bDelete = true;
	}
	unlock();
	if(bDelete) {
	// Destroy the semaphore
	sem_destroy(&_shared_block->semaphore);
	// Last process in the shared memory segment
	// Unlink the shared memory segment
	shm_unlink(SHARED_MEMORY_NAME);
	_shared_memory_block = nullptr;
	// Decrement the number of attached instances and, if this is the
	// last attached process, perform final cleanup while still holding
	// the semaphore so no other process can attach in between.
	lock();
	_shared_block->attached_intances--;
	if(_shared_block->attached_intances == 0) {
	bDelete = true;
	// Destroy the semaphore before unmapping the shared memory.
	// Do not unlock after this, because the semaphore is no longer valid.
	sem_destroy(&_shared_block->semaphore);
	// Last process in the shared memory segment
	// Unlink the shared memory segment
	shm_unlink(SHARED_MEMORY_NAME);
	_shared_memory_block = nullptr;
	} else {
	unlock();

[Copilot]

The wait loop uses static int nWaitCount = 0; inside SharedMemoryBlock::initialize(). Because it’s static, a previous initialization attempt can leave nWaitCount high and cause subsequent attempts to fail immediately. Make this a normal local counter (and consider logging less aggressively inside the spin loop to avoid flooding logs).

[Copilot]

In the eThreshold and eExit cases, the code writes the name into msg.msg_data.entry.szName, but the active union members are threshold and exit. This will populate the wrong struct and can corrupt the message contents. Write into msg.msg_data.threshold.szName / msg.msg_data.exit.szName respectively.

[Copilot]

LogModuleInit() unconditionally prints [DEBUG] ... messages to stdout during library initialization. This is noisy for consumers and bypasses the project’s logging controls. Prefer using the existing LOG(...) macro (respecting verbosity) or guard these prints behind the env var / a compile-time debug flag.

[Copilot]

SharedMemoryBlock::get_instance() always returns the singleton even if SharedMemoryBlock::initialize() failed (constructor sets _initialized=false but callers aren’t prevented from using _shared_block==nullptr). This can lead to null dereferences in lock()/get_data() downstream. Consider making get_instance() return nullptr on initialization failure (and/or delete the partially-constructed singleton).

[Copilot]

Pull request overview

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

---

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

[Copilot]

In the eExit case the name is copied into msg.msg_data.entry.szName rather than msg.msg_data.exit.szName. That leaves exit.szName empty and overwrites unrelated union storage. Copy into the exit member for exit messages.

[Copilot]

GetChildName() treats child_offset == 0 as “no child”, but offsets use INVALID_OFFSET (-1) as the sentinel elsewhere. A valid first child at index 0 would be incorrectly hidden. Check for INVALID_OFFSET instead of 0 (and consider returning an empty string rather than a newline).

[Copilot]

GetChildName() computes the child pointer using byte arithmetic ((uint8_t*)_locations + child_offset), but child_offset is used as an index everywhere else (e.g., it’s assigned i in PerfSequence::AddLocation). This will point at the wrong address. Use _locations + child_offset (or multiply by sizeof(Location) if the offset is intended to be bytes).

[Copilot]

GetDataRecord() divides by _location->count when computing average and average_elapsed. When count == 0 this is a divide-by-zero (NaN/inf) and can propagate into reporting. Guard the division (e.g., return 0 averages when count is 0).

Suggested change

	// Copy the total time
	retVal->average = (double)(_location->total / (double)(_location->count));
	// Copy the total elapsed time
	retVal->average_elapsed = (double)(_location->total_elapsed / (double)(_location->count));
	// Copy the min time
	retVal->min = (double)_location->min;
	// Copy the max time
	retVal->max = (double)_location->max;
	// Copy the count
	retVal->count = _location->count;
	// Copy the count
	retVal->count = _location->count;
	// Copy the total time
	if(retVal->count > 0) {
	retVal->average = (double)(_location->total / (double)(retVal->count));
	// Copy the total elapsed time
	retVal->average_elapsed = (double)(_location->total_elapsed / (double)(retVal->count));
	} else {
	retVal->average = 0.0;
	retVal->average_elapsed = 0.0;
	}
	// Copy the min time
	retVal->min = (double)_location->min;
	// Copy the max time
	retVal->max = (double)_location->max;

[Copilot]

PerfSequence::~PerfSequence() does not delete _current_location, which is allocated with new in SetCurrentLocation(...). This leaks per-instance memory (and may leak repeatedly if sequences are recreated). Ensure _current_location is deleted (or convert it to a value type / smart pointer).

Suggested change

	// Clean up the shared memory block
	if(_shared_memory_block != nullptr) {
	// `_shared_memory_block` is obtained via SharedMemoryBlock::get_instance(),
	// so PerfSequence does not own it and must not delete it here.
	// Treat the shared memory block as process-lifetime from this file.
	_shared_memory_block = nullptr;
	if(_current_location != nullptr) {
	delete _current_location;
	_current_location = nullptr;
	}
	// Clean up the shared memory block
	if(_shared_memory_block != nullptr) {
	// `_shared_memory_block` is obtained via SharedMemoryBlock::get_instance(),
	// so PerfSequence does not own it and must not delete it here.
	// Treat the shared memory block as process-lifetime from this file.
	_shared_memory_block = nullptr;

[Copilot]

RDKLatencyReport() walks a DataRecord chain returned by PerfSequence::GetDataRecord(), but that chain is allocated with new (recursively) and is never freed. Repeated reporting will leak memory. Add an explicit cleanup/free routine for the DataRecord tree (or return an owning smart pointer / value type) after printing.

[Copilot]

The latency test loop runs 499 iterations and each iteration can fork() (via test_latency_2). This can make the test binary very slow and resource-intensive by default. Consider lowering the default iterations / removing the fork from the hot loop, and making the iteration count configurable via argv/env.

[Copilot]

CircularBuffer::push/pop/peek/find emit eWarning logs for normal control-flow states like full/empty/not-found (e.g., buffer full in push() will happen routinely once more than MAX_TIME_STAMPS are recorded, especially for root timestamps that are never popped). Since warnings are not gated by s_VerboseLog, this can flood logs and significantly slow down recording. Consider downgrading these to eTrace or gating them behind the extended-logging flag.

[Copilot]

PerfSequence logs the shared-memory addresses using LOG(eError, ...) during normal construction. eError messages are always emitted and may alarm users / spam logs even in healthy runs. Consider lowering these to eTrace/eWarning (or removing) unless this indicates an actual error condition.

Suggested change

	LOG(eError, "Sequence array at %p\n", _sequences);
	// Get the pointer to the location array
	_locations = (Location*) ((uint8_t*) _sequences + sizeof(Sequence) * MAX_SEQUENCE);
	LOG(eError, "Location array at %p\n", _locations);
	LOG(eTrace, "Sequence array at %p\n", _sequences);
	// Get the pointer to the location array
	_locations = (Location*) ((uint8_t*) _sequences + sizeof(Sequence) * MAX_SEQUENCE);
	LOG(eTrace, "Location array at %p\n", _locations);

[Copilot]

When shm_open(..., O_CREAT | O_EXCL, ...) fails, the code logs ERR_LOG("shm already created") at error level before checking errno. For the expected EEXIST case this will produce an error log on every non-first attach. Consider only logging at eTrace/eWarning for EEXIST, and reserve eError for unexpected errno values.

Suggested change

	// Create the shared memory segment exclusively. If it already exists, return an error
	_shm_fd = shm_open(SHARED_MEMORY_NAME, O_CREAT | O_EXCL | O_RDWR, 0666);
	if (_shm_fd == -1) {
	ERR_LOG("shm already created");
	// Try to open the existing shared memory segment
	_shm_fd = shm_open(SHARED_MEMORY_NAME, O_RDWR, 0666);
	if (_shm_fd == -1) {
	// Create the shared memory segment exclusively. If it already exists, open the existing segment.
	_shm_fd = shm_open(SHARED_MEMORY_NAME, O_CREAT | O_EXCL | O_RDWR, 0666);
	if (_shm_fd == -1) {
	if (errno == EEXIST) {
	LOG(eTrace, "Shared memory segment already exists, opening existing segment\n");
	// Try to open the existing shared memory segment
	_shm_fd = shm_open(SHARED_MEMORY_NAME, O_RDWR, 0666);
	if (_shm_fd == -1) {
	ERR_LOG("shm_open");
	return false;
	}
	}
	else {