search

Astro36 / qp

Quick Pool: High Performance Async Generic Pool
MIT License
17 stars 3 forks source link

qp deadlocks in Astro36/rust-pool-benchmark #2

Open bikeshedder opened 2 years ago

bikeshedder commented 2 years ago

I haven't been able to figure out what's wrong with it but I'm curious why that's happening:

I'm opening this as I'm currently evaluating the possibility of going back to a crossbeam based implementation in deadpool. I only switched to a Mutex<VecDeque> in 0.7 as I saw a neglectable performance impact back then. It also made features like resizable pools trivial.

I've looked at your Semaphore implementation and the one from async-lock. I gave both a try and either one performs better than the one from tokio. I'm just very hesitant to switch until we figure out why the benchmark deadlocks.

Using a different Semaphore implementation and crossbeam-queue yields a performance improvement of 2x-3x which is quite amazing and worth exploring.

I also opened an issue in the bb8 issue tracker, but since it has a completely different implementation and doesn't even use semaphores I don't think those problems are related in any way:

I just hope it's not an error in the benchmark implementation and I've opened those two issues in error. :see_no_evil:

romanstingler commented 1 year ago

@bikeshedder for me the qp-postgres-v0.1 works but as you said the qp-v0.2.0 fails, If I increase the pool_size to exactly or higher my CPU thread count, the benchmark passes. Could you check the on your machine?

bikeshedder commented 1 year ago

If you do increase the pool_size >= num_cpus it's very likely that the semaphore is never contested. You could try adding some tokio::task::yield_now calls. Though there isn't a great alternative to real CPU cores except for more real CPU cores.

Connection pools need a fair and correct semaphore implementation. Even async-lock fails in one of those regards:

To my knowledge only one fair and correct semaphore for async code exists. That's the one from the tokio project.

This is the reason why I'm sticking with the tokio::sync::Semaphore in deadpool for now. It's fair and proven correct.

Personally I wouldn't trust bb8 or qp as long as those issues haven't been addressed. Any high load situation can easily render your program unuseable. It doesn't even have to be a DoS attack. Even normal load can trigger this issue.

romanstingler commented 1 year ago

@bikeshedder thank you for your answer, I just started learning Rust and I started porting some of our PHP Controllers to Rust and use Axum and diesel_async and tested with it's deadpool and bb8 feature flag.

Within various benchmarks and concurrent testing, I couldn't reproduce a hang on bb8. Please note, that this is just a prototype and not meant for production use. I love your passion and your work.