From 8d873aa63b52cd9acfce594a0a86f1604a79cd57 Mon Sep 17 00:00:00 2001 From: flow Date: Sun, 28 May 2023 17:20:42 -0300 Subject: [PATCH 1/2] feat: optimize DHAT import tree building algorithm This commit changes the way in which the stackTable tree is constructed in DHAT's import algorithm. Previously, the algorithm would look a bit like this: For each PP: For each frame in the PP, from top to bottom: Loop over all existing stackTable entries, until we find the one we want, or go through all of them. If we found an entry, we are good. If not, create a new entry. [...] [...] This approach, while correct, isn't optimal, since we would potentially loop over a wide range of stackTable entries that couldn't possibly be the one we were looking for. With this commit, the algorithm becomes more or less like this: For each PP: For each frame in the PP, from top to bottom: Loop over the stackTable entries whose prefix is the same as the one we're looking for. If we found an entry, we are good. If not, create a new entry, also updating the list of entries with our prefix. [...] [...] With this, we avoid looking over a very big number of entries whose prefix are different from ours. The effect is that of switching from a linear search on a list to following pointers on a tree structure. The list of entries with the same prefix is implemented as a JS Map of prefixes to lists of indexes into the stackTable. With this change, big DHAT profiles take a lot less time to load (e.g. a 20MB profile would take ~2min before, now it takes ~2.5s :D). Signed-off-by: flow --- src/profile-logic/import/dhat.js | 48 ++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/src/profile-logic/import/dhat.js b/src/profile-logic/import/dhat.js index 314049af54..97d28108b3 100644 --- a/src/profile-logic/import/dhat.js +++ b/src/profile-logic/import/dhat.js @@ -7,6 +7,7 @@ import type { Pid, Bytes, IndexIntoFuncTable, + IndexIntoStackTable, } from 'firefox-profiler/types'; import { @@ -253,11 +254,18 @@ export function attemptToConvertDhat(json: mixed): Profile | null { const bytesAtGmax: Bytes[] = []; const endBytes: Bytes[] = []; + // Maps prefixes to their descendents (more specific prefixes). + const postfix: Map = + new Map(); + for (const pp of dhat.pps) { // Never reset the stackIndex, stack indexes always growing larger. - let stackIndex = -1; + let stackIndex = 0; let prefix = null; + // List of possible stack indexes to look for. + let candidateStackTables = postfix.get(null); + // Go from root to tip on the backtrace. for (let i = pp.fs.length - 1; i >= 0; i--) { // The dhat frame indexes matches the process profile frame index. @@ -267,20 +275,16 @@ export function attemptToConvertDhat(json: mixed): Profile | null { 'Expected to find a funcIndex from a frameIndex' ); - // Case 1: The stack index starts at -1, increment by 1 to start searching stacks - // at index 0. - // Case 2: This is the previously matched stack index, increment it by 1 to continue - // searching at the next stack index. - stackIndex++; - - // Start searching for a stack index. - for (; stackIndex < stackTable.length; stackIndex++) { - const nextFrameIndex = stackTable.frame[stackIndex]; - if ( - frameTable.func[nextFrameIndex] === funcIndex && - stackTable.prefix[stackIndex] === prefix - ) { - break; + if (candidateStackTables) { + // Start searching for a stack index. + stackIndex = stackTable.length; + for (const sliceStackIndex of candidateStackTables) { + const nextFrameIndex = stackTable.frame[sliceStackIndex]; + // No need to look for the prefix, since candidateStackTables already takes that into account. + if (frameTable.func[nextFrameIndex] === funcIndex) { + stackIndex = sliceStackIndex; + break; + } } } @@ -290,8 +294,22 @@ export function attemptToConvertDhat(json: mixed): Profile | null { stackTable.category.push(otherCategory); stackTable.category.push(otherSubCategory); stackTable.prefix.push(prefix); + + const candidateList = postfix.get(prefix); + if (candidateList) { + // Append us to the list of possible stack indexes of our parent. + candidateList.push(stackIndex); + } else { + // We are the first descendents of our parent. + postfix.set(prefix, [stackIndex]); + } + // The stack index already points to this spot. stackTable.length++; + // Since we just created a stack index, the next frames necessarily don't have an existing stack index. + candidateStackTables = []; + } else { + candidateStackTables = postfix.get(stackIndex); } prefix = stackIndex; From bed5b29072fd07a852cef4e49912963e172a3166 Mon Sep 17 00:00:00 2001 From: flow Date: Tue, 30 May 2023 18:06:00 -0300 Subject: [PATCH 2/2] refactor(DHAT): improve code readability of the stack tree builder This makes some misc. changes to make it a bit easier to understand the code added in the previous commit. This doesn't (i hope) change any behaviours! Signed-off-by: flow --- src/profile-logic/import/dhat.js | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/profile-logic/import/dhat.js b/src/profile-logic/import/dhat.js index 97d28108b3..14a1097d30 100644 --- a/src/profile-logic/import/dhat.js +++ b/src/profile-logic/import/dhat.js @@ -259,13 +259,9 @@ export function attemptToConvertDhat(json: mixed): Profile | null { new Map(); for (const pp of dhat.pps) { - // Never reset the stackIndex, stack indexes always growing larger. let stackIndex = 0; let prefix = null; - // List of possible stack indexes to look for. - let candidateStackTables = postfix.get(null); - // Go from root to tip on the backtrace. for (let i = pp.fs.length - 1; i >= 0; i--) { // The dhat frame indexes matches the process profile frame index. @@ -275,9 +271,14 @@ export function attemptToConvertDhat(json: mixed): Profile | null { 'Expected to find a funcIndex from a frameIndex' ); + // We want this to be the fallback, so that a stack index gets created when the 'if' below fails, + // or when we don't find a matching frame inside that loop. + stackIndex = stackTable.length; + + // List of possible stack indexes to look for. + const candidateStackTables = postfix.get(prefix); if (candidateStackTables) { // Start searching for a stack index. - stackIndex = stackTable.length; for (const sliceStackIndex of candidateStackTables) { const nextFrameIndex = stackTable.frame[sliceStackIndex]; // No need to look for the prefix, since candidateStackTables already takes that into account. @@ -295,10 +296,9 @@ export function attemptToConvertDhat(json: mixed): Profile | null { stackTable.category.push(otherSubCategory); stackTable.prefix.push(prefix); - const candidateList = postfix.get(prefix); - if (candidateList) { + if (candidateStackTables) { // Append us to the list of possible stack indexes of our parent. - candidateList.push(stackIndex); + candidateStackTables.push(stackIndex); } else { // We are the first descendents of our parent. postfix.set(prefix, [stackIndex]); @@ -306,10 +306,6 @@ export function attemptToConvertDhat(json: mixed): Profile | null { // The stack index already points to this spot. stackTable.length++; - // Since we just created a stack index, the next frames necessarily don't have an existing stack index. - candidateStackTables = []; - } else { - candidateStackTables = postfix.get(stackIndex); } prefix = stackIndex;