From 9fe47e98d5b1f8a26b28f96593447a55c36881b8 Mon Sep 17 00:00:00 2001 From: Kevin Jahns Date: Wed, 10 Apr 2019 21:01:59 +0200 Subject: [PATCH] type._map points to the last element instead to enable merging of deletes in Map --- src/structs/AbstractItem.js | 58 ++++++++++++++++++------------------- src/structs/ItemType.js | 42 +++++++-------------------- src/types/AbstractType.js | 16 +++++----- src/utils/Transaction.js | 5 ++++ tests/y-map.tests.js | 10 +++---- 5 files changed, 58 insertions(+), 73 deletions(-) diff --git a/src/structs/AbstractItem.js b/src/structs/AbstractItem.js index 12b92eb6..502e9767 100644 --- a/src/structs/AbstractItem.js +++ b/src/structs/AbstractItem.js @@ -140,6 +140,9 @@ export class AbstractItem extends AbstractStruct { o = this.left.right } else if (parentSub !== null) { o = parent._map.get(parentSub) || null + while (o !== null && o.left !== null) { + o = o.left + } } else { o = parent._start } @@ -180,39 +183,39 @@ export class AbstractItem extends AbstractStruct { const right = this.left.right this.right = right this.left.right = this - if (right !== null) { - right.left = this - } } else { let r if (parentSub !== null) { - const pmap = parent._map - r = pmap.get(parentSub) || null - pmap.set(parentSub, this) + r = parent._map.get(parentSub) || null + while (r !== null && r.left !== null) { + r = r.left + } } else { r = parent._start parent._start = this } this.right = r - if (r !== null) { - r.left = this + } + if (this.right !== null) { + this.right.left = this + } else if (parentSub !== null) { + // set as current parent value if right === null and this is parentSub + parent._map.set(parentSub, this) + if (this.left !== null) { + // this is the current attribute value of parent. delete right + this.left.delete(transaction) } } - // adjust the length of parent + // adjust length of parent if (parentSub === null && this.countable && !this.deleted) { parent._length += length } addStruct(store, this) - if (parent !== null) { - maplib.setIfUndefined(transaction.changed, parent, set.create).add(parentSub) - } + maplib.setIfUndefined(transaction.changed, parent, set.create).add(parentSub) // @ts-ignore - if ((parent._item !== null && parent._item.deleted) || (this.left !== null && parentSub !== null)) { + if ((parent._item !== null && parent._item.deleted) || (this.right !== null && parentSub !== null)) { // delete if parent is deleted or if this is not the current attribute value of parent this.delete(transaction) - } else if (parentSub !== null && this.left === null && this.right !== null) { - // this is the current attribute value of parent. delete right - this.right.delete(transaction) } } @@ -281,9 +284,9 @@ export class AbstractItem extends AbstractStruct { left = this.left right = this } else { - // Is a map item. Insert at the start - left = null - right = parent.type._map.get(this.parentSub) + // Is a map item. Insert as current value + left = parent.type._map.get(this.parentSub) + right = null } // make sure that parent is redone if (parent._deleted === true && parent.redone === null) { @@ -308,7 +311,7 @@ export class AbstractItem extends AbstractStruct { if (right.redone !== null && right.redone.parent === parent) { right = right.redone } - right = right._right + right = right.right } } this.redone = this.copy(nextID(transaction), left, left === null ? null : left.lastId, right, right === null ? null : right.id, parent, this.parentSub) @@ -411,18 +414,15 @@ export class AbstractItem extends AbstractStruct { r = new GC(this.id, this.length) } else { r = new ItemDeleted(this.id, this.left, this.origin, this.right, this.rightOrigin, this.parent, this.parentSub, this.length) - if (r.left !== null) { - r.left.right = r - } if (r.right !== null) { r.right.left = r + } else if (r.parentSub !== null) { + r.parent._map.set(r.parentSub, r) } - if (r.left === null) { - if (r.parentSub === null) { - r.parent._start = r - } else { - r.parent._map.set(r.parentSub, r) - } + if (r.left !== null) { + r.left.right = r + } else if (r.parentSub === null) { + r.parent._start = r } } replaceStruct(store, this, r) diff --git a/src/structs/ItemType.js b/src/structs/ItemType.js index 53d76c99..8d16016a 100644 --- a/src/structs/ItemType.js +++ b/src/structs/ItemType.js @@ -21,18 +21,6 @@ import { import * as encoding from 'lib0/encoding.js' // eslint-disable-line import * as decoding from 'lib0/decoding.js' -/** - * @param {Transaction} transaction - * @param {StructStore} store - * @param {AbstractItem | null} item - */ -const gcChildren = (transaction, store, item) => { - while (item !== null) { - item.gc(transaction, store) - item = item.right - } -} - export const structTypeRefNumber = 7 /** @@ -113,23 +101,7 @@ export class ItemType extends AbstractItem { super.delete(transaction) transaction.changed.delete(this.type) transaction.changedParentTypes.delete(this.type) - // delete map types - for (let value of this.type._map.values()) { - if (!value.deleted) { - value.delete(transaction) - } - } - // delete array types - let t = this.type._start - while (t !== null) { - if (!t.deleted) { - t.delete(transaction) - } - t = t.right - } - if (gcChildren) { - this.gcChildren(transaction, transaction.y.store) - } + this.gcChildren(transaction, transaction.y.store) } /** @@ -137,10 +109,18 @@ export class ItemType extends AbstractItem { * @param {StructStore} store */ gcChildren (transaction, store) { - gcChildren(transaction, store, this.type._start) + let item = this.type._start + while (item !== null) { + item.gc(transaction, store) + item = item.right + } this.type._start = null this.type._map.forEach(item => { - gcChildren(transaction, store, item) + while (item !== null) { + item.gc(transaction, store) + // @ts-ignore + item = item.left + } }) this._map = new Map() } diff --git a/src/types/AbstractType.js b/src/types/AbstractType.js index 5260ff67..e39f36fe 100644 --- a/src/types/AbstractType.js +++ b/src/types/AbstractType.js @@ -447,9 +447,9 @@ export const typeMapDelete = (transaction, parent, key) => { * @param {Object|number|Array|string|ArrayBuffer|AbstractType} value */ export const typeMapSet = (transaction, parent, key, value) => { - const right = parent._map.get(key) || null + const left = parent._map.get(key) || null if (value == null) { - new ItemJSON(nextID(transaction), null, null, right, right === null ? null : right.id, parent, key, [value]).integrate(transaction) + new ItemJSON(nextID(transaction), left, left === null ? null : left.lastId, null, null, parent, key, [value]).integrate(transaction) return } switch (value.constructor) { @@ -457,14 +457,14 @@ export const typeMapSet = (transaction, parent, key, value) => { case Object: case Array: case String: - new ItemJSON(nextID(transaction), null, null, right, right === null ? null : right.id, parent, key, [value]).integrate(transaction) + new ItemJSON(nextID(transaction), left, left === null ? null : left.lastId, null, null, parent, key, [value]).integrate(transaction) break case ArrayBuffer: - new ItemBinary(nextID(transaction), null, null, right, right === null ? null : right.id, parent, key, value).integrate(transaction) + new ItemBinary(nextID(transaction), left, left === null ? null : left.lastId, null, null, parent, key, value).integrate(transaction) break default: if (value instanceof AbstractType) { - new ItemType(nextID(transaction), null, null, right, right === null ? null : right.id, parent, key, value).integrate(transaction) + new ItemType(nextID(transaction), left, left === null ? null : left.lastId, null, null, parent, key, value).integrate(transaction) } else { throw new Error('Unexpected content type') } @@ -492,7 +492,7 @@ export const typeMapGetAll = (parent) => { let res = {} for (const [key, value] of parent._map) { if (!value.deleted) { - res[key] = value.getContent()[0] + res[key] = value.getContent()[value.length - 1] } } return res @@ -517,9 +517,9 @@ export const typeMapHas = (parent, key) => { export const typeMapGetSnapshot = (parent, key, snapshot) => { let v = parent._map.get(key) || null while (v !== null && (!snapshot.sm.has(v.id.client) || v.id.clock >= (snapshot.sm.get(v.id.client) || 0))) { - v = v.right + v = v.left } - return v !== null && isVisible(v, snapshot) ? v.getContent()[0] : undefined + return v !== null && isVisible(v, snapshot) ? v.getContent()[v.length - 1] : undefined } /** diff --git a/src/utils/Transaction.js b/src/utils/Transaction.js index 04543d7a..43a2625b 100644 --- a/src/utils/Transaction.js +++ b/src/utils/Transaction.js @@ -190,6 +190,10 @@ export const transact = (y, f) => { if (left.deleted === right.deleted && left.constructor === right.constructor) { if (left.mergeWith(right)) { structs.splice(pos, 1) + if (right instanceof AbstractItem && right.parentSub !== null && right.parent._map.get(right.parentSub) === right) { + // @ts-ignore we already did a constructor check above + right.parent._map.set(right.parentSub, left) + } } } } @@ -210,6 +214,7 @@ export const transact = (y, f) => { } } // try to merge replacedItems + // TODO: replacedItems should hold ids for (const replacedItem of transaction._replacedItems) { const id = replacedItem.id const client = id.client diff --git a/tests/y-map.tests.js b/tests/y-map.tests.js index 223e1ab4..5cfe0a2f 100644 --- a/tests/y-map.tests.js +++ b/tests/y-map.tests.js @@ -117,7 +117,7 @@ export const testGetAndSetOfMapPropertyWithConflict = tc => { testConnector.flushAllMessages() for (let user of users) { var u = user.getMap('map') - t.compare(u.get('stuff'), 'c0') + t.compare(u.get('stuff'), 'c1') } compare(users) } @@ -128,8 +128,8 @@ export const testGetAndSetOfMapPropertyWithConflict = tc => { export const testGetAndSetAndDeleteOfMapProperty = tc => { const { testConnector, users, map0, map1 } = init(tc, { users: 3 }) map0.set('stuff', 'c0') - map0.delete('stuff') map1.set('stuff', 'c1') + map1.delete('stuff') testConnector.flushAllMessages() for (let user of users) { var u = user.getMap('map') @@ -150,7 +150,7 @@ export const testGetAndSetOfMapPropertyWithThreeConflicts = tc => { testConnector.flushAllMessages() for (let user of users) { var u = user.getMap('map') - t.compare(u.get('stuff'), 'c0') + t.compare(u.get('stuff'), 'c3') } compare(users) } @@ -166,10 +166,10 @@ export const testGetAndSetAndDeleteOfMapPropertyWithThreeConflicts = tc => { map2.set('stuff', 'c3') testConnector.flushAllMessages() map0.set('stuff', 'deleteme') - map0.delete('stuff') map1.set('stuff', 'c1') map2.set('stuff', 'c2') map3.set('stuff', 'c3') + map3.delete('stuff') testConnector.flushAllMessages() for (let user of users) { var u = user.getMap('map') @@ -348,7 +348,7 @@ const mapTransactions = [ * @param {t.TestCase} tc */ export const testRepeatGeneratingYmapTests10 = tc => { - applyRandomTests(tc, mapTransactions, 10) + applyRandomTests(tc, mapTransactions, 4) } /**