[RPP] Update sample trace id parsing

As of https://crrev.com/c/6397026 the shape of trace id in CPU profile
changed to a traceid -> node id mapping (before it was sample index ->
traceid). This allows us to simplify their parsing as they are we no
longer need to get the stack data (node id) via the sample index,
effectively removing a level of indirection and preventing bugs due to
the samples order being altered in the frontend (see attached bug).

Fixed: 403628855
Change-Id: I1d56ce8aa6414798a95a20b5dbcd9ecb871043f7
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6394268
Reviewed-by: Paul Irish <[email protected]>
Commit-Queue: Andres Olivares <[email protected]>

Author: and-oli

front_end/models/cpu_profile/CPUProfileDataModel.ts

@@ -557,5 +557,13 @@ export type ExtendedProfileNode = Protocol.Profiler.ProfileNode&{parent?: number
 export type ExtendedProfile = Protocol.Profiler.Profile&{
   nodes: Protocol.Profiler.ProfileNode[] | ExtendedProfileNode[],
   lines?: number[],
+  /**
+   * A sample can be manually collected with v8::CpuProfiler::collectSample.
+   * When this is done an id (trace id) can be passed to the API to
+   * identify the collected sample in the resulting CPU profile. We
+   * do this for several trace events, to efficiently calculate their
+   * stack trace and improve the JS flamechart we build. This property
+   * contains the mapping of the trace ids with the shape traceId -> nodeId
+   */
   traceIds?: Record<string, number>,
 };

front_end/models/trace/helpers/SamplesIntegrator.test.ts

@@ -243,7 +243,7 @@ describeWithEnvironment('SamplesIntegrator', function() {
           {id: 5, callFrame: {functionName: 'sheep', scriptId, url, lineNumber, columnNumber}},
         ],
         samples: [3, 3, 3, 5],
-        traceIds: {3: traceId},
+        traceIds: {[traceId]: 5},
         timeDeltas: new Array(4).fill(100),
       };
 

front_end/models/trace/helpers/SamplesIntegrator.ts

@@ -70,12 +70,6 @@ export class SamplesIntegrator {
    * in the stack before the event came.
    */
   #lockedJsStackDepth: number[] = [];
-  /**
-   * For samples with a trace id, creates a profile call and keeps it
-   * in a record keyed by that id. The value is typed as an union with
-   * undefined to avoid nullish accesses when a key is not present.
-   */
-  #callsByTraceIds: Record<number, Types.Events.SyntheticProfileCall|undefined> = {};
   /**
    * Used to keep track when samples should be integrated even if they
    * are not children of invocation trace events. This is useful in
@@ -237,7 +231,6 @@ export class SamplesIntegrator {
   callsFromProfileSamples(): Types.Events.SyntheticProfileCall[] {
     const samples = this.#profileModel.samples;
     const timestamps = this.#profileModel.timestamps;
-    const debugModeEnabled = this.#engineConfig.debugMode;
     if (!samples) {
       return [];
     }
@@ -249,16 +242,10 @@ export class SamplesIntegrator {
       if (!node) {
         continue;
       }
-      const maybeTraceId = this.#profileModel.traceIds?.[i];
       const call = makeProfileCall(node, this.#profileId, i, timestamp, this.#processId, this.#threadId);
-      // Separate samples with trace ids so that they are only used when
-      // processing the owner event.
-      if (maybeTraceId === undefined) {
-        calls.push(call);
-      } else {
-        this.#callsByTraceIds[maybeTraceId] = call;
-      }
-      if (debugModeEnabled) {
+      calls.push(call);
+
+      if (this.#engineConfig.debugMode) {
         const traceId = this.#profileModel.traceIds?.[i];
         this.jsSampleEvents.push(this.#makeJSSampleEvent(call, timestamp, traceId));
       }
@@ -322,6 +309,19 @@ export class SamplesIntegrator {
     return callFrames;
   }
 
+  #getStackForSampleTraceId(traceId: number, timestamp: Types.Timing.Micro): Types.Events.SyntheticProfileCall[]|null {
+    const nodeId = this.#profileModel.traceIds?.[traceId];
+    const node = nodeId && this.#profileModel.nodeById(nodeId);
+    const maybeCallForTraceId =
+        node && makeProfileCall(node, this.#profileId, -1, timestamp, this.#processId, this.#threadId);
+    if (!maybeCallForTraceId) {
+      return null;
+    }
+    if (this.#engineConfig.debugMode) {
+      this.jsSampleEvents.push(this.#makeJSSampleEvent(maybeCallForTraceId, timestamp, traceId));
+    }
+    return this.#makeProfileCallsForStack(maybeCallForTraceId);
+  }
   /**
    * Update tracked stack using this event's call stack.
    */
@@ -331,11 +331,9 @@ export class SamplesIntegrator {
       stackTrace = this.#makeProfileCallsForStack(event);
     }
     const traceId = extractSampleTraceId(event);
-    if (traceId !== null) {
-      const maybeCallForTraceId = this.#callsByTraceIds[traceId];
-      if (maybeCallForTraceId) {
-        stackTrace = this.#makeProfileCallsForStack(maybeCallForTraceId, event.ts);
-      }
+    const maybeCallForTraceId = traceId && this.#getStackForSampleTraceId(traceId, event.ts);
+    if (maybeCallForTraceId) {
+      stackTrace = maybeCallForTraceId;
     }
 
     SamplesIntegrator.filterStackFrames(stackTrace, this.#engineConfig);