Skip to content

Commit

Permalink
Bug 1880809 - [devtools] Use binary search when adding sources in the…
Browse files Browse the repository at this point in the history
… source tree. r=devtools-reviewers,nchevobbe

This helps address a performance issue when a bulk or sources are registered.
We could have use findIndexLast, given that the sources are often coming sorted,
but this is not always the case. For example when a new arbitrary source is created by the page.

Also, while the sources look sorted, there is no actual sort being done from the Source Map Worker,
nor by the SOURCE server resource watcher. We would need to add sorting there to be reliable.

In this patch, I'm also avoiding unecessary array in the actor codebase, and, simplifying the load source map actions.

Differential Revision: https://phabricator.services.mozilla.com/D202149
  • Loading branch information
ochameau committed Mar 1, 2024
1 parent c759c48 commit 9fe319f
Show file tree
Hide file tree
Showing 6 changed files with 66 additions and 37 deletions.
40 changes: 20 additions & 20 deletions devtools/client/debugger/src/actions/sources/newSources.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,30 +36,22 @@ import { prefs } from "../../utils/prefs";
import sourceQueue from "../../utils/source-queue";
import { validateSourceActor, ContextError } from "../../utils/context";

function loadSourceMaps(sources) {
function loadSourceMapsForSourceActors(sourceActors) {
return async function ({ dispatch }) {
try {
const sourceList = await Promise.all(
sources.map(async sourceActor => {
const originalSourcesInfo = await dispatch(
loadSourceMap(sourceActor)
);
originalSourcesInfo.forEach(
sourcesInfo => (sourcesInfo.sourceActor = sourceActor)
);
sourceQueue.queueOriginalSources(originalSourcesInfo);
return originalSourcesInfo;
})
await Promise.all(
sourceActors.map(sourceActor => dispatch(loadSourceMap(sourceActor)))
);

await sourceQueue.flush();
return sourceList.flat();
} catch (error) {
// This may throw a context error if we navigated while processing the source maps
if (!(error instanceof ContextError)) {
throw error;
}
}
return [];

// Once all the source maps, of all the bulk of new source actors are processed,
// flush the SourceQueue. This help aggregate all the original sources in one action.
await sourceQueue.flush();
};
}

Expand All @@ -70,7 +62,7 @@ function loadSourceMaps(sources) {
function loadSourceMap(sourceActor) {
return async function ({ dispatch, getState, sourceMapLoader, panel }) {
if (!prefs.clientSourceMapsEnabled || !sourceActor.sourceMapURL) {
return [];
return;
}

let sources, ignoreListUrls, resolvedSourceMapURL, exception;
Expand Down Expand Up @@ -135,12 +127,20 @@ function loadSourceMap(sourceActor) {
type: "CLEAR_SOURCE_ACTOR_MAP_URL",
sourceActor,
});
return [];
return;
}

// Before dispatching this action, ensure that the related sourceActor is still registered
validateSourceActor(getState(), sourceActor);
return sources;

for (const originalSource of sources) {
// The Source Map worker doesn't set the `sourceActor` attribute,
// which is handy to know what is the related bundle.
originalSource.sourceActor = sourceActor;
}

// Register all the new reported original sources in the queue to be flushed once all new bundles are processed.
sourceQueue.queueOriginalSources(sources);
};
}

Expand Down Expand Up @@ -341,7 +341,7 @@ export function newGeneratedSources(sourceResources) {
await dispatch(checkNewSources(newSources));

(async () => {
await dispatch(loadSourceMaps(newSourceActors));
await dispatch(loadSourceMapsForSourceActors(newSourceActors));

// We would like to sync breakpoints after we are done
// loading source maps as sometimes generated and original
Expand Down
22 changes: 15 additions & 7 deletions devtools/client/debugger/src/reducers/sources-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ const IGNORED_EXTENSIONS = ["css", "svg", "png"];
import { isPretty, getRawSourceURL } from "../utils/source";
import { prefs } from "../utils/prefs";

const lazy = {};
ChromeUtils.defineESModuleGetters(lazy, {
BinarySearch: "resource://gre/modules/BinarySearch.sys.mjs",
});

export function initialSourcesTreeState() {
return {
// List of all Thread Tree Items.
Expand Down Expand Up @@ -224,8 +229,10 @@ function addThread(state, thread) {
// (this is also used by sortThreadItems to sort the thread as a Tree in the Browser Toolbox)
threadItem.thread = thread;

// We have to re-sort all threads because of the new `thread` attribute on current thread item
state.threadItems.sort(sortThreadItems);
// We have to remove and re-insert the thread as its order will be based on the newly set `thread` attribute
state.threadItems = [...state.threadItems];
state.threadItems.splice(state.threadItems.indexOf(threadItem), 1);
addSortedItem(state.threadItems, threadItem, sortThreadItems);
}
}

Expand Down Expand Up @@ -303,13 +310,12 @@ function isSourceVisibleInSourceTree(
* The already sorted into which a value should be added.
* @param {any} newValue
* The value to add in the array while keeping the array sorted.
* @param {Function} sortFunction
* @param {Function} comparator
* A function to compare two array values and their ordering.
* Follow same behavior as Array sorting function.
*/
function addSortedItem(array, newValue, sortFunction) {
let index = array.findIndex(value => sortFunction(value, newValue) === 1);
index = index >= 0 ? index : array.length;
function addSortedItem(array, newValue, comparator) {
const index = lazy.BinarySearch.insertionIndexOf(comparator, array, newValue);
array.splice(index, 0, newValue);
}

Expand Down Expand Up @@ -396,6 +402,8 @@ function sortItems(a, b) {
return -1;
} else if (b.type == "directory" && a.type == "source") {
return 1;
} else if (a.type == "group" && b.type == "group") {
return a.groupName.localeCompare(b.groupName);
} else if (a.type == "directory" && b.type == "directory") {
return a.path.localeCompare(b.path);
} else if (a.type == "source" && b.type == "source") {
Expand Down Expand Up @@ -445,7 +453,7 @@ function sortThreadItems(a, b) {
if (a.thread.processID > b.thread.processID) {
return 1;
} else if (a.thread.processID < b.thread.processID) {
return 0;
return -1;
}

// Order the frame targets and the worker targets by their target name
Expand Down
14 changes: 13 additions & 1 deletion devtools/client/shared/test-helpers/jest-fixtures/ChromeUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,21 @@

"use strict";

const mockedESM = {
BinarySearch: {
insertionIndexOf() {
return 0;
},
},
};

module.exports = {
import: () => ({}),
addProfilerMarker: () => {},
defineESModuleGetters: () => {},
defineESModuleGetters: (lazy, dict) => {
for (const key in dict) {
lazy[key] = mockedESM[key];
}
},
importESModule: () => ({}),
};
14 changes: 7 additions & 7 deletions devtools/server/actors/resources/sources.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,13 @@ class SourceWatcher {
// Before fetching all sources, process existing ones.
// The ThreadActor is already up and running before this code runs
// and have sources already registered and for which newSource event already fired.
onAvailable(
threadActor.sourcesManager.iter().map(s => {
const resource = s.form();
resource.resourceType = SOURCE;
return resource;
})
);
const sources = [];
for (const sourceActor of threadActor.sourcesManager.iter()) {
const resource = sourceActor.form();
resource.resourceType = SOURCE;
sources.push(resource);
}
onAvailable(sources);

// Requesting all sources should end up emitting newSource on threadActor.sourcesManager
threadActor.addAllSources();
Expand Down
6 changes: 5 additions & 1 deletion devtools/server/actors/thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -1538,7 +1538,11 @@ class ThreadActor extends Actor {
// overhead of an RDP packet for every source right now. Let the default
// timeout flush the buffered packets.

return this.sourcesManager.iter().map(s => s.form());
const forms = [];
for (const source of this.sourcesManager.iter()) {
forms.push(source.form());
}
return forms;
}

/**
Expand Down
7 changes: 6 additions & 1 deletion devtools/server/actors/utils/sources-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -341,8 +341,13 @@ class SourcesManager extends EventEmitter {
return this.blackBoxedSources.set(url, ranges);
}

/**
* List all currently registered source actors.
*
* @return Iterator<SourceActor>
*/
iter() {
return [...this._sourceActors.values()];
return this._sourceActors.values();
}

/**
Expand Down

0 comments on commit 9fe319f

Please sign in to comment.