diff --git a/packages/common/src/utils.ts b/packages/common/src/utils.ts index e7053b181b..3837f2bdc8 100644 --- a/packages/common/src/utils.ts +++ b/packages/common/src/utils.ts @@ -735,6 +735,25 @@ export const arrayToList = (array: readonly T[]): Node[] => return acc; }, [] as Node[]); +/** + * Converts a readonly array or map into an iterable. + * Useful for avoiding entry allocations when iterating object / map on each iteration. + */ +export const toIterable = ( + values: readonly T[] | ReadonlyMap, +): Iterable => { + return Array.isArray(values) ? values : values.values(); +}; + +/** + * Converts a readonly array or map into an array. + */ +export const toArray = ( + values: readonly T[] | ReadonlyMap, +): T[] => { + return Array.from(toIterable(values)); +}; + export const isTestEnv = () => import.meta.env.MODE === ENV.TEST; export const isDevEnv = () => import.meta.env.MODE === ENV.DEVELOPMENT; diff --git a/packages/element/src/Scene.ts b/packages/element/src/Scene.ts index b35c54cae1..a31c962642 100644 --- a/packages/element/src/Scene.ts +++ b/packages/element/src/Scene.ts @@ -6,14 +6,13 @@ import { toBrandedType, isDevEnv, isTestEnv, - isReadonlyArray, + toArray, } from "@excalidraw/common"; import { isNonDeletedElement } from "@excalidraw/element"; import { isFrameLikeElement } from "@excalidraw/element/typeChecks"; import { getElementsInGroup } from "@excalidraw/element/groups"; import { - orderByFractionalIndex, syncInvalidIndices, syncMovedIndices, validateFractionalIndices, @@ -268,19 +267,13 @@ class Scene { } replaceAllElements(nextElements: ElementsMapOrArray) { - // ts doesn't like `Array.isArray` of `instanceof Map` - if (!isReadonlyArray(nextElements)) { - // need to order by fractional indices to get the correct order - nextElements = orderByFractionalIndex( - Array.from(nextElements.values()) as OrderedExcalidrawElement[], - ); - } - + // we do trust the insertion order on the map, though maybe we shouldn't and should prefer order defined by fractional indices + const _nextElements = toArray(nextElements); const nextFrameLikes: ExcalidrawFrameLikeElement[] = []; - validateIndicesThrottled(nextElements); + validateIndicesThrottled(_nextElements); - this.elements = syncInvalidIndices(nextElements); + this.elements = syncInvalidIndices(_nextElements); this.elementsMap.clear(); this.elements.forEach((element) => { if (isFrameLikeElement(element)) { diff --git a/packages/element/src/delta.ts b/packages/element/src/delta.ts index a58a75e6c5..2499f7d66c 100644 --- a/packages/element/src/delta.ts +++ b/packages/element/src/delta.ts @@ -316,7 +316,7 @@ export class Delta { } /** - * Returns all the object1 keys that have distinct values. + * Returns sorted object1 keys that have distinct values. */ public static getLeftDifferences( object1: T, @@ -325,11 +325,11 @@ export class Delta { ) { return Array.from( this.distinctKeysIterator("left", object1, object2, skipShallowCompare), - ); + ).sort(); } /** - * Returns all the object2 keys that have distinct values. + * Returns sorted object2 keys that have distinct values. */ public static getRightDifferences( object1: T, @@ -338,7 +338,7 @@ export class Delta { ) { return Array.from( this.distinctKeysIterator("right", object1, object2, skipShallowCompare), - ); + ).sort(); } /** @@ -430,7 +430,8 @@ export class AppStateDelta implements DeltaContainer { const delta = Delta.calculate( prevAppState, nextAppState, - undefined, + // making the order of keys in deltas stable for hashing purposes + AppStateDelta.orderAppStateKeys, AppStateDelta.postProcess, ); @@ -539,40 +540,6 @@ export class AppStateDelta implements DeltaContainer { return Delta.isEmpty(this.delta); } - /** - * It is necessary to post process the partials in case of reference values, - * for which we need to calculate the real diff between `deleted` and `inserted`. - */ - private static postProcess( - deleted: Partial, - inserted: Partial, - ): [Partial, Partial] { - try { - Delta.diffObjects( - deleted, - inserted, - "selectedElementIds", - // ts language server has a bit trouble resolving this, so we are giving it a little push - (_) => true as ValueOf, - ); - Delta.diffObjects( - deleted, - inserted, - "selectedGroupIds", - (prevValue) => (prevValue ?? false) as ValueOf, - ); - } catch (e) { - // if postprocessing fails it does not make sense to bubble up, but let's make sure we know about it - console.error(`Couldn't postprocess appstate change deltas.`); - - if (isTestEnv() || isDevEnv()) { - throw e; - } - } finally { - return [deleted, inserted]; - } - } - /** * Mutates `nextAppState` be filtering out state related to deleted elements. * @@ -807,6 +774,51 @@ export class AppStateDelta implements DeltaContainer { ObservedElementsAppState >; } + + /** + * It is necessary to post process the partials in case of reference values, + * for which we need to calculate the real diff between `deleted` and `inserted`. + */ + private static postProcess( + deleted: Partial, + inserted: Partial, + ): [Partial, Partial] { + try { + Delta.diffObjects( + deleted, + inserted, + "selectedElementIds", + // ts language server has a bit trouble resolving this, so we are giving it a little push + (_) => true as ValueOf, + ); + Delta.diffObjects( + deleted, + inserted, + "selectedGroupIds", + (prevValue) => (prevValue ?? false) as ValueOf, + ); + } catch (e) { + // if postprocessing fails it does not make sense to bubble up, but let's make sure we know about it + console.error(`Couldn't postprocess appstate change deltas.`); + + if (isTestEnv() || isDevEnv()) { + throw e; + } + } finally { + return [deleted, inserted]; + } + } + + private static orderAppStateKeys(partial: Partial) { + const orderedPartial: { [key: string]: unknown } = {}; + + for (const key of Object.keys(partial).sort()) { + // relying on insertion order + orderedPartial[key] = partial[key as keyof ObservedAppState]; + } + + return orderedPartial as Partial; + } } type ElementPartial = Omit< diff --git a/packages/element/src/index.ts b/packages/element/src/index.ts index d7edec8ae9..eafa609c43 100644 --- a/packages/element/src/index.ts +++ b/packages/element/src/index.ts @@ -1,3 +1,5 @@ +import { toIterable } from "@excalidraw/common"; + import { isInvisiblySmallElement } from "./sizeHelpers"; import { isLinearElementType } from "./typeChecks"; @@ -5,6 +7,7 @@ import type { ExcalidrawElement, NonDeletedExcalidrawElement, NonDeleted, + ElementsMapOrArray, } from "./types"; /** @@ -16,12 +19,10 @@ export const getSceneVersion = (elements: readonly ExcalidrawElement[]) => /** * Hashes elements' versionNonce (using djb2 algo). Order of elements matters. */ -export const hashElementsVersion = ( - elements: readonly ExcalidrawElement[], -): number => { +export const hashElementsVersion = (elements: ElementsMapOrArray): number => { let hash = 5381; - for (let i = 0; i < elements.length; i++) { - hash = (hash << 5) + hash + elements[i].versionNonce; + for (const element of toIterable(elements)) { + hash = (hash << 5) + hash + element.versionNonce; } return hash >>> 0; // Ensure unsigned 32-bit integer }; diff --git a/packages/element/src/store.ts b/packages/element/src/store.ts index 74284ea1c3..19ba1a286c 100644 --- a/packages/element/src/store.ts +++ b/packages/element/src/store.ts @@ -6,6 +6,7 @@ import { isTestEnv, randomId, Emitter, + toIterable, } from "@excalidraw/common"; import type { DTO, ValueOf } from "@excalidraw/common/utility-types"; @@ -158,25 +159,25 @@ export class Store { ): SceneElementsMap { const movedElements = new Map(); - for (const [id, prevElement] of prevElements.entries()) { - const nextElement = nextElements.get(id); + for (const prevElement of toIterable(prevElements)) { + const nextElement = nextElements.get(prevElement.id); if (!nextElement) { // Nothing to care about here, element was forcefully deleted continue; } - const elementSnapshot = this.snapshot.elements.get(id); + const elementSnapshot = this.snapshot.elements.get(prevElement.id); // Checks for in progress async user action if (!elementSnapshot) { // Detected yet uncomitted local element - nextElements.delete(id); + nextElements.delete(prevElement.id); } else if (elementSnapshot.version < prevElement.version) { // Element was already commited, but the snapshot version is lower than current local version - nextElements.set(id, elementSnapshot); + nextElements.set(prevElement.id, elementSnapshot); // Mark the element as potentially moved, as it could have - movedElements.set(id, elementSnapshot); + movedElements.set(prevElement.id, elementSnapshot); } } @@ -601,10 +602,10 @@ export class StoreSnapshot { public getChangedElements(prevSnapshot: StoreSnapshot) { const changedElements: Record = {}; - for (const [id, nextElement] of this.elements.entries()) { + for (const nextElement of toIterable(this.elements)) { // Due to the structural clone inside `maybeClone`, we can perform just these reference checks - if (prevSnapshot.elements.get(id) !== nextElement) { - changedElements[id] = nextElement; + if (prevSnapshot.elements.get(nextElement.id) !== nextElement) { + changedElements[nextElement.id] = nextElement; } } @@ -794,26 +795,26 @@ export class StoreSnapshot { const changedElements: SceneElementsMap = new Map() as SceneElementsMap; - for (const [id, prevElement] of this.elements) { - const nextElement = nextElements.get(id); + for (const prevElement of toIterable(this.elements)) { + const nextElement = nextElements.get(prevElement.id); if (!nextElement) { // element was deleted changedElements.set( - id, + prevElement.id, newElementWith(prevElement, { isDeleted: true }), ); } } - for (const [id, nextElement] of nextElements) { - const prevElement = this.elements.get(id); + for (const nextElement of toIterable(nextElements)) { + const prevElement = this.elements.get(nextElement.id); if ( !prevElement || // element was added prevElement.version < nextElement.version // element was updated ) { - changedElements.set(id, nextElement); + changedElements.set(nextElement.id, nextElement); } } @@ -821,9 +822,7 @@ export class StoreSnapshot { return; } - const changedElementsHash = hashElementsVersion( - Array.from(changedElements.values()), - ); + const changedElementsHash = hashElementsVersion(changedElements); if ( options.shouldCompareHashes && @@ -843,15 +842,15 @@ export class StoreSnapshot { private createElementsSnapshot(changedElements: SceneElementsMap) { const clonedElements = new Map() as SceneElementsMap; - for (const [id, prevElement] of this.elements) { + for (const prevElement of toIterable(this.elements)) { // Clone previous elements, never delete, in case nextElements would be just a subset of previous elements // i.e. during collab, persist or whenenever isDeleted elements get cleared - clonedElements.set(id, prevElement); + clonedElements.set(prevElement.id, prevElement); } - for (const [id, changedElement] of changedElements) { + for (const changedElement of toIterable(changedElements)) { // TODO: consider just creating new instance, once we can ensure that all reference properties on every element are immutable - clonedElements.set(id, deepCopyElement(changedElement)); + clonedElements.set(changedElement.id, deepCopyElement(changedElement)); } return clonedElements; diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 7f958a51da..5dfb1ba052 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -307,7 +307,6 @@ import Scene from "@excalidraw/element/Scene"; import { Store, CaptureUpdateAction, - StoreIncrement, } from "@excalidraw/element/store"; import type { ElementUpdate } from "@excalidraw/element/mutateElement";