Latency by DouglasAdler · Pull Request #16 · rdkcentral/rdkperf

DouglasAdler · 2026-04-11T19:57:14Z

No description provided.

Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>

* Development (#10) * Test for crash on exit Signed-off-by: Douglas Adler <douglas.adler@yahoo.com> * Test for crash on exit Signed-off-by: Douglas Adler <douglas.adler@yahoo.com> --------- Signed-off-by: Douglas Adler <douglas.adler@yahoo.com> * Initial latency metrics Signed-off-by: Douglas Adler <douglas.adler@yahoo.com> --------- Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>

and elapsed time metrics. Signed-off-by: Douglas Adler <douglas_adler2@comcast.com>

Signed-off-by: Douglas Adler <douglas_adler2@comcast.com>

* Framework for memory tracking Signed-off-by: Adler, Douglas <Douglas_Adler2+comcast@comcast.com>

* Development (#10) * Test for crash on exit Signed-off-by: Douglas Adler <douglas.adler@yahoo.com> * Test for crash on exit Signed-off-by: Douglas Adler <douglas.adler@yahoo.com> --------- Signed-off-by: Douglas Adler <douglas.adler@yahoo.com> * Deploy cla action * Deploy cla action --------- Signed-off-by: Douglas Adler <douglas.adler@yahoo.com> Co-authored-by: rdkcmf <github@code.rdkcentral.com> Co-authored-by: Alan Ryan <20208488+Alan-Ryan@users.noreply.github.com>

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.

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

Agent-Logs-Url: https://github.com/rdkcentral/rdkperf/sessions/8da17201-483f-4eac-afaa-3be3dc29a607 Co-authored-by: DouglasAdler <3778029+DouglasAdler@users.noreply.github.com>

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 · 2026-04-11T20:35:49Z

+SharedMemoryBlock* SharedMemoryBlock::get_instance(uint64_t maxDataSize) 
+{
+    if(_shared_memory_block == nullptr) {
+        LOG(eWarning, "Creating new SharedMemoryBlock instance\n");
+        _shared_memory_block = new SharedMemoryBlock(maxDataSize);
+    }
+
+    return _shared_memory_block;
+}


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 · 2026-04-11T20:35:49Z

+typedef struct CircBufferObject_s
+{
+    CircBufferRecord    records[sizeof(CircBufferRecord) * MAX_TIME_STAMPS];
+    uint32_t            maxRecords;
+    uint32_t            head;
+    uint32_t            tail;
+    uint32_t            currentSize;
+    char*               name;   // pointer to the name of the location or sequence, stored in parent structure
+} CircBufferObject;


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 · 2026-04-11T20:35:50Z

+typedef struct CircBufferObject_s
+{
+    CircBufferRecord    records[sizeof(CircBufferRecord) * MAX_TIME_STAMPS];
+    uint32_t            maxRecords;
+    uint32_t            head;
+    uint32_t            tail;
+    uint32_t            currentSize;
+    char*               name;   // pointer to the name of the location or sequence, stored in parent structure
+} CircBufferObject;
+
+
+typedef struct Sequence_s
+{
+    char        name[MAX_NAME_LEN];
+    int32_t     location_offset;
+    uint8_t     timeStampCirBuffer[sizeof(CircBufferObject)];   // Array of root timestamps for the sequence
+    //uint64_t    rootTimeStamp; // root timestamp in microseconds, updated each time the first location is recorded
+} Sequence;


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 · 2026-04-11T20:35:54Z

+typedef struct Location_s
+{
+    char        name[MAX_NAME_LEN];
+    Sequence*   sequence;
+    uint8_t     timeStampCirBuffer[sizeof(CircBufferObject)];
+    uint32_t    count;


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.

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

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 · 2026-04-12T18:36:42Z

+
+typedef struct CircBufferObject_s
+{
+    CircBufferRecord    records[sizeof(CircBufferRecord) * MAX_TIME_STAMPS];


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 · 2026-04-12T18:36:43Z

+            _locations[i].max = 0;
+
+            CircularBuffer circBuffer(&_locations[i].timeStampCirBuffer[0], MAX_TIME_STAMPS);
+            circBuffer.set_name(_locations[i].name);
+            // If this is the first location in the sequence set the starting location offset


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 · 2026-04-12T18:36:43Z

+    // Count the locations
+    Location* location = _locations + _current_sequence->location_offset;
+    while(location != nullptr) {
+        count++;
+        if(location->child_offset == INVALID_OFFSET) {


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 · 2026-04-12T18:36:44Z

+        LOG(eError, "No current sequence to get data record\n");
+        return nullptr;
+    }
+


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 · 2026-04-12T18:36:44Z

+    if(pSeq->GetSequence(sequence)) {
+        DataRecord* pData = pSeq->GetDataRecord();
+        if(pData != nullptr) {
+            fprintf(fp, "Sequence %s Depth %d\n", pSeq->GetName(), pSeq->GetLocationsDepth());
+            uint8_t depth = 1;


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 · 2026-04-12T18:36:44Z

+        LOG(eWarning, "[%s] Buffer FULL (size=%u, max=%u) - removing oldest key %u at tail=%u\n", 
+            circBuffer->name, circBuffer->currentSize, circBuffer->maxRecords, 
+            circBuffer->records[circBuffer->tail].key, circBuffer->tail);
+        circBuffer->tail = (circBuffer->tail + 1) % circBuffer->maxRecords;


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 · 2026-04-12T18:36:44Z

+            uint64_t rootDiff = (uint64_t)(timeStamp - rootTimeStamp);
+            // Update the total elapsed time
+            LOG(eTrace, "Root timestamp %lu, root diff %lf\n", rootTimeStamp, rootDiff);
+            _location->total_elapsed +=  rootDiff;


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).

and semaphore management in `PerfSequence` and `SharedMemoryBlock`. Signed-off-by: Douglas Adler <douglas_adler2@comcast.com>

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

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 · 2026-04-12T19:29:46Z

+    uint64_t timeStamp = 0;
+    return _timeStamps.pop(count, timeStamp);


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 · 2026-04-12T19:29:47Z

+// Module constructor/destructor functions
+static void __attribute__((constructor)) PerfSequenceModuleInit();
+static void __attribute__((destructor)) PerfSequenceModuleTerminate();
+
+// This function is assigned to execute as a library init
+//  using __attribute__((constructor))
+void PerfSequenceModuleInit()
+{
+    LOG(eWarning, "Initialize\n");
+    if(_perf_sequence != nullptr) {
+        LOG(eWarning, "_perf_sequence already initialized\n");
+        delete _perf_sequence;
+    }
+    _perf_sequence = new PerfSequence();
+}
+
+// This function is assigned to execute as a library termination
+//  using __attribute__((destructor))
+void PerfSequenceModuleTerminate()
+{


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 · 2026-04-12T19:29:47Z

+    // 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;


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 · 2026-04-12T19:29:47Z

+        // Not the first proces in the shared memory segment
+        while(memcmp(&_shared_block->initialization_uuid[0], initialization_uuid_binary, INITIALIZATION_UUID_BINARY_LEN) != 0) {
+            ERR_LOG("Shared memory segment not initialized");
+            // wait a bit for the shared memory segment to be initialized
+            usleep(1000);
+
+            static int nWaitCount = 0;
+            if(nWaitCount++ > 1000) {
+                // Waited too long for the shared memory segment to be initialized
+                LOG(eError, "Shared memory segment not initialized and waited too long\n");
+                return false;
+            }


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 · 2026-04-12T19:29:47Z

    case eThreshold:
        msg.msg_data.entry.pID = getpid();
        msg.msg_data.entry.tID = pthread_self();
        msg.msg_data.entry.nThresholdInUS = nThresholdInUS;
-        memcpy((void*)msg.msg_data.entry.szName, (void*)szName, MIN((size_t)(MAX_NAME_LEN - 1), strlen(szName)));
+        memcpy((void*)msg.msg_data.entry.szName, (void*)szName, MIN((size_t)(MAX_MSG_NAME_LEN - 1), strlen(szName)));
        break;
    case eExit:
        msg.msg_data.exit.pID = getpid();
        msg.msg_data.exit.tID = pthread_self();
        msg.msg_data.exit.nTimeStamp = nTimeStamp;
-        memcpy((void*)msg.msg_data.entry.szName, (void*)szName, MIN((size_t)(MAX_NAME_LEN - 1), strlen(szName)));
+        memcpy((void*)msg.msg_data.entry.szName, (void*)szName, MIN((size_t)(MAX_MSG_NAME_LEN - 1), strlen(szName)));
        break;


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 · 2026-04-12T19:29:47Z

    LOG(eWarning, "RDK Perf Logging initialize extending logging set to %d\n", s_VerboseLog);
-    const char *env_log_level = getenv("RDKPER_EXTENDED_LOGGING");
+    const char *env_log_level = getenv("RDKPERF_EXTENDED_LOGGING");
+    fprintf(stdout, "[DEBUG] env_log_level=%s\n", env_log_level ? env_log_level : "NULL");
    if(env_log_level != NULL &&
       strncasecmp(env_log_level, "true", strlen("true")) == 0) {
+      fprintf(stdout, "[DEBUG] Setting s_VerboseLog to true\n");
      s_VerboseLog = true;
      LOG(eWarning, "Enabling RDKPERF extended logging %d", s_VerboseLog);
+    } else if(env_log_level != NULL) {
+      fprintf(stdout, "[DEBUG] env_log_level='%s' did not match 'true'\n", env_log_level);
    }


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 · 2026-04-12T19:29:48Z

+SharedMemoryBlock::SharedMemoryBlock(uint64_t maxDataSize) 
+: _initialized(false)
+, _shm_fd(-1)
+, _shared_block(nullptr)
+{
+    _data_size = sizeof(GlobalStorage) + maxDataSize;
+    _initialized = initialize();
+}


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).

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

Signed-off-by: Douglas Adler <douglas_adler2@comcast.com>

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 · 2026-04-12T19:42:47Z

        msg.msg_data.exit.pID = getpid();
        msg.msg_data.exit.tID = pthread_self();
        msg.msg_data.exit.nTimeStamp = nTimeStamp;
-        memcpy((void*)msg.msg_data.entry.szName, (void*)szName, MIN((size_t)(MAX_NAME_LEN - 1), strlen(szName)));
+        memcpy((void*)msg.msg_data.exit.szName, (void*)szName, MIN((size_t)(MAX_MSG_NAME_LEN - 1), strlen(szName)));
        break;


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 · 2026-04-12T19:42:47Z

+    const char* retVal = "\n";
+
+    if(_location->child_offset == 0) {  
+        return retVal;
+    }


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 · 2026-04-12T19:42:48Z

+    if(_location->child_offset != 0) {
+        Location* child = (Location*)((uint8_t*)_locations + _location->child_offset);
+        retVal = child->name;
+    }


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 · 2026-04-12T19:42:48Z

+    // 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;


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 · 2026-04-12T19:42:48Z

+    // 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;  


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 · 2026-04-12T19:42:49Z

+        DataRecord* pData = pSeq->GetDataRecord();
+        if(pData != nullptr) {
+            fprintf(fp, "Sequence %s Depth %d\n", pSeq->GetName(), pSeq->GetLocationsDepth());
+            uint8_t depth = 1;
+            while(pData != nullptr) {


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 · 2026-04-12T19:42:49Z

+    for(int idx = 0; idx < 499; idx++) {
+        // if(idx % 250 == 0) {
+        //     LOG(eWarning, "Running latency test %d\n", idx);
+        // }
+        test_latency();


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 · 2026-04-12T19:42:49Z

+    if (full()) {
+        // Remove the oldest record
+        LOG(eWarning, "[%s] Buffer FULL (size=%u, max=%u) - removing oldest key %u at tail=%u\n", 
+            circBuffer->name, circBuffer->currentSize, circBuffer->maxRecords, 
+            circBuffer->records[circBuffer->tail].key, circBuffer->tail);


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 · 2026-04-12T19:42:49Z

+    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);


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 · 2026-04-12T19:42:50Z

+    // 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) {


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 {

Add latency architecture document

+, _current_location(nullptr)
+{
+    // Caclulate the size of the memory block
+    _memory_block_size = PerfLocation::GetLocationDataSize() * PerfLocation::GetMaxLocations();


Copilot

Pull request overview

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

+typedef struct CircBufferObject_s
+{
+    CircBufferRecord    records[sizeof(CircBufferRecord) * MAX_TIME_STAMPS];
+    uint32_t            maxRecords;


+    uint32_t            tail;
+    uint32_t            currentSize;
+    char*               name;   // pointer to the name of the location or sequence, stored in parent structure
+} CircBufferObject;


+                CircularBuffer rootTimeStamps(&_sequences[i].timeStampCirBuffer[0], MAX_TIME_STAMPS);
+                rootTimeStamps.set_name(_sequences[i].name);


+            CircularBuffer circBuffer(&_locations[i].timeStampCirBuffer[0], MAX_TIME_STAMPS);
+            circBuffer.set_name(_locations[i].name);


+        DataRecord* pData = pSeq->GetDataRecord();
+        if(pData != nullptr) {
+            fprintf(fp, "Sequence %s Depth %d\n", pSeq->GetName(), pSeq->GetLocationsDepth());
+            uint8_t depth = 1;
+            while(pData != nullptr) {
+                for(uint8_t i = 0; i <= depth; i++) {
+                    fprintf(fp, "--");
+                }
+                fprintf(fp, "| %s >> Elapsed %0.3lf Avg %0.3lf Min %0.3lf Max %0.3lf Count %u \n", 
+                            pData->name, 
+                            (double)pData->average_elapsed / 1000.0,
+                            (double)pData->average / 1000.0, 
+                            (double)pData->min / 1000.0, 
+                            (double)pData->max / 1000.0, 
+                            pData->count);
+                pData = pData->child;
+                depth++;
+            }
+            return true;
+        }


+    _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) {
+            ERR_LOG("shm_open");
+            return false;
+        }
+    }


+    // 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;
+    }


-        pthread_getname_np(msg.msg_data.entry.tID, msg.msg_data.entry.szThreadName, MAX_NAME_LEN);
-        memcpy((void*)msg.msg_data.entry.szName, (void*)szName, MIN((size_t)(MAX_NAME_LEN - 1), strlen(szName)));
+        pthread_getname_np(msg.msg_data.entry.tID, msg.msg_data.entry.szThreadName, MAX_MSG_NAME_LEN);
+        memcpy((void*)msg.msg_data.entry.szName, (void*)szName, MIN((size_t)(MAX_MSG_NAME_LEN - 1), strlen(szName)));


+        msg.msg_data.threshold.pID = getpid();
+        msg.msg_data.threshold.tID = pthread_self();
+        msg.msg_data.threshold.nThresholdInUS = nThresholdInUS;
+        memcpy((void*)msg.msg_data.threshold.szName, (void*)szName, MIN((size_t)(MAX_MSG_NAME_LEN - 1), strlen(szName)));


Reason for change: Fix race condition where process terminate happens before the timer thread is started. Test Procedure: Test using gst-inspect Risks: None Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>

Reason for change: Simplifed the timer loop and added some additional tests. Test Procedure: Test using make quick-exit-test Risks: None Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>

Reason for change: Follow directions from https://etwiki.sys.comcast.net/spaces/RDKDocumentation/pages/1589622522/Setting+up+Coverity+for+RDK-E+in+the+GitHub+Actions+workflow Test Procedure: None Risks: None Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>

Copilot

Copilot review overview

🟡 Changes recommended

Critical workflow, synchronization, invalid-access, and test-execution issues remain unresolved.

Review effort: Lite
Findings: 10 · 8

Open (18)

Coverity workflow invokes missing build.sh · New
Coverity workflow invokes missing build.sh · New
Unknown sequence reporting can dereference an invalid location · New
Report reads shared data without holding the interprocess lock · New
Forking after starting timer threads can deadlock the child · New
In ~SharedMemoryBlock(), the decision to destroy the semaphore/unlink SHM is made under lock, but…
GetChildName() computes the child pointer using byte arithmetic (`(uint8_t*)_locations +…
CircularBuffer::set_name() writes a pointer into shared memory (circBuffer->name), which is…
CircBufferObject::name is a raw pointer stored inside shared memory. Pointer values are not valid…
CircBufferObject::records is declared as an array of sizeof(CircBufferRecord) * MAX_TIME_STAMPS…
memcpy() copies szName into the message buffer without guaranteeing NUL termination. This can…
memcpy() copies szName into the message buffer without guaranteeing NUL termination. This can…
SharedMemoryBlock::initialize() treats any shm_open(O_CREAT|O_EXCL, ...) failure as "already…
RDKLatencyReport() walks the DataRecord chain returned by GetDataRecord() but never frees it.…
GetTimeStamp() uses peek() (tail element) and then checks the returned key matches count.…
GetDataRecord() divides by _location->count without guarding against 0. If a report is…
When creating a new location, the timestamp circular buffer is not reset; the CircularBuffer…
When creating a new sequence, CircularBuffer rootTimeStamps(...) currently calls…

Previously missed (2)

In code that hasn't changed since last review

Public header omits the uint32_t definition

rdkperf/rdk_mem_tracker.h:32

This public header uses uint32_t in its declarations but does not include the header that defines it. Any consumer that includes rdk_mem_tracker.h without first including <stdint.h> will fail to compile; add the fixed-width integer include to this header itself.

Singleton teardown skips shared memory cleanup

src/rdk_perf_sequence.cpp:97

Dropping this pointer without destroying the singleton means SharedMemoryBlock::~SharedMemoryBlock() never runs. Consequently attached_intances is never decremented and the shared-memory object is never cleaned up by normal library teardown; later runs reuse stale shared state.

+    if(pSeq->GetSequence(sequence)) {
+        DataRecord* pData = pSeq->GetDataRecord();


+    SCOPED_LOCK();
+    FILE* fp = stderr;
+    //fp = fopen("/tmp/rdkperf_latency.log", "a");
+
+    PerfSequence* pSeq = PerfSequence::GetInstance();
+    if(pSeq->GetSequence(sequence)) {
+        DataRecord* pData = pSeq->GetDataRecord();
+        if(pData != nullptr) {


…nts to formatting function' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

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

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved shared-memory safety, lifecycle, reporting, buffer, test, and Coverity workflow issues remain.

Review effort: Lite
Findings: 9 · 8

Open (17)

Location traversal lacks interprocess locking · New
Shared memory permissions allow unrestricted local access · New
Attachers can map shared memory before initialization · New
Report reads shared data without holding the interprocess lock
Unknown sequence reporting can dereference an invalid location
In ~SharedMemoryBlock(), the decision to destroy the semaphore/unlink SHM is made under lock, but…
GetChildName() computes the child pointer using byte arithmetic (`(uint8_t*)_locations +…
CircBufferObject::name is a raw pointer stored inside shared memory. Pointer values are not valid…
CircBufferObject::records is declared as an array of sizeof(CircBufferRecord) * MAX_TIME_STAMPS…
memcpy() copies szName into the message buffer without guaranteeing NUL termination. This can…
memcpy() copies szName into the message buffer without guaranteeing NUL termination. This can…
SharedMemoryBlock::initialize() treats any shm_open(O_CREAT|O_EXCL, ...) failure as "already…
RDKLatencyReport() walks the DataRecord chain returned by GetDataRecord() but never frees it.…
GetTimeStamp() uses peek() (tail element) and then checks the returned key matches count.…
GetDataRecord() divides by _location->count without guarding against 0. If a report is…
When creating a new location, the timestamp circular buffer is not reset; the CircularBuffer…
When creating a new sequence, CircularBuffer rootTimeStamps(...) currently calls…

Resolved since last review (4)

Forking after starting timer threads can deadlock the child
Coverity workflow invokes missing build.sh
Coverity workflow invokes missing build.sh
CircularBuffer::set_name() writes a pointer into shared memory (circBuffer->name), which is…

Previously missed (1)

In code that hasn't changed since last review

Public header uses uint32_t without including its definition

rdkperf/rdk_mem_tracker.h:21

This public header uses uint32_t in its declarations but does not include the header that defines it. Consumers that include rdk_mem_tracker.h directly can fail to compile depending on include order; make the header self-contained.

+        if(_current_sequence->location_offset != INVALID_OFFSET) {
+            Location* location = _locations + _current_sequence->location_offset;
+            while(location->name[0] != '\0') {
+                LOG(eTrace, "Comparing location %s with %s\n", location->name, locationName);
+                if(strncmp(location->name, locationName, MAX_NAME_LEN) == 0) {
+                    LOG(eTrace, "Found location %s\n", locationName);
+                    SetCurrentLocation(location);
+                    retVal = true;
+                }
+                if(location->child_offset == INVALID_OFFSET) {
+                    break;
+                }
+                location = _locations + location->child_offset;


+    bool bInitialize_memory = false;
+
+    // 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);


+        _shm_fd = shm_open(SHARED_MEMORY_NAME, O_RDWR, 0666);
+        if (_shm_fd == -1) {
+            ERR_LOG("shm_open");
+            return false;
+        }
+    }
+    else {
+        // We are the first to create the shared memory segment and can now initialize the memory
+        bInitialize_memory = true;
+
+        // Set the size of the shared memory segment
+        if (ftruncate(_shm_fd, _data_size) == -1) {
+            ERR_LOG("ftruncate");
+            return false;
+        }
+    }
+
+    // Map the shared memory segment into the process's address space
+    _shared_block = static_cast<GlobalStorage*>(mmap(nullptr, _data_size, PROT_READ | PROT_WRITE, MAP_SHARED, _shm_fd, 0));


Copilot

Copilot review overview

🟡 Changes recommended

Unresolved shared-memory correctness, memory-safety, build, lifecycle, and CI issues remain.

Review effort: Lite
Findings: 10 · 8

Open (18)

CircularBuffer methods are defined twice · New
Attachers can map shared memory before initialization
Shared memory permissions allow unrestricted local access
Location traversal lacks interprocess locking
Report reads shared data without holding the interprocess lock
Unknown sequence reporting can dereference an invalid location
In ~SharedMemoryBlock(), the decision to destroy the semaphore/unlink SHM is made under lock, but…
GetChildName() computes the child pointer using byte arithmetic (`(uint8_t*)_locations +…
CircBufferObject::name is a raw pointer stored inside shared memory. Pointer values are not valid…
CircBufferObject::records is declared as an array of sizeof(CircBufferRecord) * MAX_TIME_STAMPS…
memcpy() copies szName into the message buffer without guaranteeing NUL termination. This can…
memcpy() copies szName into the message buffer without guaranteeing NUL termination. This can…
SharedMemoryBlock::initialize() treats any shm_open(O_CREAT|O_EXCL, ...) failure as "already…
RDKLatencyReport() walks the DataRecord chain returned by GetDataRecord() but never frees it.…
GetTimeStamp() uses peek() (tail element) and then checks the returned key matches count.…
GetDataRecord() divides by _location->count without guarding against 0. If a report is…
When creating a new location, the timestamp circular buffer is not reset; the CircularBuffer…
When creating a new sequence, CircularBuffer rootTimeStamps(...) currently calls…

Previously missed (2)

In code that hasn't changed since last review

Public header omits stdint.h for uint32_t

rdkperf/rdk_mem_tracker.h:23

This public header uses uint32_t in both its C and C++ declarations but never includes <stdint.h>. Any consumer that includes this header without first including another header defining uint32_t will fail to compile. Include the type header directly here.

Shared memory block is never released

src/rdk_perf_sequence.cpp:98

This destructor only drops the local pointer; it never destroys/releases the SharedMemoryBlock. Consequently attached_intances is never decremented and the POSIX shared-memory object is never unlinked, so every process that loads the library leaves persistent shared state and the documented last-process cleanup cannot occur.

+#if USE_CIRCULAR_BUFFER
+// Circular Buffer (CoPilot Implementation)
+CircularBuffer::CircularBuffer()
+: circBuffer(nullptr)
+{


DouglasAdler and others added 7 commits March 29, 2023 19:54

Test for crash on exit

44c8e1e

Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>

Test for crash on exit

f17188f

Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>

Latency now working with root time stamps

6cb6ebb

and elapsed time metrics. Signed-off-by: Douglas Adler <douglas_adler2@comcast.com>

Improved latency feature after testing in actual gstreamer pipeline

ef15b4d

Signed-off-by: Douglas Adler <douglas_adler2@comcast.com>

* Additional latency debugging of the circular buffer

30b2041

* Framework for memory tracking Signed-off-by: Adler, Douglas <Douglas_Adler2+comcast@comcast.com>

Copilot AI lite review requested due to automatic review settings April 11, 2026 19:57

Copilot started reviewing on behalf of DouglasAdler April 11, 2026 19:57 View session

DouglasAdler commented Apr 11, 2026

View reviewed changes

Copilot AI reviewed Apr 11, 2026

View reviewed changes

DouglasAdler and others added 2 commits April 11, 2026 20:25

Merge branch 'main' into latency

9b34816

Update test/unit_tests.cpp

a6add49

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

Copilot AI review requested due to automatic review settings April 11, 2026 20:28

Copilot started reviewing on behalf of DouglasAdler April 11, 2026 20:29 View session

DouglasAdler and others added 2 commits April 11, 2026 15:30

Update test/unit_tests.cpp

b99cfaa

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

Fix RDKPerf temporaries in unit_tests.cpp - use named local variables

15f07ea

Agent-Logs-Url: https://github.com/rdkcentral/rdkperf/sessions/8da17201-483f-4eac-afaa-3be3dc29a607 Co-authored-by: DouglasAdler <3778029+DouglasAdler@users.noreply.github.com>

Copilot AI reviewed Apr 11, 2026

View reviewed changes

Update src/rdk_perf_sequence.cpp

a59d047

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

Copilot AI review requested due to automatic review settings April 12, 2026 18:24

Copilot started reviewing on behalf of DouglasAdler April 12, 2026 18:25 View session

Apply suggestion from @Copilot

30f21be

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

Copilot AI reviewed Apr 12, 2026

View reviewed changes

DouglasAdler and others added 2 commits April 12, 2026 19:16

Added CoPilot suggestions to clean up shared memory block

e1183b7

and semaphore management in `PerfSequence` and `SharedMemoryBlock`. Signed-off-by: Douglas Adler <douglas_adler2@comcast.com>

Apply suggestion from @Copilot

041c238

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

Copilot AI review requested due to automatic review settings April 12, 2026 19:24

Apply suggestion from @Copilot

a5c0d3d

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

Copilot started reviewing on behalf of DouglasAdler April 12, 2026 19:24 View session

Copilot AI reviewed Apr 12, 2026

View reviewed changes

Apply suggestion from @Copilot

98ccbf1

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

Copilot AI review requested due to automatic review settings April 12, 2026 19:36

Added additional CoPilot suggestions

8460432

Signed-off-by: Douglas Adler <douglas_adler2@comcast.com>

Copilot AI reviewed Apr 12, 2026

View reviewed changes

Create LATENCY_ARCHITECTURE.md

343184d

Add latency architecture document

Copilot AI review requested due to automatic review settings July 14, 2026 21:20

Copilot started reviewing on behalf of DouglasAdler July 14, 2026 21:20 View session

github-advanced-security Bot found potential problems Jul 14, 2026

View reviewed changes

Comment thread rdkperf/rdk_mem_tracker.cpp Fixed

Comment thread src/rdk_perf_sequence.cpp

, _current_location(nullptr)

{

// Caclulate the size of the memory block

_memory_block_size = PerfLocation::GetLocationDataSize() * PerfLocation::GetMaxLocations();

Copilot AI reviewed Jul 14, 2026

View reviewed changes

DouglasAdler added 3 commits September 27, 2026 13:15

WNCXIONE-662 : Timer loop stuck

f82ff6c

Reason for change: Fix race condition where process terminate happens before the timer thread is started. Test Procedure: Test using gst-inspect Risks: None Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>

WNCXIONE-662 : Timer loop stuck

8609206

Reason for change: Simplifed the timer loop and added some additional tests. Test Procedure: Test using make quick-exit-test Risks: None Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>

Copilot AI review requested due to automatic review settings September 27, 2026 19:10

Copilot started reviewing on behalf of DouglasAdler September 27, 2026 19:10 View session

Copilot AI reviewed Sep 27, 2026

View reviewed changes

Potential fix for pull request finding 'CodeQL / Wrong type of argume…

35d976a

…nts to formatting function' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 27, 2026 19:45

Copilot started reviewing on behalf of DouglasAdler September 27, 2026 19:46 View session

Potential fix for pull request finding

125b72e

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

Copilot AI reviewed Sep 27, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 27, 2026 19:50

Copilot started reviewing on behalf of DouglasAdler September 27, 2026 19:50 View session

Copilot AI reviewed Sep 27, 2026

View reviewed changes

Comment thread src/rdk_perf_location.cpp

Comment on lines +35 to +39

#if USE_CIRCULAR_BUFFER

// Circular Buffer (CoPilot Implementation)

CircularBuffer::CircularBuffer()

: circBuffer(nullptr)

{

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

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

		uint64_t timeStamp = 0;
		return _timeStamps.pop(count, timeStamp);

-    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;

		CircularBuffer rootTimeStamps(&_sequences[i].timeStampCirBuffer[0], MAX_TIME_STAMPS);
		rootTimeStamps.set_name(_sequences[i].name);

		CircularBuffer circBuffer(&_locations[i].timeStampCirBuffer[0], MAX_TIME_STAMPS);
		circBuffer.set_name(_locations[i].name);

		if(pSeq->GetSequence(sequence)) {
		DataRecord* pData = pSeq->GetDataRecord();

Conversation

DouglasAdler commented Apr 11, 2026

Uh oh!

DouglasAdler left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!