From 20005eecdbede50322f82206487c91fdc1660632 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Sun, 28 Apr 2019 17:20:35 +0200 Subject: [PATCH] Merge deleted items more efficiently. Previously deleted items were simply added to transaction._mergeStructs. But this inherently inefficient as it will splice the struct store for every item. Now Yjs iterates over transaction.ds and tries to merge structs. It iterates from right to left so merging should be more efficient that before. But more work needs to be done. For example we could set structs[i] = null and filter the structs after merging is done. --- src/structs/AbstractItem.js | 9 +++-- src/structs/ItemType.js | 13 ++++++- src/utils/Transaction.js | 69 ++++++++++++++++++++++++++----------- 3 files changed, 67 insertions(+), 24 deletions(-) diff --git a/src/structs/AbstractItem.js b/src/structs/AbstractItem.js index 614fb4ef..a27b0de7 100644 --- a/src/structs/AbstractItem.js +++ b/src/structs/AbstractItem.js @@ -423,6 +423,8 @@ export class AbstractItem extends AbstractStruct { gcChildren (transaction, store) { } /** + * @todo remove transaction param + * * @param {Transaction} transaction * @param {StructStore} store * @param {boolean} parentGCd @@ -430,7 +432,9 @@ export class AbstractItem extends AbstractStruct { * @private */ gc (transaction, store, parentGCd) { - this.delete(transaction) // @todo: shouldn't be necessary + if (!this.deleted) { + throw error.unexpectedCase() + } let r if (parentGCd) { r = new GC(this.id, this.length) @@ -448,7 +452,6 @@ export class AbstractItem extends AbstractStruct { } } replaceStruct(store, this, r) - transaction._mergeStructs.add(r.id) } /** @@ -616,7 +619,7 @@ export const computeItemParams = (transaction, store, leftid, rightid, parentid, case GC: break default: - if (!parentItem.deleted) { + if (!parentItem.deleted && (left === null || left.constructor !== GC) && (right === null || right.constructor !== GC)) { parent = parentItem.type } } diff --git a/src/structs/ItemType.js b/src/structs/ItemType.js index 57ee6669..bdf648b0 100644 --- a/src/structs/ItemType.js +++ b/src/structs/ItemType.js @@ -106,11 +106,22 @@ export class ItemType extends AbstractItem { while (item !== null) { if (!item.deleted) { item.delete(transaction) + } else { + // Whis will be gc'd later and we want to merge it if possible + // We try to merge all deleted items after each transaction, + // but we have no knowledge about that this needs to be merged + // since it is not in transaction.ds. Hence we add it to transaction._mergeStructs + transaction._mergeStructs.add(item.id) } item = item.right } this.type._map.forEach(item => { - item.delete(transaction) + if (!item.deleted) { + item.delete(transaction) + } else { + // same as above + transaction._mergeStructs.add(item.id) + } }) transaction.changed.delete(this.type) transaction.changedParentTypes.delete(this.type) diff --git a/src/utils/Transaction.js b/src/utils/Transaction.js index 26d33c7a..41c9cb87 100644 --- a/src/utils/Transaction.js +++ b/src/utils/Transaction.js @@ -176,26 +176,6 @@ export const transact = (y, f, origin = null) => { callEventHandlerListeners(type._dEH, events, transaction) }) y.emit('afterTransaction', [transaction, y]) - // replace deleted items with ItemDeleted / GC - for (const [client, deleteItems] of ds.clients) { - /** - * @type {Array} - */ - // @ts-ignore - const structs = store.clients.get(client) - for (let di = 0; di < deleteItems.length; di++) { - const deleteItem = deleteItems[di] - for (let si = findIndexSS(structs, deleteItem.clock); si < structs.length; si++) { - const struct = structs[si] - if (deleteItem.clock + deleteItem.len <= struct.id.clock) { - break - } - if (struct.deleted && struct instanceof AbstractItem) { - struct.gc(transaction, store, false) - } - } - } - } /** * @param {Array} structs * @param {number} pos @@ -213,6 +193,53 @@ export const transact = (y, f, origin = null) => { } } } + // replace deleted items with ItemDeleted / GC + for (const [client, deleteItems] of ds.clients) { + /** + * @type {Array} + */ + // @ts-ignore + const structs = store.clients.get(client) + for (let di = deleteItems.length - 1; di >= 0; di--) { + const deleteItem = deleteItems[di] + const endDeleteItemClock = deleteItem.clock + deleteItem.len + for ( + let si = findIndexSS(structs, deleteItem.clock), struct = structs[si]; + si < structs.length && struct.id.clock < endDeleteItemClock; + struct = structs[++si] + ) { + const struct = structs[si] + if (deleteItem.clock + deleteItem.len <= struct.id.clock) { + break + } + if (struct.deleted && struct instanceof AbstractItem) { + struct.gc(transaction, store, false) + } + } + } + } + // try to merge deleted / gc'd items + // merge from right to left for better efficiecy and so we don't miss any merge targets + for (const [client, deleteItems] of ds.clients) { + /** + * @type {Array} + */ + // @ts-ignore + const structs = store.clients.get(client) + for (let di = deleteItems.length - 1; di >= 0; di--) { + const deleteItem = deleteItems[di] + // start with merging the item next to the last deleted item + const mostRightIndexToCheck = math.min(structs.length - 1, 1 + findIndexSS(structs, deleteItem.clock + deleteItem.len - 1)) + for ( + let si = mostRightIndexToCheck, struct = structs[si]; + si > 0 && struct.id.clock >= deleteItem.clock; + struct = structs[--si] + ) { + tryToMergeWithLeft(structs, si) + } + } + } + // on all affected store.clients props, try to merge for (const [client, clock] of transaction.afterState) { const beforeClock = transaction.beforeState.get(client) || 0 @@ -230,6 +257,8 @@ export const transact = (y, f, origin = null) => { } } // try to merge mergeStructs + // @todo: it makes more sense to transform mergeStructs to a DS, sort it, and merge from right to left + // but at the moment DS does not handle duplicates for (const mid of transaction._mergeStructs) { const client = mid.client const clock = mid.clock