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.

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

feat!: improve MemoryStore #378

Merged
merged 10 commits into from Sep 3, 2023
Merged

feat!: improve MemoryStore #378

merged 10 commits into from Sep 3, 2023

Conversation

nfriedly
Copy link
Member

@nfriedly nfriedly commented Aug 23, 2023

I've been thinking about how to make the MemoryStore have per-user reset times and still only a single global timer (like precise-memory-rate-limit), but not have to iterate through all known clients to remove expired ones (unlike precise-memory-rate-limit).

I came up with the idea for a previous and current set of clients, where we could pull active ones forward from previous to current, and then at the end of the time window, just blow away the previous, move the current to previous, and create a new current set.

I then refined it a bit to perform less allocations (and by extension, need less garbage collection) by merely swapping previous and current, and then moving the leftover clients from the old previous to a client pool. With an appropriately sized pool, it should be able to completely avoid allocations for most requests.

I had to tweak the tests to use a positive number for windowMs, but after that they all passed.

I've also been thinking about a 'ClusterMemoryStore' where there would be a MemoryStore instance on the primary process, and then each worker would communicate with it via IPC to increment and decrement keys. Then you would only need an external data store for multi-server deployments. Probably should go in it's own module, though.

I've been thinking about how to make the MemoryStore have precise, per-user reset times and still only a single global timer (like `precise-memory-rate-limit`), but *not* have to iterate through all known clients to remove expired ones (unlike `precise-memory-rate-limit`).

I came up with the idea for a previous and current set of clients, where we could pull active ones forward from previous to current, and then at the end of the timer, just blow away the previous, move the current to previous, and create a new current set.

I then refined it a bit to perform less allocations (and by extension, need less garbage collection) by merely swapping previous and current, and then moving the clients from the old previous to a client pool. With an appropriately sized pool, it should be able to completely avoid allocations for most requests.
@nfriedly
Copy link
Member Author

(Sorry I've been kind of all over the place recently. I'll try and get a few of these things wrapped up sometime soon...)

Comment on lines +84 to +88
// Indicates that init was called more than once.
// Could happen if a store was shared between multiple instances.
if (this.interval) {
clearTimeout(this.interval)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes #326

@gamemaker1
Copy link
Member

Wow! This looks really promising, I'll try reviewing it in detail soon.

source/memory-store.ts Outdated Show resolved Hide resolved
source/memory-store.ts Outdated Show resolved Hide resolved
source/memory-store.ts Outdated Show resolved Hide resolved
source/memory-store.ts Outdated Show resolved Hide resolved
@gamemaker1 gamemaker1 changed the title feat: Improved Memory Store feat: improve MemoryStore Aug 26, 2023
source/memory-store.ts Outdated Show resolved Hide resolved
source/memory-store.ts Outdated Show resolved Hide resolved
@nfriedly
Copy link
Member Author

Ugh, I need to redo the performance testing, that bug invalidated everything I did.

@gamemaker1 gamemaker1 changed the title feat: improve MemoryStore feat!: improve MemoryStore Aug 31, 2023
@gamemaker1 gamemaker1 changed the base branch from main to next September 1, 2023 02:54
after fixing the bug where clients weren't cleared from previous, performance testing showed it to be slightly slower than not using a pool at all.
(Both are slightly slower than the old MemoryStore, but they're also more correct, or at least, closer to what most people expect.)
@nfriedly
Copy link
Member Author

nfriedly commented Sep 1, 2023

Ok, now that I fixed the bug with not deleting clients from previous when moving to current, I re-tested and found that the pool actually makes things slightly slower. So I removed it.

I also updated the Use Cases section of the readme although, honestly, we could probably delete it at this point. I wrote that because of how often people were confused by the behavior of the old MemoryStore, and I think the new one will just do what they expect.

I was trying to be fancy, but testing shows that the performance is roughly equal, and this is easier to explain
@nfriedly nfriedly marked this pull request as ready for review September 1, 2023 20:15
@nfriedly
Copy link
Member Author

nfriedly commented Sep 1, 2023

Ok, I believe this is now ready to go.

It's simplified a lot, because my earlier implementation was too clever by half.

Copy link
Member

@gamemaker1 gamemaker1 left a comment

Choose a reason for hiding this comment

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

LGTM!

Ok, now that I fixed the bug with not deleting clients from previous when moving to current, I re-tested and found that the pool actually makes things slightly slower. So I removed it.

Looks like we didn't need to use necromancy after all XD

@nfriedly nfriedly merged commit 1afd610 into next Sep 3, 2023
29 checks passed
@nfriedly nfriedly deleted the memory-store branch September 3, 2023 03:26
nfriedly added a commit that referenced this pull request Sep 11, 2023
This new MemoryStore has precise, per-key reset times, which should more closely match user expectations

---------

Co-authored-by: Vedant K <gamemaker0042@gmail.com>
nfriedly added a commit that referenced this pull request Sep 12, 2023
This new MemoryStore has precise, per-key reset times, which should more closely match user expectations

---------

Co-authored-by: Vedant K <gamemaker0042@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants