[perf] use MGET/MSET/DEL for manipulating multiple keys in one operation

In some cases we can get rid of MULTI/EXEC operations too.
- putDocInMemory: from 10 down to 2 operations
- removeDocFromMemory: from 14+4 down to 4+4 operations
- updateDoc: from 13 down to 8 operations

services/document-updater/app/js/RedisManager.js

@@ -88,19 +88,18 @@ module.exports = RedisManager = {
       rclient.sadd(keys.docsInProject({ project_id }), doc_id, (error) => {
         if (error) return callback(error)
 
-        const multi = rclient.multi()
-        multi.set(keys.docLines({ doc_id }), docLines)
-        multi.set(keys.projectKey({ doc_id }), project_id)
-        multi.set(keys.docVersion({ doc_id }), version)
-        multi.set(keys.docHash({ doc_id }), docHash)
-        if (ranges != null) {
-          multi.set(keys.ranges({ doc_id }), ranges)
-        } else {
-          multi.del(keys.ranges({ doc_id }))
-        }
-        multi.set(keys.pathname({ doc_id }), pathname)
-        multi.set(keys.projectHistoryId({ doc_id }), projectHistoryId)
-        multi.exec(callback)
+        rclient.mset(
+          {
+            [keys.docLines({ doc_id })]: docLines,
+            [keys.projectKey({ doc_id })]: project_id,
+            [keys.docVersion({ doc_id })]: version,
+            [keys.docHash({ doc_id })]: docHash,
+            [keys.ranges({ doc_id })]: ranges,
+            [keys.pathname({ doc_id })]: pathname,
+            [keys.projectHistoryId({ doc_id })]: projectHistoryId
+          },
+          callback
+        )
       })
     })
   },
@@ -119,17 +118,19 @@ module.exports = RedisManager = {
 
     let multi = rclient.multi()
     multi.strlen(keys.docLines({ doc_id }))
-    multi.del(keys.docLines({ doc_id }))
-    multi.del(keys.projectKey({ doc_id }))
-    multi.del(keys.docVersion({ doc_id }))
-    multi.del(keys.docHash({ doc_id }))
-    multi.del(keys.ranges({ doc_id }))
-    multi.del(keys.pathname({ doc_id }))
-    multi.del(keys.projectHistoryId({ doc_id }))
-    multi.del(keys.projectHistoryType({ doc_id }))
-    multi.del(keys.unflushedTime({ doc_id }))
-    multi.del(keys.lastUpdatedAt({ doc_id }))
-    multi.del(keys.lastUpdatedBy({ doc_id }))
+    multi.del(
+      keys.docLines({ doc_id }),
+      keys.projectKey({ doc_id }),
+      keys.docVersion({ doc_id }),
+      keys.docHash({ doc_id }),
+      keys.ranges({ doc_id }),
+      keys.pathname({ doc_id }),
+      keys.projectHistoryId({ doc_id }),
+      keys.projectHistoryType({ doc_id }),
+      keys.unflushedTime({ doc_id }),
+      keys.lastUpdatedAt({ doc_id }),
+      keys.lastUpdatedBy({ doc_id })
+    )
     return multi.exec(function (error, response) {
       if (error != null) {
         return callback(error)
@@ -483,19 +484,19 @@ module.exports = RedisManager = {
           return callback(error)
         }
         const multi = rclient.multi()
-        multi.set(keys.docLines({ doc_id }), newDocLines) // index 0
-        multi.set(keys.docVersion({ doc_id }), newVersion) // index 1
-        multi.set(keys.docHash({ doc_id }), newHash) // index 2
+        multi.mset({
+          [keys.docLines({ doc_id })]: newDocLines,
+          [keys.docVersion({ doc_id })]: newVersion,
+          [keys.docHash({ doc_id })]: newHash,
+          [keys.ranges({ doc_id })]: ranges,
+          [keys.lastUpdatedAt({ doc_id })]: Date.now(),
+          [keys.lastUpdatedBy({ doc_id })]: updateMeta && updateMeta.user_id
+        })
         multi.ltrim(
           keys.docOps({ doc_id }),
           -RedisManager.DOC_OPS_MAX_LENGTH,
           -1
         ) // index 3
-        if (ranges != null) {
-          multi.set(keys.ranges({ doc_id }), ranges) // index 4
-        } else {
-          multi.del(keys.ranges({ doc_id })) // also index 4
-        }
         // push the ops last so we can get the lengths at fixed index position 7
         if (jsonOps.length > 0) {
           multi.rpush(keys.docOps({ doc_id }), ...Array.from(jsonOps)) // index 5
@@ -519,12 +520,6 @@ module.exports = RedisManager = {
           // hasn't been modified before (the content in mongo has been
           // valid up to this point). Otherwise leave it alone ("NX" flag).
           multi.set(keys.unflushedTime({ doc_id }), Date.now(), 'NX')
-          multi.set(keys.lastUpdatedAt({ doc_id }), Date.now()) // index 8
-          if (updateMeta != null ? updateMeta.user_id : undefined) {
-            multi.set(keys.lastUpdatedBy({ doc_id }), updateMeta.user_id) // index 9
-          } else {
-            multi.del(keys.lastUpdatedBy({ doc_id })) // index 9
-          }
         }
         return multi.exec(function (error, result) {
           let docUpdateCount
@@ -536,7 +531,7 @@ module.exports = RedisManager = {
             docUpdateCount = undefined // only using project history, don't bother with track-changes
           } else {
             // project is using old track-changes history service
-            docUpdateCount = result[7] // length of uncompressedHistoryOps queue (index 7)
+            docUpdateCount = result[4]
           }
 
           if (

services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js

@@ -153,7 +153,6 @@ describe('RedisManager', function () {
           this.projectHistoryId.toString(),
           this.unflushed_time
         ])
-      return (this.rclient.sadd = sinon.stub().yields(null, 0))
     })
 
     describe('successfully', function () {
@@ -469,6 +468,7 @@ describe('RedisManager', function () {
       this.project_update_list_length = sinon.stub()
 
       this.RedisManager.getDocVersion = sinon.stub()
+      this.multi.mset = sinon.stub()
       this.multi.set = sinon.stub()
       this.multi.rpush = sinon.stub()
       this.multi.expire = sinon.stub()
@@ -477,9 +477,6 @@ describe('RedisManager', function () {
       this.multi.exec = sinon
         .stub()
         .callsArgWith(0, null, [
-          this.hash,
-          null,
-          null,
           null,
           null,
           null,
@@ -524,27 +521,16 @@ describe('RedisManager', function () {
             .should.equal(true)
         })
 
-        it('should set the doclines', function () {
-          return this.multi.set
-            .calledWith(`doclines:${this.doc_id}`, JSON.stringify(this.lines))
-            .should.equal(true)
-        })
-
-        it('should set the version', function () {
-          return this.multi.set
-            .calledWith(`DocVersion:${this.doc_id}`, this.version)
-            .should.equal(true)
-        })
-
-        it('should set the hash', function () {
-          return this.multi.set
-            .calledWith(`DocHash:${this.doc_id}`, this.hash)
-            .should.equal(true)
-        })
-
-        it('should set the ranges', function () {
-          return this.multi.set
-            .calledWith(`Ranges:${this.doc_id}`, JSON.stringify(this.ranges))
+        it('should set most details in a single MSET call', function () {
+          this.multi.mset
+            .calledWith({
+              [`doclines:${this.doc_id}`]: JSON.stringify(this.lines),
+              [`DocVersion:${this.doc_id}`]: this.version,
+              [`DocHash:${this.doc_id}`]: this.hash,
+              [`Ranges:${this.doc_id}`]: JSON.stringify(this.ranges),
+              [`lastUpdatedAt:${this.doc_id}`]: Date.now(),
+              [`lastUpdatedBy:${this.doc_id}`]: 'last-author-fake-id'
+            })
             .should.equal(true)
         })
 
@@ -554,18 +540,6 @@ describe('RedisManager', function () {
             .should.equal(true)
         })
 
-        it('should set the last updated time', function () {
-          return this.multi.set
-            .calledWith(`lastUpdatedAt:${this.doc_id}`, Date.now())
-            .should.equal(true)
-        })
-
-        it('should set the last updater', function () {
-          return this.multi.set
-            .calledWith(`lastUpdatedBy:${this.doc_id}`, 'last-author-fake-id')
-            .should.equal(true)
-        })
-
         it('should push the doc op into the doc ops list', function () {
           return this.multi.rpush
             .calledWith(
@@ -747,8 +721,15 @@ describe('RedisManager', function () {
       })
 
       return it('should still set the doclines', function () {
-        return this.multi.set
-          .calledWith(`doclines:${this.doc_id}`, JSON.stringify(this.lines))
+        this.multi.mset
+          .calledWith({
+            [`doclines:${this.doc_id}`]: JSON.stringify(this.lines),
+            [`DocVersion:${this.doc_id}`]: this.version,
+            [`DocHash:${this.doc_id}`]: this.hash,
+            [`Ranges:${this.doc_id}`]: JSON.stringify(this.ranges),
+            [`lastUpdatedAt:${this.doc_id}`]: Date.now(),
+            [`lastUpdatedBy:${this.doc_id}`]: 'last-author-fake-id'
+          })
           .should.equal(true)
       })
     })
@@ -770,15 +751,16 @@ describe('RedisManager', function () {
         )
       })
 
-      it('should not set the ranges', function () {
-        return this.multi.set
-          .calledWith(`Ranges:${this.doc_id}`, JSON.stringify(this.ranges))
-          .should.equal(false)
-      })
-
-      return it('should delete the ranges key', function () {
-        return this.multi.del
-          .calledWith(`Ranges:${this.doc_id}`)
+      it('should set empty ranges', function () {
+        this.multi.mset
+          .calledWith({
+            [`doclines:${this.doc_id}`]: JSON.stringify(this.lines),
+            [`DocVersion:${this.doc_id}`]: this.version,
+            [`DocHash:${this.doc_id}`]: this.hash,
+            [`Ranges:${this.doc_id}`]: null,
+            [`lastUpdatedAt:${this.doc_id}`]: Date.now(),
+            [`lastUpdatedBy:${this.doc_id}`]: 'last-author-fake-id'
+          })
           .should.equal(true)
       })
     })
@@ -866,15 +848,16 @@ describe('RedisManager', function () {
         )
       })
 
-      it('should set the last updater to null', function () {
-        return this.multi.del
-          .calledWith(`lastUpdatedBy:${this.doc_id}`)
-          .should.equal(true)
-      })
-
-      return it('should still set the last updated time', function () {
-        return this.multi.set
-          .calledWith(`lastUpdatedAt:${this.doc_id}`, Date.now())
+      it('should unset last updater', function () {
+        this.multi.mset
+          .calledWith({
+            [`doclines:${this.doc_id}`]: JSON.stringify(this.lines),
+            [`DocVersion:${this.doc_id}`]: this.version,
+            [`DocHash:${this.doc_id}`]: this.hash,
+            [`Ranges:${this.doc_id}`]: JSON.stringify(this.ranges),
+            [`lastUpdatedAt:${this.doc_id}`]: Date.now(),
+            [`lastUpdatedBy:${this.doc_id}`]: undefined
+          })
           .should.equal(true)
       })
     })
@@ -882,16 +865,14 @@ describe('RedisManager', function () {
 
   describe('putDocInMemory', function () {
     beforeEach(function () {
-      this.multi.set = sinon.stub()
+      this.rclient.mset = sinon.stub().yields(null)
       this.rclient.sadd = sinon.stub().yields()
-      this.multi.del = sinon.stub()
       this.lines = ['one', 'two', 'three', 'これは']
       this.version = 42
       this.hash = crypto
         .createHash('sha1')
         .update(JSON.stringify(this.lines), 'utf8')
         .digest('hex')
-      this.multi.exec = sinon.stub().callsArgWith(0, null, [this.hash])
       this.ranges = { comments: 'mock', entries: 'mock' }
       return (this.pathname = '/a/b/c.tex')
     })
@@ -910,45 +891,17 @@ describe('RedisManager', function () {
         )
       })
 
-      it('should set the lines', function () {
-        return this.multi.set
-          .calledWith(`doclines:${this.doc_id}`, JSON.stringify(this.lines))
-          .should.equal(true)
-      })
-
-      it('should set the version', function () {
-        return this.multi.set
-          .calledWith(`DocVersion:${this.doc_id}`, this.version)
-          .should.equal(true)
-      })
-
-      it('should set the hash', function () {
-        return this.multi.set
-          .calledWith(`DocHash:${this.doc_id}`, this.hash)
-          .should.equal(true)
-      })
-
-      it('should set the ranges', function () {
-        return this.multi.set
-          .calledWith(`Ranges:${this.doc_id}`, JSON.stringify(this.ranges))
-          .should.equal(true)
-      })
-
-      it('should set the project_id for the doc', function () {
-        return this.multi.set
-          .calledWith(`ProjectId:${this.doc_id}`, this.project_id)
-          .should.equal(true)
-      })
-
-      it('should set the pathname for the doc', function () {
-        return this.multi.set
-          .calledWith(`Pathname:${this.doc_id}`, this.pathname)
-          .should.equal(true)
-      })
-
-      it('should set the projectHistoryId for the doc', function () {
-        return this.multi.set
-          .calledWith(`ProjectHistoryId:${this.doc_id}`, this.projectHistoryId)
+      it('should set all the details in a single MSET call', function () {
+        this.rclient.mset
+          .calledWith({
+            [`doclines:${this.doc_id}`]: JSON.stringify(this.lines),
+            [`ProjectId:${this.doc_id}`]: this.project_id,
+            [`DocVersion:${this.doc_id}`]: this.version,
+            [`DocHash:${this.doc_id}`]: this.hash,
+            [`Ranges:${this.doc_id}`]: JSON.stringify(this.ranges),
+            [`Pathname:${this.doc_id}`]: this.pathname,
+            [`ProjectHistoryId:${this.doc_id}`]: this.projectHistoryId
+          })
           .should.equal(true)
       })
 
@@ -977,17 +930,19 @@ describe('RedisManager', function () {
         )
       })
 
-      it('should delete the ranges key', function () {
-        return this.multi.del
-          .calledWith(`Ranges:${this.doc_id}`)
+      it('should unset ranges', function () {
+        this.rclient.mset
+          .calledWith({
+            [`doclines:${this.doc_id}`]: JSON.stringify(this.lines),
+            [`ProjectId:${this.doc_id}`]: this.project_id,
+            [`DocVersion:${this.doc_id}`]: this.version,
+            [`DocHash:${this.doc_id}`]: this.hash,
+            [`Ranges:${this.doc_id}`]: null,
+            [`Pathname:${this.doc_id}`]: this.pathname,
+            [`ProjectHistoryId:${this.doc_id}`]: this.projectHistoryId
+          })
           .should.equal(true)
       })
-
-      return it('should not set the ranges', function () {
-        return this.multi.set
-          .calledWith(`Ranges:${this.doc_id}`, JSON.stringify(this.ranges))
-          .should.equal(false)
-      })
     })
 
     describe('with null bytes in the serialized doc lines', function () {
@@ -1070,33 +1025,21 @@ describe('RedisManager', function () {
         .should.equal(true)
     })
 
-    it('should delete the lines', function () {
+    it('should delete the details in a singe call', function () {
       return this.multi.del
-        .calledWith(`doclines:${this.doc_id}`)
-        .should.equal(true)
-    })
-
-    it('should delete the version', function () {
-      return this.multi.del
-        .calledWith(`DocVersion:${this.doc_id}`)
-        .should.equal(true)
-    })
-
-    it('should delete the hash', function () {
-      return this.multi.del
-        .calledWith(`DocHash:${this.doc_id}`)
-        .should.equal(true)
-    })
-
-    it('should delete the unflushed time', function () {
-      return this.multi.del
-        .calledWith(`UnflushedTime:${this.doc_id}`)
-        .should.equal(true)
-    })
-
-    it('should delete the project_id for the doc', function () {
-      return this.multi.del
-        .calledWith(`ProjectId:${this.doc_id}`)
+        .calledWith(
+          `doclines:${this.doc_id}`,
+          `ProjectId:${this.doc_id}`,
+          `DocVersion:${this.doc_id}`,
+          `DocHash:${this.doc_id}`,
+          `Ranges:${this.doc_id}`,
+          `Pathname:${this.doc_id}`,
+          `ProjectHistoryId:${this.doc_id}`,
+          `ProjectHistoryType:${this.doc_id}`,
+          `UnflushedTime:${this.doc_id}`,
+          `lastUpdatedAt:${this.doc_id}`,
+          `lastUpdatedBy:${this.doc_id}`
+        )
         .should.equal(true)
     })
 
@@ -1105,30 +1048,6 @@ describe('RedisManager', function () {
         .calledWith(`DocsIn:${this.project_id}`, this.doc_id)
         .should.equal(true)
     })
-
-    it('should delete the pathname for the doc', function () {
-      return this.multi.del
-        .calledWith(`Pathname:${this.doc_id}`)
-        .should.equal(true)
-    })
-
-    it('should delete the pathname for the doc', function () {
-      return this.multi.del
-        .calledWith(`ProjectHistoryId:${this.doc_id}`)
-        .should.equal(true)
-    })
-
-    it('should delete lastUpdatedAt', function () {
-      return this.multi.del
-        .calledWith(`lastUpdatedAt:${this.doc_id}`)
-        .should.equal(true)
-    })
-
-    return it('should delete lastUpdatedBy', function () {
-      return this.multi.del
-        .calledWith(`lastUpdatedBy:${this.doc_id}`)
-        .should.equal(true)
-    })
   })
 
   describe('clearProjectState', function () {