Skip to content

Commit

Permalink
Fix heap snapshot comparisons when an entire category is removed
Browse files Browse the repository at this point in the history
I recently introduced a bug in heap snapshot comparisons where if all
instances of a class are removed, the class name shows up as blank in
the comparison view. This change fixes the bug by using the class name
from the "before" state when there is no "after" state.

Bug: 377200307
Change-Id: Ia1c42ea25f90fde0f6a92766f52cb6e66c9ec119
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/5992931
Reviewed-by: Simon Zünd <szuend@chromium.org>
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
  • Loading branch information
sethbrenith authored and Devtools-frontend LUCI CQ committed Nov 6, 2024
1 parent 2de6ccf commit 204e831
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 4 deletions.
4 changes: 2 additions & 2 deletions front_end/entrypoints/heap_snapshot_worker/HeapSnapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1333,7 +1333,7 @@ export abstract class HeapSnapshot {
selfSizes[i] = node.selfSize();
}

result[classKey] = {indexes, ids, selfSizes};
result[classKey] = {name: node.className(), indexes, ids, selfSizes};
}

this.#aggregatesForDiffInternal = {interfaceDefinitions, aggregates: result};
Expand Down Expand Up @@ -2480,7 +2480,7 @@ export abstract class HeapSnapshot {
let j = 0;
const l = baseIds.length;
const m = indexes.length;
const diff = new HeapSnapshotModel.HeapSnapshotModel.Diff(aggregate ? aggregate.name : '');
const diff = new HeapSnapshotModel.HeapSnapshotModel.Diff(aggregate ? aggregate.name : baseAggregate.name);

const nodeB = this.createNode(indexes[j]);
while (i < l && j < m) {
Expand Down
2 changes: 2 additions & 0 deletions front_end/models/heap_snapshot_model/HeapSnapshotModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,12 @@ export class Aggregate {
}

export class AggregateForDiff {
name: string;
indexes: number[];
ids: number[];
selfSizes: number[];
constructor() {
this.name = '';
this.indexes = [];
this.ids = [];
this.selfSizes = [];
Expand Down
7 changes: 5 additions & 2 deletions test/e2e/memory/memory_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -580,9 +580,10 @@ describe('The Memory Panel', function() {
await takeHeapSnapshot();
await waitForNonEmptyHeapSnapshotData();
await setClassFilter('DuplicatedClassName');
let rows = await waitForMany('tr.data-grid-data-grid-node', 2);
let rows = await waitForMany('tr.data-grid-data-grid-node', 3);
assert.strictEqual(30, await getCountFromCategoryRow(rows[0]));
assert.strictEqual(3, await getCountFromCategoryRow(rows[1]));
assert.strictEqual(2, await getCountFromCategoryRow(rows[2]));
await focusTableRow(rows[0]);
await expandFocusedRow();
const {frontend, target} = await getBrowserAndPages();
Expand All @@ -598,10 +599,12 @@ describe('The Memory Panel', function() {
await waitForNonEmptyHeapSnapshotData();
await changeViewViaDropdown('Comparison');
await setClassFilter('DuplicatedClassName');
rows = await waitForMany('tr.data-grid-data-grid-node', 2);
rows = await waitForMany('tr.data-grid-data-grid-node', 3);
assert.strictEqual(5, await getAddedCountFromComparisonRow(rows[0]));
assert.strictEqual(1, await getRemovedCountFromComparisonRow(rows[0]));
assert.strictEqual(1, await getAddedCountFromComparisonRow(rows[1]));
assert.strictEqual(10, await getRemovedCountFromComparisonRow(rows[1]));
assert.strictEqual(0, await getAddedCountFromComparisonRow(rows[2]));
assert.strictEqual(2, await getRemovedCountFromComparisonRow(rows[2]));
});
});
5 changes: 5 additions & 0 deletions test/e2e/resources/memory/duplicated-names.html
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ <h1>Memory Panel (Heap profiler) Test</h1>
}
holder.a.x = holder.d[0];
holder.d[0].x = [1, 2, 3];
(function () {
class DuplicatedClassName {}
holder.e = new DuplicatedClassName();
holder.f = new DuplicatedClassName();
})();
document.getElementById('update').addEventListener('click', function () {
holder.b = new DuplicatedClassName();
holder.e = new DuplicatedClassName();
Expand Down

0 comments on commit 204e831

Please sign in to comment.