Skip to content

Replace O(N) blockList based lookups with O(1) blockIndex property #5798

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.

Sign up for GitHub

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

Open
Ashutoshx7 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Replace O(N) blockList based lookups with O(1) blockIndex property #5798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion js/activity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7133,7 +7133,7 @@ class Activity {
pasteDy = 0;
const map = new Map();
for (let i = 0; i < blocksArray.length; i++) {
const idx = this.blocks.blockList.indexOf(blocksArray[i]);
const idx = blocksArray[i].blockIndex;
map.set(
idx,
blocksArray[i].connections.filter(blk => blk !== null)
Expand Down
119 changes: 78 additions & 41 deletions js/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,11 @@ class Block {
this.name = protoblock.name;
this.activity = this.blocks.activity;

// Cached index in blocks.blockList — set when block is added
// to blockList. Avoids O(N) blockList.indexOf() lookups that
// are scattered across the codebase (33+ call sites).
this.blockIndex = -1;

this.collapsed = false; // Is this collapsible block collapsed?
this.inCollapsed = false; // Is this block in a collapsed stack?
this.trash = false; // Is this block in the trash?
Expand Down Expand Up @@ -1039,7 +1044,7 @@ class Block {
generateArtwork(firstTime) {
// Get the block labels from the protoblock.
const that = this;
const thisBlock = this.blocks.blockList.indexOf(this);
const thisBlock = this.blockIndex;
let block_label = "";

/**
Expand Down Expand Up @@ -1071,7 +1076,7 @@ class Block {

const __callback = (that, firstTime) => {
that.activity.refreshCanvas();
const thisBlock = that.blocks.blockList.indexOf(that);
const thisBlock = that.blockIndex;

if (firstTime) {
that._loadEventHandlers();
Expand Down Expand Up @@ -1289,7 +1294,7 @@ class Block {
artwork = artwork.replace("arg_label_" + i, this.protoblock.staticLabels[i]);
}

that.blocks.blockArt[that.blocks.blockList.indexOf(that)] = artwork;
that.blocks.blockArt[that.blockIndex] = artwork;

_blockMakeBitmap(artwork, __processBitmap, this);
}
Expand All @@ -1300,7 +1305,7 @@ class Block {
* @returns {void}
*/
_finishImageLoad() {
// const thisBlock = this.blocks.blockList.indexOf(this);
// const thisBlock = this.blockIndex;
let proto, obj, label, attr;
// Value blocks get a modifiable text label.
if (SPECIALINPUTS.includes(this.name)) {
Expand Down Expand Up @@ -1490,7 +1495,7 @@ class Block {
* @returns {void}
*/
_generateCollapseArtwork(postProcess) {
const thisBlock = this.blocks.blockList.indexOf(this);
const thisBlock = this.blockIndex;

/**
* Run the postprocess function after the artwork is loaded.
Expand Down Expand Up @@ -1746,7 +1751,7 @@ class Block {
that._ensureDecorationOnTop();

// Save the collapsed block artwork for export.
that.blocks.blockCollapseArt[that.blocks.blockList.indexOf(that)] = that.collapseArtwork
that.blocks.blockCollapseArt[that.blockIndex] = that.collapseArtwork
.replace(/fill_color/g, PALETTEFILLCOLORS[that.protoblock.palette.name])
.replace(/stroke_color/g, PALETTESTROKECOLORS[that.protoblock.palette.name])
.replace("block_label", safeSVG(that.collapseText.text));
Expand Down Expand Up @@ -2076,7 +2081,7 @@ class Block {
*/
getBlockId() {
// Generate a UID based on the block index into the blockList.
const number = blockBlocks.blockList.indexOf(this);
const number = this.blockIndex;
return "_" + number.toString();
}

Expand All @@ -2099,7 +2104,7 @@ class Block {
*/
loadThumbnail(imagePath) {
// Load an image thumbnail onto block.
const thisBlock = this.blocks.blockList.indexOf(this);
const thisBlock = this.blockIndex;
const that = this;

if (this.blocks.blockList[thisBlock].value === null && imagePath === null) {
Expand Down Expand Up @@ -2236,7 +2241,7 @@ class Block {
collapseToggle() {
// Find the blocks to collapse/expand inside of a collapable
// block.
const thisBlock = this.blocks.blockList.indexOf(this);
const thisBlock = this.blockIndex;
this.blocks.findDragGroup(thisBlock);

if (this.collapseBlockBitmap === null) {
Expand Down Expand Up @@ -2355,7 +2360,7 @@ class Block {
const intervals = [];
let i = 0;

let c = this.blocks.blockList.indexOf(this),
let c = this.blockIndex,
lastIntervalBlock;
while (c !== null) {
lastIntervalBlock = c;
Expand Down Expand Up @@ -2868,7 +2873,7 @@ class Block {
*/
_loadEventHandlers() {
const that = this;
const thisBlock = this.blocks.blockList.indexOf(this);
const thisBlock = this.blockIndex;

this._calculateBlockHitArea();

Expand All @@ -2890,6 +2895,12 @@ class Block {
let locked = false;
let getInput = window.hasMouse;

// Cached drag state: computed once at mousedown, reused during pressmove.
// This avoids redundant O(N) findDragGroup and O(D) rest2 chain walks
// on every mouse move event (which fires 60+ times per second).
let _cachedDragGroup = null;
let _dragHasRest2 = false;

/**
* Handles the click event on the block container.
* @param {Event} event - The click event.
Expand Down Expand Up @@ -3012,7 +3023,7 @@ class Block {
that.blocks.mouseDownTime = new Date().getTime();

that.blocks.longPressTimeout = setTimeout(() => {
that.blocks.activeBlock = that.blocks.blockList.indexOf(that);
that.blocks.activeBlock = that.blockIndex;
that._triggerLongPress = true;
that.blocks.triggerLongPress();
}, LONGPRESSTIME);
Expand Down Expand Up @@ -3056,6 +3067,26 @@ class Block {
x: Math.round(that.container.x - that.original.x),
y: Math.round(that.container.y - that.original.y)
};

// Pre-compute the drag group and rest2 check once at mousedown.
// The drag group doesn't change during a drag (blocks only
// connect/disconnect on mouseup), so caching it avoids
// redundant O(N) recursive traversal on every mouse move.
that.blocks.findDragGroup(thisBlock);
_cachedDragGroup = that.blocks.dragGroup.slice();

// Pre-compute whether the chain below contains a rest2 block.
// This avoids walking the entire connection chain on every
// mouse move event.
_dragHasRest2 = false;
let checkBlock = that.blocks.blockList[that.connections[1]];
while (checkBlock != null) {
if (checkBlock?.name === "rest2") {
_dragHasRest2 = true;
break;
}
checkBlock = checkBlock?.blocks.blockList[checkBlock.connections[1]];
}
});

/**
Expand All @@ -3079,14 +3110,11 @@ class Block {
return;
}

// Do not allow a stack of blocks to be dragged if the stack contains a silence block.
let block = that.blocks.blockList[that.connections[1]];
while (block != null) {
if (block?.name === "rest2") {
this.activity.errorMsg(_("Silence block cannot be removed."), block);
return;
}
block = block?.blocks.blockList[block.connections[1]];
// Use the cached rest2 check from mousedown instead of
// walking the chain on every mouse move event.
if (_dragHasRest2) {
this.activity.errorMsg(_("Silence block cannot be removed."), that);
return;
}

if (window.hasMouse) {
Expand Down Expand Up @@ -3138,6 +3166,10 @@ class Block {
that.label.style.display = "none";
}

// Defer checkBounds during the entire block movement to avoid
// N separate checkBounds calls (one per block in the drag group).
that.blocks._beginDeferCheckBounds();

that.blocks.moveBlockRelative(thisBlock, dx, dy);

// If we are over the trash, warn the user.
Expand All @@ -3157,17 +3189,19 @@ class Block {
that.container.setChildIndex(that.text, that.container.children.length - 1);
}

// ...and move any connected blocks.
that.blocks.findDragGroup(thisBlock);
if (that.blocks.dragGroup.length > 0) {
for (let b = 0; b < that.blocks.dragGroup.length; b++) {
const blk = that.blocks.dragGroup[b];
// Move connected blocks using the cached drag group from
// mousedown, avoiding redundant O(N) recursive traversal.
if (_cachedDragGroup !== null && _cachedDragGroup.length > 0) {
for (let b = 0; b < _cachedDragGroup.length; b++) {
const blk = _cachedDragGroup[b];
if (b !== 0) {
that.blocks.moveBlockRelative(blk, dx, dy);
}
}
}

that.blocks._endDeferCheckBounds();

that.activity.refreshCanvas();
});

Expand All @@ -3191,6 +3225,9 @@ class Block {
}
that.blocks.activeBlock = null;

// Clear cached drag state.
_cachedDragGroup = null;
_dragHasRest2 = false;
moved = false;
});

Expand All @@ -3213,6 +3250,9 @@ class Block {
that.blocks.unhighlight(thisBlock, true);
that.blocks.activeBlock = null;

// Clear cached drag state.
_cachedDragGroup = null;
_dragHasRest2 = false;
moved = false;
});
}
Expand All @@ -3228,7 +3268,7 @@ class Block {
* @returns {void}
*/
_mouseoutCallback(event, moved, haveClick, hideDOM) {
const thisBlock = this.blocks.blockList.indexOf(this);
const thisBlock = this.blockIndex;
if (!this.activity.logo.runningLilypond) {
document.body.style.cursor = "default";
}
Expand Down Expand Up @@ -3270,7 +3310,7 @@ class Block {
// Just in case the blocks are not properly docked after
// the move (workaround for issue #38 -- Blocks fly
// apart). Still need to get to the root cause.
this.blocks.adjustDocks(this.blocks.blockList.indexOf(this), true);
this.blocks.adjustDocks(this.blockIndex, true);
}
} else if (SPECIALINPUTS.includes(this.name) || ["media", "loadFile"].includes(this.name)) {
if (!haveClick) {
Expand Down Expand Up @@ -3328,7 +3368,7 @@ class Block {
}

// Numeric pie menus
const blk = this.blocks.blockList.indexOf(this);
const blk = this.blockIndex;

if (this.blocks.octaveNumber(blk)) {
return true;
Expand Down Expand Up @@ -3385,7 +3425,7 @@ class Block {
return false;
}

return this.blocks.blockList[cblk].connections[1] === this.blocks.blockList.indexOf(this);
return this.blocks.blockList[cblk].connections[1] === this.blockIndex;
}

/**
Expand All @@ -3407,7 +3447,7 @@ class Block {
return false;
}

return this.blocks.blockList[cblk].connections[2] === this.blocks.blockList.indexOf(this);
return this.blocks.blockList[cblk].connections[2] === this.blockIndex;
}

/**
Expand All @@ -3429,7 +3469,7 @@ class Block {
return false;
}

return this.blocks.blockList[cblk].connections[3] === this.blocks.blockList.indexOf(this);
return this.blocks.blockList[cblk].connections[3] === this.blockIndex;
}

/**
Expand Down Expand Up @@ -3976,7 +4016,7 @@ class Block {
} else {
// If the number block is connected to a pitch block, then
// use the pie menu for octaves. Other special cases as well.
const blk = this.blocks.blockList.indexOf(this);
const blk = this.blockIndex;
if (this.blocks.octaveNumber(blk)) {
piemenuNumber(this, [8, 7, 6, 5, 4, 3, 2, 1], this.value);
} else if (this.blocks.noteValueNumber(blk, 2)) {
Expand Down Expand Up @@ -4124,7 +4164,7 @@ class Block {
}
}

// const blk = this.blocks.blockList.indexOf(this);
// const blk = this.blockIndex;
if (!this._usePiemenu()) {
let focused = false;

Expand Down Expand Up @@ -4257,7 +4297,7 @@ class Block {
_checkWidgets(closeInput) {
// Detect if label is changed, then reinit widget windows
// if they are open.
const thisBlock = this.blocks.blockList.indexOf(this);
const thisBlock = this.blockIndex;
const topBlock = this.blocks.findTopBlock(thisBlock);
const widgetTitle = document.getElementsByClassName("wftTitle");
let lockInit = false;
Expand Down Expand Up @@ -4436,7 +4476,7 @@ class Block {
}

if (isNaN(this.value)) {
const thisBlock = this.blocks.blockList.indexOf(this);
const thisBlock = this.blockIndex;
this.activity.errorMsg(newValue + ": " + _("Not a number"), thisBlock);
this.activity.refreshCanvas();
this.value = oldValue;
Expand All @@ -4447,15 +4487,15 @@ class Block {
this.blocks.blockList[cblk1].name === "pitch" &&
(this.value > 8 || this.value < 1)
) {
const thisBlock = this.blocks.blockList.indexOf(this);
const thisBlock = this.blockIndex;
this.activity.errorMsg(_("Octave value must be between 1 and 8."), thisBlock);
this.activity.refreshCanvas();
this.label.value = oldValue;
this.value = oldValue;
}

if (String(this.value).length > 10) {
const thisBlock = this.blocks.blockList.indexOf(this);
const thisBlock = this.blockIndex;
this.activity.errorMsg(_("Numbers can have at most 10 digits."), thisBlock);
this.activity.refreshCanvas();
this.label.value = oldValue;
Expand Down Expand Up @@ -4563,10 +4603,7 @@ class Block {
// Check to see which connection we are using in
// cblock. We only do something if blk is attached to
// the name connection (1).
if (
cblock.connections[1] === this.blocks.blockList.indexOf(this) &&
closeInput
) {
if (cblock.connections[1] === this.blockIndex && closeInput) {
// If the label was the name of a storein, update the
// associated box this.blocks and the palette buttons.
if (this.value !== "box") {
Expand Down
Loading
Loading