fix: make WeakConnectionHandle more lightweight #276
Conversation
…ion for a ConnectionInner alive ConnectionInner is quite large, so before the weak handle would keep a large allocation alive. Now it just keeps a tiny allocation alive, consisting of a pointer and 2 refcounts. The downside of this approach is that it does a tiny allocation in fn weak_handle. The alternative would be to do the double arc inside Connection, which would make every connection access a double dereference, but prevent this allocation.
| /// Returns a weak reference to the inner connection struct. | ||
| pub fn weak_handle(&self) -> WeakConnectionHandle { | ||
| WeakConnectionHandle(Arc::downgrade(&self.0.0)) | ||
| WeakConnectionHandle(Arc::downgrade(&Arc::new(self.0.0.clone()))) |
There was a problem hiding this comment.
This is the allocation we have to do.
There was a problem hiding this comment.
I'm confused, is Arc::downgrade(&Arc::new(..)) not leading to a strong count of 0, the inner value being dropped right away and thus all upgrades would fail?
There was a problem hiding this comment.
Ooof yes, this is happening, just verified it in my example.
There was a problem hiding this comment.
... which also shows we're very much missing tests for this both in quinn and in iroh
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/quinn/pr/276/docs/iroh_quinn/ Last updated: 2025-12-17T12:22:43Z |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #276 +/- ##
==========================================
- Coverage 76.69% 76.67% -0.02%
==========================================
Files 83 83
Lines 23241 23241
==========================================
- Hits 17824 17821 -3
- Misses 5417 5420 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ramfox
left a comment
ramfox
left a comment
There was a problem hiding this comment.
From testing on iroh in n0-computer/iroh#3785 , this indeed has the expected impact!
flub
left a comment
flub
left a comment
There was a problem hiding this comment.
Just making sure this doesn't show up as ready because it seems buggy.
My understanding is that this is some kind of release blocker, adding to appropriate milestone.
|
Not sure what I was thinking, this doesn't work. So closing this in favour of the version that always does a double deref: #277 |
Description
Prevent WeakConnectionHandle from keeping the allocation for a ConnectionInner alive.
ConnectionInner is quite large, 6048 bytes when I measured it. Arc is just an 8 byte pointer. So before the weak handle would keep a large allocation alive. Now it just keeps a tiny allocation alive, consisting of a pointer and 2 refcounts, so 24 bytes on 64 bit machines.
The downside of this approach is that it does a tiny allocation in fn weak_handle. The alternative would be to do the double arc inside Connection, which would make every connection access a double dereference, but prevent this allocation.
I would assume that tiny allocations are pretty fast though. But just in case we want to do the other variant, here it is: #277
Breaking Changes
None
Notes & open questions