Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(perf): Revert commit to bring back up to 20% perf #574

Merged
merged 2 commits into from
May 26, 2024
+33 −10
Merged

chore(perf): Revert commit to bring back up to 20% perf #574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95a8988
Revert commit to bring back up to 20% perf
JaoodxD May 22, 2024
9422ee8
Add description for tests
JaoodxD May 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 2 additions & 7 deletions src/fixed-queue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,36 +63,32 @@ class FixedCircularBuffer {
top: number
list: Array<Task | undefined>
next: FixedCircularBuffer | null
_size: number

constructor () {
this.bottom = 0;
this.top = 0;
this.list = new Array(kSize);
this.next = null;
this._size = 0;
}

isEmpty () {
return this.top === this.bottom && this._size === 0;
return this.top === this.bottom;
}

isFull () {
return this.top === this.bottom && this._size === kSize;
return ((this.top + 1) & kMask) === this.bottom;
}

push (data:Task) {
this.list[this.top] = data;
this.top = (this.top + 1) & kMask;
this._size++;
}

shift () {
const nextItem = this.list[this.bottom];
if (nextItem === undefined) { return null; }
this.list[this.bottom] = undefined;
this.bottom = (this.bottom + 1) & kMask;
this._size--;
return nextItem;
}

Expand All @@ -114,7 +110,6 @@ class FixedCircularBuffer {
curr = next;
}
this.top = (this.top - 1) & kMask;
this._size--;
}
}

Expand Down
34 changes: 31 additions & 3 deletions test/fixed-queue.ts
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do benchmarks with payloads twice or three times its size?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to push something bigger than integers or just more items?
If the second option then it's already done.
Results could be seen here #555 (comment) for twice the size, 64 times the size and 1024 times the size of single circular buffer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good call, sorry I missed that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem.
Btw in the last few days I tried to play with circular buffer size, tried to preallocate and reuse ring buffers with a pool and so on but nothing really brings benefits, only performance degradations.
It feels like FixedQueue was created by a dark magician who put an "unbeatable" spell on it. 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is pretty well optimized. Liked the tradeoff of memory for more performance 👌

Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,19 @@ test('remove not queued task should not lead to errors', async ({ equal }) => {
});

test('removing elements from intermediate CircularBuffer should not lead to issues', async ({ equal, same }) => {
/*
The test intends to check following scenario:
1) We fill the queue with 3 full circular buffers amount of items.
2) Empty the middle circular buffer with remove().
3) This should lead to the removal of the middle buffer from the queue:
- Before emptying: tail buffer -> middle buffer -> head buffer.
- After emptying: tail buffer -> head buffer.
*/

const queue = new FixedQueue();

const batchSize = 2048;
// size of single circular buffer
const batchSize = 2047;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it fails with 2048?
In the worst case, it will pre-allocate another buffer, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general it doesn't fail but it should match the batch size due to test's intensions.
The test may require additional clarification with comments.
The main goal was to check following scenario:

  1. We fill the queue with 3 full circular buffers amount of items.
  2. Empty the middle circular buffer.
  3. This should lead to the removal of the middle buffer from the queue:
  • Before emptying: tail buffer -> middle buffer -> head buffer.
  • After emptying: tail buffer -> head buffer.

My initial remove implementation didn't remove empty buffer so I wrote a test to fix it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, now is clear; maybe add that to the description of the test can help understand why are we doing that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I added the description I must admit that the test is actually insufficient/fragile due to lack of internal checks.
As you rightly noticed it doesn't and it wouldn't break even if the size of internal circular buffer will ever changes.
We potentialy could try to really check the amount of buffers with something like following, but this approach will definetly be broken when we ship "real" private fields in further major release.

const queue = new FixedQueue()

let buffer = queue.tail
let count = 0

while (buffer !== null) {
  count++
  buffer = buffer.next
}

console.log(`The queue has ${count} buffers`)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I'd not recommend to alter the internals just for testing; I've always advocated for testing the outcome rather than expectations.
If we want to check how many internal circular buffers has been made, we can always check the overall outcome, we know that if two circular buffers are made, at least we should have 2048 + 1, this at the FixedTaskQueue level.

At the FixedQueue we can go deep but always using the interfaces as a bridge and not going beyond

Copy link
Contributor Author

@JaoodxD JaoodxD May 24, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree that it's not the right way. I just worry about further consequences due to the lack of clarity.
Is there anything else blocking the PR from being landed?


const firstBatch = Array.from({ length: batchSize }, () => new QueueTask());
const secondBatch = Array.from({ length: batchSize }, () => new QueueTask());
Expand Down Expand Up @@ -94,9 +104,18 @@ test('removing elements from intermediate CircularBuffer should not lead to issu
});

test('removing elements from first CircularBuffer should not lead to issues', async ({ equal, same }) => {
/*
The test intends to check following scenario:
1) We fill the queue with 3 full circular buffers amount of items.
2) Empty the first circular buffer with remove().
3) This should lead to the removal of the tail buffer from the queue:
- Before emptying: tail buffer -> middle buffer -> head buffer.
- After emptying: tail buffer (previously middle) -> head buffer.
*/
const queue = new FixedQueue();

const batchSize = 2048;
// size of single circular buffer
const batchSize = 2047;

const firstBatch = Array.from({ length: batchSize }, () => new QueueTask());
const secondBatch = Array.from({ length: batchSize }, () => new QueueTask());
Expand Down Expand Up @@ -125,9 +144,18 @@ test('removing elements from first CircularBuffer should not lead to issues', as
});

test('removing elements from last CircularBuffer should not lead to issues', async ({ equal, same }) => {
/*
The test intends to check following scenario:
1) We fill the queue with 3 full circular buffers amount of items.
2) Empty the last circular buffer with remove().
3) This should lead to the removal of the head buffer from the queue:
- Before emptying: tail buffer -> middle buffer -> head buffer.
- After emptying: tail buffer -> head buffer (previously middle).
*/
const queue = new FixedQueue();

const batchSize = 2048;
// size of single circular buffer
const batchSize = 2047;

const firstBatch = Array.from({ length: batchSize }, () => new QueueTask());
const secondBatch = Array.from({ length: batchSize }, () => new QueueTask());
Expand Down