Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend the workaround in the v53 upgrader to generate missing subcategory columns. #5369

Merged
merged 2 commits into from
Feb 18, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/profile-logic/import/simpleperf.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { PROCESSED_PROFILE_VERSION } from 'firefox-profiler/app-logic/constants'
import type { Milliseconds } from 'firefox-profiler/types/units';
import type {
CategoryList,
CategoryColor,
FrameTable,
FuncTable,
IndexIntoCategoryList,
Expand Down Expand Up @@ -64,7 +65,10 @@ class Categories {
return this.categoryList;
}

static createCategory(name: string, color: string): IndexIntoCategoryList {
static createCategory(
name: string,
color: CategoryColor
): IndexIntoCategoryList {
const index = this.categoryList.length;
this.categoryList.push({ name, color, subcategories: ['Other'] });

Expand Down
31 changes: 22 additions & 9 deletions src/profile-logic/processed-profile-versioning.js
Original file line number Diff line number Diff line change
Expand Up @@ -2350,21 +2350,34 @@
for (const thread of profile.threads) {
const { frameTable, stackTable } = thread;

// Best-effort fix for profiles missing frameTable.category, such as the
// ones generated by Lean before https://github.com/leanprover/lean4/pull/6363
if (!('category' in frameTable)) {
frameTable.category = [];
frameTable.subcategory = [];
for (let frameIndex = 0; frameIndex < frameTable.length; frameIndex++) {
frameTable.category[frameIndex] = null;
frameTable.subcategory[frameIndex] = null;
}
// Attempt to keep some existing profiles working which weren't compliant
// with the profile format's type definitions.
if (!frameTable.category) {
// Profiles generated by Lean before https://github.com/leanprover/lean4/pull/6363
// didn't have category / subcategory columns in the frameTable. Migrate
// the category / subcategory values from the stackTable.
frameTable.category = new Array(frameTable.length).fill(null);
frameTable.subcategory = new Array(frameTable.length).fill(null);

Check warning on line 2360 in src/profile-logic/processed-profile-versioning.js

View check run for this annotation

Codecov / codecov/patch

src/profile-logic/processed-profile-versioning.js#L2359-L2360

Added lines #L2359 - L2360 were not covered by tests
for (let stackIndex = 0; stackIndex < stackTable.length; stackIndex++) {
const frameIndex = stackTable.frame[stackIndex];
frameTable.category[frameIndex] = stackTable.category[stackIndex];
frameTable.subcategory[frameIndex] =
stackTable.subcategory[stackIndex];
}
} else if (!frameTable.subcategory) {
// Profiles from vernier before https://github.com/jhawthorn/vernier/issues/128
// didn't contain a subcategory column in the frameTable.
//
// Supply a column where every value is set to 0.
// 0 is always a valid value for the subcategory.
//
// The requirements for subcategory values are:
// - For frames with a null category, the subcategory value is ignored.
// - For frames with a non-null category, the subcategory must be non-null.
// - Subcategory 0 refers to the category itself; in profile.meta.categories,
// every category should have a subcategory list which starts with the
// generic subcategory "Other" at index 0.
frameTable.subcategory = new Array(frameTable.length).fill(0);

Check warning on line 2380 in src/profile-logic/processed-profile-versioning.js

View check run for this annotation

Codecov / codecov/patch

src/profile-logic/processed-profile-versioning.js#L2380

Added line #L2380 was not covered by tests
}

// Remove stackTable.category and stackTable.subcategory.
Expand Down
50 changes: 45 additions & 5 deletions src/types/profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,21 @@ export type FrameTable = {|
// nativeSymbol, but each has a different func and line.
inlineDepth: number[],

// The category of the frame. This is used to calculate the category of the stack nodes
// which use this frame:
// - If the frame has a null category, the stack node inherits its parent node's category
// and subcategory. If there is no parent node, we use the "default category" (see ProfileMeta.categories).
// - If the frame has a non-null category, this category and subcategory is used for the stack node.
category: (IndexIntoCategoryList | null)[],

// The subcategory of a frame. This is used to calculate the subcategory of the stack nodes
// which use this frame.
// Must be non-null if the frame's category is non-null.
// Ignored if the frame's category is null.
// 0 is always a valid value and refers to the "Other" subcategory (see Category.subcategories).
subcategory: (IndexIntoSubcategoryListForCategory | null)[],

// The frame's function.
func: IndexIntoFuncTable[],

// The symbol index (referring into this thread's nativeSymbols table) corresponding
Expand Down Expand Up @@ -391,9 +404,36 @@ export type Lib = {|
codeId: string | null, // e.g. "6132B96B70fd000"
|};

// The list of available category colors.
//
// We don't accept just any CSS color so that the front-end has more freedom about
// picking colors, for example to ensure contrast or to adjust to light/dark modes.
export type CategoryColor =
| 'transparent' // used for "idle" frames / stacks
| 'purple'
| 'green'
| 'orange'
| 'yellow'
| 'lightblue'
| 'blue'
| 'brown'
| 'magenta'
| 'red'
| 'lightred'
| 'darkgrey'
| 'grey'; // <-- "grey" marks the default category

// A category in profile.meta.categories, used for stack frames and call nodes.
export type Category = {|
// The category name.
name: string,
color: string,

// The category color. Must be picked from the CategoryColor list. At least one
// category with color "grey" must be present in the category list.
color: CategoryColor,

// The list of subcategories. Must always have at least one element; subcategory
// zero must be the "Other" subcategory and is used to refer to the category itself.
subcategories: string[],
|};

Expand Down Expand Up @@ -739,10 +779,10 @@ export type ProfileMeta = {|
// The extensions property landed in Firefox 60, and is only optional because older
// processed profile versions may not have it. No upgrader was written for this change.
extensions?: ExtensionTable,
// The list of categories as provided by the platform. The categories are present for
// all Firefox profiles, but imported profiles may not include any category support.
// The front-end will provide a default list of categories, but the saved profile
// will not include them.
// The list of categories used in this profile. If present, it must contain at least the
// "default category" which is defined as the first category whose color is "grey" - this
// category usually has the name "Other".
// If meta.categories is not present, a default list is substituted.
categories?: CategoryList,
// The name of the product, most likely "Firefox".
product: 'Firefox' | string,
Expand Down