Skip to content
This repository was archived by the owner on Feb 16, 2026. It is now read-only.

Director reference counts - #3599

Closed
bsdphk wants to merge 5 commits into
varnishcache:masterfrom
bsdphk:vdir_refcnt
Closed

bsdphk wants to merge 5 commits into
varnishcache:masterfrom
bsdphk:vdir_refcnt

Conversation

@bsdphk

@bsdphk bsdphk commented Apr 21, 2021

Copy link
Copy Markdown
Contributor

Replace the director cooling list with reference counts, the cheapest way I could find.

@nigoroll

Copy link
Copy Markdown
Member

This patch takes the cost of each set {req.backend_hint,bereq.backend} in VCL from a pointer assignment to two or four mutex operations. This cost was the primary motivation for the extension of the coollist idea which was the basis for #2725. The criticism which killed that PR was that it required two mutex operations per task, and my plan was to move acquisition of the coollist reference to under the vcl_mtx, which should basically eliminate any additional overhead for normal VCL execution.
I agree that the coollist concept brings with it additional complexity, so I would first like to come to a decision which of the two concepts to pursue.

If we were to go in the direction of this PR, I wonder if we needed to consider this scenario: We must destroy all directors from a vmod before unloading that vmod. Because of cross-vmod director references, I think that directors also need to gain a reference on their defining vmod, which is returned when the director refcount goes to zero. That, in turn, might require us rechoreograph the vmod fini callback -> VRT_DelDirector() dance.

@nigoroll

Copy link
Copy Markdown
Member

pow-wow: maybe we could get out of the complexity vs. efficiency dilemma by introducing a director scope. References would only need to be taken for scopes smaller than the VCL.

@nigoroll

Copy link
Copy Markdown
Member

It looks like this should work. I would like to play a little with actual dynamic backends.

@nigoroll
nigoroll self-requested a review April 26, 2021 12:02
@mbgrydeland

Copy link
Copy Markdown
Contributor

It is my experience that dynamic backends are in more common use nowadays than the static ones. Especially in virtual and hosted environments, dynamically resolved backends are a must-have. So we should not dismiss the added cost presented to those as a small corner case.

One avenue that we may consider is to lessen the cost of this extra refcounting when it is unnecessary. That is, when a backend isn't needed (cache hits), we shouldn't have to take a reference to the backend either. One way to encourage this could be to use the 8.0 major release opportunity to retire req.backend_hint, making it explicit that this is only for the bereq object.

Though removing req.backend_hint does present a problem for another use case. One not uncommon construct is to vary the amount of allowed grace time based on the backend health. This necessarily has to be done from vcl_recv, and requiring an early ref.

I find it hard to come up with a good solution in this space, though I will readily agree that the current timer based late removal is far from a good solution.

The patch itself looks sound to me.

bsdphk added a commit that referenced this pull request Nov 24, 2021
bsdphk added a commit that referenced this pull request Nov 24, 2021
bsdphk added a commit that referenced this pull request Nov 24, 2021
bsdphk added a commit that referenced this pull request Nov 24, 2021
bsdphk added a commit that referenced this pull request Nov 24, 2021
@bsdphk

bsdphk commented Nov 24, 2021

Copy link
Copy Markdown
Contributor Author

This has now been committed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants