From 1ce8fd597950be28134ab5d39b70942b33fe1943 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 10 Oct 2022 11:39:15 +1100 Subject: [PATCH] fix: make circular buffer not recreate an array on each overflow --- ts/data/data.ts | 2 +- .../apis/open_group_api/sogsv3/sogsApiV3.ts | 2 +- .../sogsv3/sogsRollingDeletions.ts | 2 +- ts/session/utils/RingBuffer.ts | 52 ++++- ts/test/session/unit/utils/RingBuffer_test.ts | 184 ++++++++++++++---- 5 files changed, 198 insertions(+), 44 deletions(-) diff --git a/ts/data/data.ts b/ts/data/data.ts index f7adbe688..a9b47e674 100644 --- a/ts/data/data.ts +++ b/ts/data/data.ts @@ -392,7 +392,7 @@ async function removeMessage(id: string): Promise { /** * Note: this method will not clean up external files, just delete from SQL. - * File are cleaned up on app start if they are not linked to any messages + * Files are cleaned up on app start if they are not linked to any messages * */ async function removeMessagesByIds(ids: Array): Promise { diff --git a/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts b/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts index b07b36f58..731fb6f34 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts @@ -326,7 +326,7 @@ const handleMessagesResponseV4 = async ( /* * When a message is deleted from the server, we get the deleted event as a data: null on the message itself * and an update on its reactions. - * But, because we just deleted that message, we can skip trying to udpate its reactions: it's not in the DB anymore. + * But, because we just deleted that message, we can skip trying to update its reactions: it's not in the DB anymore. */ if (sogsRollingDeletions.hasMessageDeletedId(conversationId, messageWithReaction.id)) { continue; diff --git a/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts b/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts index 80bbb8de2..9582c6e1b 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts @@ -13,7 +13,7 @@ const addMessageDeletedId = (conversationId: string, messageDeletedId: number) = if (!ringBuffer) { return; } - ringBuffer.add(messageDeletedId); + ringBuffer.insert(messageDeletedId); }; const hasMessageDeletedId = (conversationId: string, messageDeletedId: number) => { diff --git a/ts/session/utils/RingBuffer.ts b/ts/session/utils/RingBuffer.ts index 6d9173d6f..259a3962a 100644 --- a/ts/session/utils/RingBuffer.ts +++ b/ts/session/utils/RingBuffer.ts @@ -4,6 +4,8 @@ * */ export class RingBuffer { + private newest = -1; + private oldest = 0; private buffer: Array = []; private readonly capacity: number; @@ -16,25 +18,59 @@ export class RingBuffer { } public getLength(): number { - return this.buffer.length; + if (this.isEmpty()) { + return 0; + } + + // When only one item was added, newest = 0 and oldest = 0. + // When more than one item was added, but less than capacity, newest = nbItemsAdded & oldest = 0. + // As soon as we overflow, oldest is incremented to oldest+1 and newest rolls back to 0, + // so this test fails here and we have to extract the length based on the two parts instead. + if (this.newest >= this.oldest) { + return this.newest + 1; + } + const firstPart = this.capacity - this.oldest; + const secondPart = this.newest + 1; + return firstPart + secondPart; } - public add(item: T) { - this.buffer.push(item); - this.crop(); + public insert(item: T) { + // see comments in `getLength()` + this.newest = (this.newest + 1) % this.capacity; + if (this.buffer.length >= this.capacity) { + this.oldest = (this.oldest + 1) % this.capacity; + } + this.buffer[this.newest] = item; } public has(item: T) { - return this.buffer.includes(item); + // no items at all + if (this.isEmpty()) { + return false; + } + return this.toArray().includes(item); + } + + public isEmpty() { + return this.newest === -1; } public clear() { this.buffer = []; + this.newest = -1; + this.oldest = 0; } - private crop() { - while (this.buffer.length > this.capacity) { - this.buffer.shift(); + public toArray(): Array { + if (this.isEmpty()) { + return []; + } + + if (this.newest >= this.oldest) { + return this.buffer.slice(0, this.newest + 1); } + const firstPart = this.buffer.slice(this.oldest, this.capacity); + const secondPart = this.buffer.slice(0, this.newest + 1); + return [...firstPart, ...secondPart]; } } diff --git a/ts/test/session/unit/utils/RingBuffer_test.ts b/ts/test/session/unit/utils/RingBuffer_test.ts index eee05f34c..add8aa712 100644 --- a/ts/test/session/unit/utils/RingBuffer_test.ts +++ b/ts/test/session/unit/utils/RingBuffer_test.ts @@ -10,43 +10,80 @@ describe('RingBuffer Utils', () => { const ring = new RingBuffer(5000); expect(ring.getCapacity()).to.equal(5000); expect(ring.getLength()).to.equal(0); - expect(ring.has(0)).to.equal(false, '4 should not be there'); + expect(ring.has(0)).to.equal(false, '0 should not be there'); }); describe('length & capacity are right', () => { + it('length is right 0', () => { + const ring = new RingBuffer(4); + expect(ring.getLength()).to.equal(0); + }); + it('length is right 1', () => { const ring = new RingBuffer(4); - ring.add(0); + ring.insert(0); expect(ring.getLength()).to.equal(1); }); it('length is right 4', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(3); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); expect(ring.getLength()).to.equal(4); }); it('capacity does not get exceeded', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(3); - ring.add(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); expect(ring.getLength()).to.equal(4); }); }); + describe('isEmpty is correct', () => { + it('no items', () => { + const ring = new RingBuffer(4); + expect(ring.isEmpty()).to.equal(true, 'no items isEmpty should be true'); + }); + + it('length is right 1', () => { + const ring = new RingBuffer(4); + ring.insert(0); + expect(ring.isEmpty()).to.equal(false, '1 item isEmpty should be false'); + }); + + it('length is right 4', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + expect(ring.isEmpty()).to.equal(false, '4 items isEmpty should be false'); + }); + + it('more than capacity', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); + expect(ring.isEmpty()).to.equal(false, '5 item isEmpty should be false'); + }); + }); + it('items are removed in order 1', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(3); - ring.add(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); expect(ring.has(0)).to.equal(false, '0 should not be there anymore'); expect(ring.has(1)).to.equal(true, '1 should still be there'); expect(ring.has(2)).to.equal(true, '2 should still be there'); @@ -56,11 +93,11 @@ describe('RingBuffer Utils', () => { it('two times the same items can exist', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(1); - ring.add(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(1); + ring.insert(4); expect(ring.has(0)).to.equal(false, '0 should not be there anymore'); expect(ring.has(1)).to.equal(true, '1 should still be there'); expect(ring.has(2)).to.equal(true, '2 should still be there'); @@ -70,14 +107,14 @@ describe('RingBuffer Utils', () => { it('items are removed in order completely', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(3); - ring.add(10); - ring.add(20); - ring.add(30); - ring.add(40); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(10); + ring.insert(20); + ring.insert(30); + ring.insert(40); expect(ring.has(0)).to.equal(false, '0 should not be there anymore'); expect(ring.has(1)).to.equal(false, '1 should not be there'); expect(ring.has(2)).to.equal(false, '2 should not be there'); @@ -92,10 +129,10 @@ describe('RingBuffer Utils', () => { it('clear empties the list but keeps the capacity', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(1); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(1); expect(ring.getLength()).to.equal(4); expect(ring.getCapacity()).to.equal(4); ring.clear(); @@ -103,4 +140,85 @@ describe('RingBuffer Utils', () => { expect(ring.getLength()).to.equal(0); }); + + describe('toArray', () => { + it('empty buffer', () => { + const ring = new RingBuffer(4); + expect(ring.toArray()).to.deep.eq([]); + }); + + it('with 1', () => { + const ring = new RingBuffer(4); + ring.insert(0); + + expect(ring.toArray()).to.deep.eq([0]); + }); + + it('with 4', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + + expect(ring.toArray()).to.deep.eq([0, 1, 2, 3]); + }); + + it('with 5', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); + + expect(ring.toArray()).to.deep.eq([1, 2, 3, 4]); + }); + + it('more than 2 full laps erasing data', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); // first lap first item + ring.insert(5); + ring.insert(6); // first item in toArray should be this one + ring.insert(7); + ring.insert(8); // second lap first item + ring.insert(9); + + expect(ring.toArray()).to.deep.eq([6, 7, 8, 9]); + }); + }); + + describe('clear', () => { + it('empty buffer', () => { + const ring = new RingBuffer(4); + ring.clear(); + expect(ring.getCapacity()).to.deep.eq(4); + expect(ring.getLength()).to.deep.eq(0); + }); + + it('with 1', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.clear(); + expect(ring.getCapacity()).to.deep.eq(4); + expect(ring.getLength()).to.deep.eq(0); + }); + + it('with 5', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); + + ring.clear(); + expect(ring.getCapacity()).to.deep.eq(4); + expect(ring.getLength()).to.deep.eq(0); + }); + }); });