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

Performance regression when getting multiple connections with r2d2::Pool #3156

Closed
3 tasks done
heyrict opened this issue May 1, 2022 · 2 comments
Closed
3 tasks done
Labels

Comments

@heyrict
Copy link
Contributor

heyrict commented May 1, 2022

Setup

Versions

  • Rust: rustc 1.62.0-nightly (e85edd9a8 2022-04-28)
  • Diesel: v2.0.0-rc0
  • Database: postgresql
  • Operating System Linux 5.17.3-arch1-1

Feature Flags

  • diesel: ["postgres", "r2d2"]

Problem Description

As is reported in #3144, performance regression is observed when upgrading diesel from v1.4.8 to v2.0.0-rc0. Aggregation queries (e.g. count(*) are particularly affected, while the performance hit on normal queries is minimal.

What are you trying to accomplish?

What is the expected output?

The performance of v2.0.0-rc0 should be on par with v1.4.8.

What is the actual output?

cargo bench output of table.filter(id.eq(1)).count().get_result():

hash setup perf
d0bbda8 diesel 1.4 + r2d2 185,936 ns/iter (+/- 15,282)
599c492 diesel 2.0 + r2d2 1,388,410 ns/iter (+/- 158,035)
d3c23ba diesel 1.4 - r2d2 103,562 ns/iter (+/- 2,997)
58d1210 diesel 2.0 - r2d2 109,238 ns/iter (+/- 5,887)

Are you seeing any additional errors?

Steps to reproduce

The minimal reproduction sample is here.

Checklist

  • This issue can be reproduced on Rust's stable channel. (Your issue will be
    closed if this is not the case)
  • This issue can be reproduced without requiring a third party crate
@heyrict heyrict added the bug label May 1, 2022
@weiznich
Copy link
Member

weiznich commented May 2, 2022

This is caused by broken R2D2Connection::is_broken implementations in diesel. Those result in marking any connection as broken as soon as it is checked into the pool again. In turn that results in not reusing connections at all, so that the "pool" reestablish a new connection every time you checked out an connection from the pool.

#3159 addresses this. Can you verify that this solves the issue for you?

@heyrict heyrict changed the title Performance regression in aggregation queries in r2d2::Pool Performance regression with getting multiple connections with r2d2::Pool May 3, 2022
@heyrict
Copy link
Contributor Author

heyrict commented May 3, 2022

I've changed the title to reflect the actual bug. Can confirm the bug is fixed with your PR. Thank you for debugging this 👍

@heyrict heyrict changed the title Performance regression with getting multiple connections with r2d2::Pool Performance regression when getting multiple connections with r2d2::Pool May 3, 2022
weiznich added a commit that referenced this issue May 3, 2022
weiznich added a commit to weiznich/diesel that referenced this issue Jun 16, 2022
Our `R2D2Connection::is_broken` implementations where broken in such a
way that they marked any valid connection as broken as soon as it was
checked into the pool again. This resulted in the pool opening new
connections everytime a connection was checked out of the pool, which
obviously removes the possibility of reusing the same connection again
and again. This commit fixes that issue and adds some tests to ensure
that we do not break this again in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants