diff --git a/services/document-updater/app/js/sharejs/types/text.js b/services/document-updater/app/js/sharejs/types/text.js index bbbe36e0f6..d733837b34 100644 --- a/services/document-updater/app/js/sharejs/types/text.js +++ b/services/document-updater/app/js/sharejs/types/text.js @@ -32,6 +32,8 @@ // NOTE: The global scope here is shared with other sharejs files when built with closure. // Be careful what ends up in your namespace. +const Rope = require('jumprope') + let append, transformComponent const text = {} @@ -68,28 +70,23 @@ const checkValidOp = function (op) { } text.apply = function (snapshot, op) { + const rope = new Rope(snapshot) checkValidOp(op) for (const component of Array.from(op)) { if (component.i != null) { - snapshot = strInject(snapshot, component.p, component.i) + rope.insert(component.p, component.i) } else if (component.d != null) { - const deleted = snapshot.slice( - component.p, - component.p + component.d.length - ) + const deleted = ropeSubstring(rope, component.p, component.d.length) if (component.d !== deleted) { throw new Error( `Delete component '${component.d}' does not match deleted text '${deleted}'` ) } - snapshot = - snapshot.slice(0, component.p) + - snapshot.slice(component.p + component.d.length) + rope.del(component.p, component.d.length) } else if (component.c != null) { - const comment = snapshot.slice( - component.p, - component.p + component.c.length - ) + // The rope has strict bounds checks, so we need to make sure we don't + // extract text beyond the end of the rope + const comment = ropeSubstring(rope, component.p, component.c.length) if (component.c !== comment) { throw new Error( `Comment component '${component.c}' does not match commented text '${comment}'` @@ -99,7 +96,7 @@ text.apply = function (snapshot, op) { throw new Error('Unknown op type') } } - return snapshot + return rope.toString() } // Exported for use by the random op generator. @@ -388,3 +385,11 @@ if (typeof WEB !== 'undefined' && WEB !== null) { append ) } + +function ropeSubstring(rope, start, length) { + // The rope has strict bounds checks, so we need to make sure we don't + // extract text beyond the end of the rope. + start = Math.min(rope.length, start) + length = Math.min(rope.length - start, length) + return rope.substring(start, length) +} diff --git a/services/document-updater/benchmarks/apply_string_rope.js b/services/document-updater/benchmarks/apply_string_rope.js new file mode 100644 index 0000000000..a1fb5b4ed7 --- /dev/null +++ b/services/document-updater/benchmarks/apply_string_rope.js @@ -0,0 +1,43 @@ +const { Suite } = require('benchmark') +const Rope = require('jumprope') + +const suite = new Suite() + +for (const size of [5000, 100000, 2000000]) { + const positions = [] + for (let i = 0; i < 100; i++) { + positions.push(Math.floor(Math.random() * size)) + } + for (const inserts of [0, 1, 2, 5, 10, 20, 100, 1000, 10000]) { + const initialString = 'a'.repeat(size) + suite.add(`| string | ${inserts} | ${size} |`, () => { + let str = initialString + for (let i = 0; i < inserts; i++) { + const pos = positions[i % 100] + str = str.slice(0, pos) + '1234567890' + str.slice(pos) + } + return str.length + }) + + suite.add(`| rope | ${inserts} | ${size} |`, () => { + const rope = new Rope(initialString) + for (let i = 0; i < inserts; i++) { + const pos = positions[i % 100] + rope.insert(pos, '1234567890') + } + return rope.toString().length + }) + } +} + +suite.on('cycle', function (event) { + const meanMs = event.target.stats.mean * 1000 + const meanToString = Math.ceil(meanMs * 100000) / 100000 + console.log(`${event.target.name} ${meanToString}ms | `) +}) + +// markdown table headers +console.log('| method | inserts | string size | mean |') +console.log('|-|-|-|-|') + +suite.run({ async: true }) diff --git a/services/document-updater/package-lock.json b/services/document-updater/package-lock.json index 222a7418e4..bd79b8c55e 100644 --- a/services/document-updater/package-lock.json +++ b/services/document-updater/package-lock.json @@ -729,6 +729,16 @@ "tweetnacl": "^0.14.3" } }, + "benchmark": { + "version": "2.1.4", + "resolved": "https://registry.npmjs.org/benchmark/-/benchmark-2.1.4.tgz", + "integrity": "sha1-CfPeMckWQl1JjMLuVloOvzwqVik=", + "dev": true, + "requires": { + "lodash": "^4.17.4", + "platform": "^1.3.3" + } + }, "bignumber.js": { "version": "9.0.1", "resolved": "https://registry.npmjs.org/bignumber.js/-/bignumber.js-9.0.1.tgz", @@ -2634,6 +2644,10 @@ "verror": "1.10.0" } }, + "jumprope": { + "version": "https://github.com/overleaf/jumprope/archive/b3ce482e83925a6c23278f3230a47a82bd440592.tar.gz", + "integrity": "sha512-VSZr8UrKvY551MPwPR8lramBcCcnMDBh03qliCb1q2RaeQQR1GEsbJ5rAY1PCITtZXCOqeYgsEP0cSj2A0eY8w==" + }, "just-extend": { "version": "4.2.1", "resolved": "https://registry.npmjs.org/just-extend/-/just-extend-4.2.1.tgz", @@ -3406,6 +3420,12 @@ "find-up": "^2.1.0" } }, + "platform": { + "version": "1.3.6", + "resolved": "https://registry.npmjs.org/platform/-/platform-1.3.6.tgz", + "integrity": "sha512-fnWVljUchTro6RiCFvCXBbNhJc2NijN7oIQxbwsyL0buWJPG85v81ehlHI9fXrJsMNgTofEoWIQeClKpgxFLrg==", + "dev": true + }, "pprof": { "version": "3.2.0", "resolved": "https://registry.npmjs.org/pprof/-/pprof-3.2.0.tgz", diff --git a/services/document-updater/package.json b/services/document-updater/package.json index 8bde258a06..9312d5bb4d 100644 --- a/services/document-updater/package.json +++ b/services/document-updater/package.json @@ -29,12 +29,14 @@ "bunyan": "^1.8.15", "diff-match-patch": "https://github.com/overleaf/diff-match-patch/archive/89805f9c671a77a263fc53461acd62aa7498f688.tar.gz", "express": "4.17.1", + "jumprope": "https://github.com/overleaf/jumprope/archive/b3ce482e83925a6c23278f3230a47a82bd440592.tar.gz", "lodash": "^4.17.21", "mongodb": "^3.6.6", "request": "^2.88.2", "requestretry": "^4.1.2" }, "devDependencies": { + "benchmark": "^2.1.4", "chai": "^4.2.0", "chai-as-promised": "^7.1.1", "cluster-key-slot": "^1.0.5", diff --git a/services/document-updater/test/unit/js/ShareJS/TextTransformTests.js b/services/document-updater/test/unit/js/ShareJS/TextTransformTests.js index 118cf339c2..45662bcc26 100644 --- a/services/document-updater/test/unit/js/ShareJS/TextTransformTests.js +++ b/services/document-updater/test/unit/js/ShareJS/TextTransformTests.js @@ -13,6 +13,7 @@ * DS205: Consider reworking code to avoid use of IIFEs * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ +const { expect } = require('chai') const text = require('../../../../app/js/sharejs/types/text') const RangesTracker = require('../../../../app/js/RangesTracker') @@ -268,15 +269,84 @@ describe('ShareJS text type', function () { .should.equal('foo123bar') }) + it('should apply delete operations that are not in order', function () { + const result = text.apply('0123456789', [ + { d: '9', p: 9 }, + { d: '4', p: 4 }, + { d: '7', p: 6 }, + ]) + expect(result).to.equal('0123568') + }) + + it('should apply a mix of inserts and deletes in any order', function () { + const result = text.apply('we can insert and delete in any order!', [ + { d: 'any ', p: 28 }, // we can insert and delete in order! + { i: 'not', p: 6 }, // we cannot insert and delete in order! + { d: 'insert and ', p: 10 }, // we cannot delete in order! + { i: 'the same ', p: 20 }, + ]) + expect(result).to.equal('we cannot delete in the same order!') + }) + + it('should apply a mix of inserts and deletes in order', function () { + const result = text.apply('we can insert and delete in any order!', [ + { i: 'not', p: 6 }, // we cannot insert and delete in any order! + { d: 'insert and ', p: 10 }, // we cannot delete in any order! + { d: 'any ', p: 20 }, // we cannot delete in order! + { i: 'the same ', p: 20 }, + ]) + expect(result).to.equal('we cannot delete in the same order!') + }) + + it('should be able to insert a string, then delete it along with some more text that comes before', function () { + const result = text.apply('I love cake and cookies', [ + { i: ', vegetables', p: 11 }, // I love cake, vegetables and cookies + { d: 'cake, vege', p: 7 }, + ]) + expect(result).to.equal('I love tables and cookies') + }) + + it('should be able to insert a string, then delete it along with some more text that comes after', function () { + const result = text.apply('I love cake and cookies', [ + { i: 'chocolate, ', p: 7 }, // I love chocolate, cake and cookies + { d: ', cake and', p: 16 }, + ]) + expect(result).to.equal('I love chocolate cookies') + }) + it('should throw an error when deleted content does not match', function () { - return (() => text.apply('foo123bar', [{ d: '456', p: 3 }])).should.throw( - Error + ;(() => text.apply('foo123bar', [{ d: '456', p: 3 }])).should.throw( + /^Delete component/ ) }) - return it('should throw an error when comment content does not match', function () { - return (() => text.apply('foo123bar', [{ c: '456', p: 3 }])).should.throw( - Error + it('should throw an error when deleted content extends beyond the text', function () { + ;(() => text.apply('foo123bar', [{ d: '123barbaz', p: 3 }])).should.throw( + /^Delete component/ + ) + }) + + it('should throw an error when deleted content is out of bounds', function () { + ;(() => text.apply('foo123bar', [{ d: '456', p: 20 }])).should.throw( + /^Delete component/ + ) + }) + + it('should throw an error when comment content does not match', function () { + ;(() => text.apply('foo123bar', [{ c: '456', p: 3 }])).should.throw( + /^Comment component/ + ) + }) + + it('should throw an error when commented content extends beyond the text', function () { + ;(() => text.apply('foo123bar', [{ c: '123barbaz', p: 3 }])).should.throw( + /^Comment component/ + ) + }) + + it('should throw an error when commented content is out of bounds', function () { + ;(() => text.apply('foo123bar', [{ c: '456', p: 20 }])).should.throw( + /^Comment component/ ) }) })