Reuse the oldest keep-alive connection first

[bdraco]

Previously we would reuse the newest one which meant as more and more connections were cycled the first one in the list became more and more stale until it was more likely to drop

Change the order from LIFO to FIFO

[codspeed-hq]

CodSpeed Performance Report

Merging #9672 will not alter performance

_{Comparing connection_reuse_oldest (75169a6) with master (75ae623)}

Summary

✅ 13 untouched benchmarks

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.66%. Comparing base (75ae623) to head (75169a6).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #9672   +/-   ##
=======================================
  Coverage   98.66%   98.66%           
=======================================
  Files         116      116           
  Lines       35638    35638           
  Branches     4225     4225           
=======================================
  Hits        35164    35164           
  Misses        319      319           
  Partials      155      155           

Flag	Coverage Δ
CI-GHA	98.55% <100.00%> (ø)
OS-Linux	98.23% <100.00%> (ø)
OS-Windows	96.11% <100.00%> (ø)
OS-macOS	97.42% <100.00%> (-0.01%)	⬇️
Py-3.10.11	97.28% <100.00%> (+1.60%)	⬆️
Py-3.10.15	97.71% <100.00%> (ø)
Py-3.11.10	97.77% <100.00%> (ø)
Py-3.11.9	97.36% <100.00%> (-0.01%)	⬇️
Py-3.12.7	98.25% <100.00%> (ø)
Py-3.13.0	98.24% <100.00%> (ø)
Py-3.9.13	97.20% <100.00%> (+<0.01%)	⬆️
Py-3.9.20	97.62% <100.00%> (ø)
Py-pypy7.3.16	97.24% <97.61%> (-0.01%)	⬇️
VM-macos	97.42% <100.00%> (-0.01%)	⬇️
VM-ubuntu	98.23% <100.00%> (ø)
VM-windows	96.11% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Dreamsorcerer]

Is it possible that's a deliberate decision to avoid keeping too many connections open? e.g. A quick burst opens up 3 connections, then going forward only 1 request is needed at a time, so the other 2 connections will get closed. Whereas this will stop connections from getting closed.

If you have evidence this approach is better, I'm fine with it. Just checking it's thought through (and maybe a comment added to the code to explain the design choice).

[bdraco]

Is it possible that's a deliberate decision to avoid keeping too many connections open? e.g. A quick burst opens up 3 connections, then going forward only 1 request is needed at a time, so the other 2 connections will get closed. Whereas this will stop connections from getting closed.

If you have evidence this approach is better, I'm fine with it. Just checking it's thought through (and maybe a comment added to the code to explain the design choice).

My original thought was that it might make sense to reuse the newest one since its less likely to be closed, but it seems more connections get reused in production with the FIFO approach.

I still want to do more testing with this though before I move it forward. Since it conflicts with #9671 I've put it on the back burner as that one seems much more important to solve.

[asvetlov]

FIFO sounds like a good strategy for me.
I don't recall the reason for implementing LIFO.
It sits in the code from the very beginning.
I suspect it was added by accident.

[bdraco]

78% connection reuse before
84% connection reuse after

[bdraco]

On another system where there is more /10s polling

73% before
86% after

[bdraco]

So its a little bit more likely to reuse a connection. YMMV based on the server keep alive timeout.

[patchback]

Backport to 3.11: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply afb5ebb on top of patchback/backports/3.11/afb5ebb912ebe1cf88e0d2c19d5f240ffb0b72ee/pr-9672

Backporting merged PR #9672 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.11/afb5ebb912ebe1cf88e0d2c19d5f240ffb0b72ee/pr-9672 upstream/3.11

4. Now, cherry-pick PR Reuse the oldest keep-alive connection first #9672 contents into that branch:

   $ git cherry-pick -x afb5ebb912ebe1cf88e0d2c19d5f240ffb0b72ee

   If it'll yell at you with something like fatal: Commit afb5ebb912ebe1cf88e0d2c19d5f240ffb0b72ee is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x afb5ebb912ebe1cf88e0d2c19d5f240ffb0b72ee

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Reuse the oldest keep-alive connection first #9672 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.11/afb5ebb912ebe1cf88e0d2c19d5f240ffb0b72ee/pr-9672

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[Olegt0rr]

On another system where there is more /10s polling

73% before 86% after

@bdraco,
Could you share your code snippet to reproduce this measurement?
Wanna try it on my environment
Thanks :)

[bdraco]

On another system where there is more /10s polling

73% before 86% after

@bdraco,

Could you share your code snippet to reproduce this measurement?

Wanna try it on my environment

Thanks :)

Sorry there is no snippet. I was profiling a few different Home Assistant instances to gauge the effect with a real world use case.

[bdraco]

The change is available in the 3.11.0 betas if you would like to give it a test

[bdraco]

If you do a callgrind, look at the number of __aenter__ calls from from connect. That will tell you how many didn't get reused