fix(benchpress): correctly report GC memory amounts (#50760)

This PR fixes GC memory numbers reported by benchpress, where previously reported amount was always 0. This is due to the fact that Chrome browser reports GC events as a single X event now, instead of a B / E pair of events. PR Close #50760
angular · Jun 26, 2023 · dd850b2 · dd850b2
1 parent 060830e
commit dd850b2
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 19 deletions.
diff --git a/packages/benchpress/src/metric/perflog_metric.ts b/packages/benchpress/src/metric/perflog_metric.ts
@@ -344,9 +344,12 @@ export class PerflogMetric extends Metric {
           intervalStarts[name] = null!;
           if (name === 'gc') {
             result['gcTime'] += duration;
-            const amount =
-                (startEvent['args']!['usedHeapSize']! - event['args']!['usedHeapSize']!) / 1000;
-            result['gcAmount'] += amount;
+            const gcAmount = event['args']?.['gcAmount'] ?? 0;
+            const amount = gcAmount > 0 ?
+                gcAmount :
+                (startEvent['args']!['usedHeapSize']! - event['args']!['usedHeapSize']!);
+
+            result['gcAmount'] += amount / 1000;
             const majorGc = event['args']!['majorGc'];
             if (majorGc && majorGc) {
               result['majorGcTime'] += duration;

diff --git a/packages/benchpress/src/web_driver_extension.ts b/packages/benchpress/src/web_driver_extension.ts
@@ -21,6 +21,7 @@ export type PerfLogEvent = {
   args?: {
     encodedDataLength?: number,
     usedHeapSize?: number,
+    gcAmount?: number,
     majorGc?: boolean,
     url?: string,
     method?: string

diff --git a/packages/benchpress/src/webdriver/chrome_driver_extension.ts b/packages/benchpress/src/webdriver/chrome_driver_extension.ts
@@ -147,19 +147,9 @@ export class ChromeDriverExtension extends WebDriverExtension {
             categories, name, ['disabled-by-default-devtools.timeline'], 'CompositeLayers')) {
       return normalizeEvent(event, {'name': 'render'});
     } else if (this._isEvent(categories, name, ['devtools.timeline', 'v8'], 'MajorGC')) {
-      const normArgs = {
-        'majorGc': true,
-        'usedHeapSize': args['usedHeapSizeAfter'] !== undefined ? args['usedHeapSizeAfter'] :
-                                                                  args['usedHeapSizeBefore']
-      };
-      return normalizeEvent(event, {'name': 'gc', 'args': normArgs});
+      return normalizeGCEvent(event, true);
     } else if (this._isEvent(categories, name, ['devtools.timeline', 'v8'], 'MinorGC')) {
-      const normArgs = {
-        'majorGc': false,
-        'usedHeapSize': args['usedHeapSizeAfter'] !== undefined ? args['usedHeapSizeAfter'] :
-                                                                  args['usedHeapSizeBefore']
-      };
-      return normalizeEvent(event, {'name': 'gc', 'args': normArgs});
+      return normalizeGCEvent(event, false);
     } else if (
         this._isEvent(categories, name, ['devtools.timeline'], 'FunctionCall') &&
         (!args || !args['data'] ||
@@ -242,3 +232,25 @@ function normalizeEvent(chromeEvent: {[key: string]: any}, data: PerfLogEvent):
   }
   return result;
 }
+
+function normalizeGCEvent(chromeEvent: {[key: string]: any}, majorGc: boolean) {
+  const args = chromeEvent['args'];
+  const heapSizeBefore = args['usedHeapSizeBefore'];
+  const heapSizeAfter = args['usedHeapSizeAfter'];
+
+  if (heapSizeBefore === undefined && heapSizeAfter === undefined) {
+    throw new Error(
+        `GC event didn't specify heap size to calculate amount of collected memory. Expected one of usedHeapSizeBefore / usedHeapSizeAfter arguments but got ${
+            JSON.stringify(args)}`);
+  }
+
+  const normalizedArgs = {
+    'majorGc': majorGc,
+    'usedHeapSize': heapSizeAfter !== undefined ? heapSizeAfter : heapSizeBefore,
+    'gcAmount': heapSizeBefore !== undefined && heapSizeAfter !== undefined ?
+        heapSizeBefore - heapSizeAfter :
+        0,
+  };
+
+  return normalizeEvent(chromeEvent, {'name': 'gc', 'args': normalizedArgs});
+}
diff --git a/packages/benchpress/test/webdriver/chrome_driver_extension_spec.ts b/packages/benchpress/test/webdriver/chrome_driver_extension_spec.ts
@@ -153,9 +153,26 @@ import {TraceEventFactory} from '../trace_event_factory';
           .then((events) => {
             expect(events.length).toEqual(2);
             expect(events[0]).toEqual(
-                normEvents.start('gc', 1.0, {'usedHeapSize': 1000, 'majorGc': false}));
+                normEvents.start('gc', 1.0, {'usedHeapSize': 1000, 'majorGc': false, gcAmount: 0}));
             expect(events[1]).toEqual(
-                normEvents.end('gc', 2.0, {'usedHeapSize': 0, 'majorGc': false}));
+                normEvents.end('gc', 2.0, {'usedHeapSize': 0, 'majorGc': false, gcAmount: 0}));
+            done();
+          });
+    });
+
+    it('should report minor gc with GC amount', done => {
+      createExtension([
+        chromeTimelineV8Events.create(
+            'X', 'MinorGC', 1000, {'usedHeapSizeBefore': 5000, 'usedHeapSizeAfter': 2000}),
+      ])
+          .readPerfLog()
+          .then((events) => {
+            expect(events.length).toEqual(1);
+            expect(events[0]).toEqual({
+              ...normEvents.create(
+                  'X', 'gc', 1.0, {'usedHeapSize': 2000, 'majorGc': false, gcAmount: 3000}),
+              dur: 0
+            });
             done();
           });
     });
@@ -171,9 +188,26 @@ import {TraceEventFactory} from '../trace_event_factory';
           .then((events) => {
             expect(events.length).toEqual(2);
             expect(events[0]).toEqual(
-                normEvents.start('gc', 1.0, {'usedHeapSize': 1000, 'majorGc': true}));
+                normEvents.start('gc', 1.0, {'usedHeapSize': 1000, 'majorGc': true, gcAmount: 0}));
             expect(events[1]).toEqual(
-                normEvents.end('gc', 2.0, {'usedHeapSize': 0, 'majorGc': true}));
+                normEvents.end('gc', 2.0, {'usedHeapSize': 0, 'majorGc': true, gcAmount: 0}));
+            done();
+          });
+    });
+
+    it('should report major gc with GC amount', done => {
+      createExtension([
+        chromeTimelineV8Events.create(
+            'X', 'MajorGC', 1000, {'usedHeapSizeBefore': 5000, 'usedHeapSizeAfter': 2000}),
+      ])
+          .readPerfLog()
+          .then((events) => {
+            expect(events.length).toEqual(1);
+            expect(events[0]).toEqual({
+              ...normEvents.create(
+                  'X', 'gc', 1.0, {'usedHeapSize': 2000, 'majorGc': true, gcAmount: 3000}),
+              dur: 0
+            });
             done();
           });
     });