-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
Simplifications and performance improvements #200
Conversation
Add MT connection count without Mutex
The StringKeyCache is now only used inside a connection. It's assumed that connections are not used concurrently with multiple queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Simplification is always great 🚀
I was wondering whether the breaking change should warrant a new major release, but realized crystal-db is still in 0.x, so we're effectively doing major pre-1.0 releases anyway.
This is great! ❤️ 🚀 ❤️ Took a bit but here are some synthetic numbers of before and after this change: Test system:
pool_concurrency_test.crBuilds:
Comparing:
Single-thread (ST)
Multi-thread (MT)
I need to investigate a bit MT crash with SQLite3 but this performs much better! Thank you again for the time investigating this @bcardiff 👏🏽 ❤️ |
This PR performs a couple of changes that will hopefully allow further performance improvements. The additions are:
Pool#checkout_some
(this is a breaking-change, but is an API that is not advertised to be used). Having a smaller Pool api will allow use to play easily with its implementation.For single-thread I have seen 1.7% increase on query throughput. While for multi-thread I have seen a 74% increase. Don't jump too much, the overall throughput for MT is still far from the ST one (For me MT is about 25% of ST for 4 crystal workers).
The concurrency test sometimes hangs in MT mode. This is even before the changes so it's something we are able to reproduce more and continue to investigate.
Thanks to @luislavena for bringing awareness of the issue and validating some of these changes.