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

Add a catflap to control HSH_Lookup from within the object iteration #2858

Closed
wants to merge 3 commits into from

Conversation

nigoroll
Copy link
Member

@nigoroll nigoroll commented Dec 5, 2018

If non-NULL, req->catflap_init is called before the cache lookup. It may set a catflap_f callback to be called for objcores and a catflap_fini_f to be called after the objcore iterator loop finishes.

A catflap_f may return any of enum catflap_e: FLP_CONTINUE to continue the loop (and get to look at the next object, if any), FLP_DEFAULT to fall through to the built-in default handling, FLP_MISS to force a miss or FLP_HIT to consider the objcore a hit.

A catflap_fini_f, if present, gets a final say on the object beauty contest. For example, a catflap_f may have saved the most beautiful object in the private pointer, but the decision is only final after all objects were considered. The return value of catflap_fini_f is either the object to be hit, or NULL to force a cache miss. The remaining decision on the details (e.g. if it was a HITMISS or HITPASS) happens as before.

An oc == NULL argument to catflap_fini_f signals that the catflap had been called on all objects, otherwise the iteration was terminated early.

Catflap chaining has deliberately been left out of the core implementation. vmods wishing to chain catflaps can still do so (by calling req->catflap_init and deploying a custom cat which knows how to
operate additional catflaps).

@nigoroll
Copy link
Member Author

nigoroll commented Dec 5, 2018

BTW, this is 50% of the open issues from #1799

@nigoroll
Copy link
Member Author

updated, rebased and merged the aws alloc demo into a single commit

If non-NULL, req->catflap_init is called before the cache lookup. It may
set a catflap_f callback to be called for objcores and a catflap_fini_f
to be called after the objcore iterator loop finishes.

A catflap_f may return any of enum catflap_e: FLP_CONTINUE to continue
the loop (and get to look at the next object, if any), FLP_DEFAULT to
fall through to the built-in default handling, FLP_MISS to force a miss
or FLP_HIT to consider the objcore a hit.

A catflap_fini_f, if present, gets a final say on the object beauty
contest. For example, a catflap_f may have saved the most beautiful
object in the private pointer, but the decision is only final after all
objects were considered. The return value of catflap_fini_f is either
the object to be hit, or NULL to force a cache miss. The remaining
decision on the details (e.g. if it was a HITMISS or HITPASS) happens as
before.

An oc == NULL argument to catflap_fini_f signals that the catflap had
been called on all objects, otherwise the iteration was terminated early.

Catflap chaining has deliberately been left out of the core
implementation. vmods wishing to chain catflaps can still do so (by
calling req->catflap_init and deploying a custom cat which knows how to
operate additional catflaps).
- make the cat a union so common native data types can be saved without
  casting a pointer

- add the exp_oc to the catflap and fini_f so the catflap can get a say
  about the expired object also
@hermunn
Copy link
Member

hermunn commented Feb 25, 2019

I agree with the latest changes, and I think this is necessary to close #1799 in a satisfactory way.

@nigoroll
Copy link
Member Author

bugwash:

@bsdphk bsdphk closed this in 141ea56 Mar 4, 2019
@nigoroll
Copy link
Member Author

nigoroll commented Mar 4, 2019

FTR I am not entirely happy with 141ea56 for the following reasons:

  • (commit message): The union was a suggestion by @hermunn which I added because he said it would be something @bsdphk would prefer. I am not happy about the fact that this is taken as an argument to reject the PR, in particular because my original commit had a void *
  • Using multiple function pointers saves conditionals in the callbacks, which reduces the amount of work (and cpu pipeline stalls) under the oh mutex. Keeping the catflap efficient was a major concern from the start
  • The init function can be replaced by additional work for the first call with state==0. But then even more conditionals are required under the oh mutex to avoid the init-work for additional ocs.
  • The original proposal would have allowed for relatively simple catflap chaining: A catflap with chaining support could have saved the function and priv pointer to the next and called it if needed. With the reworked code I think chaining requires the last catflap in the chain to restore the first one's VCF

@bsdphk
Copy link
Contributor

bsdphk commented Mar 4, 2019

If @hermunn thinks I like unions, he should wonder why I put so few of them in Varnish :-)

Your concerns about efficiency and chaining are noted.

If real-world experience with this prototype-API indicates they are significant concerns, they will can be addressed, once we know what we need to fix.

But right now we are in "... no example at all." territory, so simplicity carries the day.

rezan pushed a commit to rezan/varnish-cache that referenced this pull request Jul 11, 2019
I have reimplemented this based on Nils's varnishcache#2858, because I found
it too complex and intrusive.  (In particular we try to avoid
unions in Varnish).

Testcase m00051 by:	Nils Goroll

Closes:	varnishcache#2858
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants