Merge pull request #6018 from overleaf/msm-rope-apply

Speed up OT Apply in docupdater using Ropes

GitOrigin-RevId: 9789c3910d40a4161f1f0d9c16cef8d8ca74788c

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)
+}

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 })

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",

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",

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/
+      )
+    })
+
+    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/
       )
     })
 
-    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 commented content is out of bounds', function () {
+      ;(() => text.apply('foo123bar', [{ c: '456', p: 20 }])).should.throw(
+        /^Comment component/
       )
     })
   })