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

Messing around with a lockfree Map #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

intendednull
Copy link

@intendednull intendednull commented Jul 12, 2022

Haven't tested this at all, so it could be a lot worse idk. I think a custom hasher would boost performance a lot, but I'm too lazy.

Also there was a bug with the original code. When emptying the queue, it stops at the first item which is not outdated, without checking the rest of the queue. Meaning any outdated items behind it will not be seen, nor removed. These changes make sure to check the entire queue, which is technically slower.

Another way to boost performance would be to individually schedule the removal of each item using time-out workers. This way we don't have to iterate over the items at all, just remove expired ones directly with their id. However this would stray pretty far from the comparison model.

@intendednull intendednull changed the title Messing around with a lockfree Set Messing around with a lockfree Map Jul 12, 2022
@ThePrimeagen
Copy link
Owner

No items in the queue can be earlier than the one at the head.

All items put into the queue are put in with the exact same amount of time in the queue. All expiration dates should be equal or further in time. Check both implementations.

@ThePrimeagen
Copy link
Owner

I cannot imagine this is somehow faster. You have to iterate on every request every single item in the map.

I think we have to go back to the drawing board. I have a great idea with two lock-free queues

@intendednull
Copy link
Author

No items in the queue can be earlier than the one at the head.

Imagine three requests are made, one with 3 seconds, the next 30 seconds, then another with 3 seconds again. In this scenario we would see the first 3 second is expired, but the 30 second is not, leaving the other expired 3 second in the queue.

All items put into the queue are put in with the exact same amount of time in the queue. All expiration dates should be equal or further in time. Check both implementations.

The server code does not enforce this, and the assumption was not clear.

I cannot imagine this is somehow faster. You have to iterate on every request every single item in the map.

Yeah probably not faster then. Let's see if I can spit out my worker idea real fast.

@intendednull
Copy link
Author

intendednull commented Jul 12, 2022

@ThePrimeagen okay modified so we're doing exactly 0 iterations. Now it's a test on how efficiently tokio manages its green threads! I'd be interested in seeing the performance, but with your given constraints it's not likely to beat a simple lock-less queue. Had fun writing it all the same :)

@intendednull
Copy link
Author

intendednull commented Jul 12, 2022

Okay after some basic tests on my machine, this seems to be about twice as fast as the current implementation? I may be doing something wrong, as I'm not sure how you tested.

With these changes I'm getting:

total_time: 5096 success 1000 errors 0 rps 0

And the same parameters on master:

total_time: 10084 success 1000 errors 0 rps 0

@intendednull
Copy link
Author

intendednull commented Jul 12, 2022

Hmm after some more testing, it seems the result vary wildly for both implementations. Sometimes it's just a minor improvement, sometimes it's as shown above.

@ThePrimeagen
Copy link
Owner

So when I add things to the queue they all get added with the exact same amount of time. 15 seconds was my requirement. Or else the queue steady state would never exist.

I'm glad to run that Tokio steady state test though.

To me it seems like I should also throw in tokio MPSC

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.

2 participants