having a limited number of oh's for private
objects is for debugging: We can now find them. Previously they
had to be tracked down on some thread or other.
Yet, I agree on this point (also mentioned in my first email)
Regarding the actual patch I am working on: [...]
First of all, we did agree that a successfull conditional fetch
should nuke the "old" object from cache, (ie: ignore obj.keep)
right ?
My understanding of what should be done for this feature was basically this:
- add storage steal feature
for conditional backend fetch 304 response:
- if stealing is ok (determined by stevedores involved)
- the new obj from the 304 response steals from expired obj
- expired obj gets a reference on the new obj
- expired obj gets a "my storage has been ripped off" flag
- otherwise copy storage
BUT: I am not working on conditional fetches (IMS) in master, my understanding
was that this is something you still want to do yourself, right?
-
Context on the "expire superseded" feature: Several patches following this idea
had been posted to this list before, here are the references I am aware of:
an initial "poc" patch which i had never taken into production, doing work in
Lookup:
https://www.varnish-cache.org/lists/pipermail/varnish-dev/2010-November/006600.html
Doc Wilco's implementation of the same idea, but working in Unbusy:
https://www.varnish-cache.org/lists/pipermail/varnish-dev/2012-January/007042.html
What I have at this point (and do successfully use in production for one month
now) is a variant of my initial patch for Varnish2 , but _much_ simpler.
I expect that will solve most of the duplication problem ?
The duplication effect we would like to address is simply this: When using a
long beresp.grace time with a short ttl, many copies of the same object will
accumulate until they expire, so usually one will run out of cache and lru nuke
will kick in. But LRU might first expire objects we could still, so selectively
expiring old "grace" copies (I like to call them superseded objects) is a much
better idea.
HSH_Lookup when (oc->objhead == NULL) was still valid:
- remove oc from oh while holding the oh->mtx anyway
- NULL oc->objhead
- EXP_Rearm outside oh->mtx
- deref oh
With HSH_Private, IIUC avoiding a second lock of oh->mtx is not possible any
more, so I wonder if HSH_Lookup sill would be the right place.
1. How could de-dup'ing a oh's list ever be relevant to pass/private
objects ? (ie: I don't understand what HSH_Private() changes for you.
I don't want to de-dupe the private_oh.
I was mainly asking about HSH_Private because of the performance implications
and because of the fact that this changes the implementation required for the
expire superseded feature to what I have now (or, put it this way: one
implementation which I thought was particularly efficient is not possible any more).
In certain evironments this may still degenerate, so it has to be
cheap.
That's what I am after.
For illustrative purposes _only_, you might want to have a look at the varnish2
patch I am using at the moment (attached)
Nils
changeset: 1332:14dcf86fc4f3
user: Nils Goroll <[email protected]>
date: Tue Aug 06 16:33:30 2013 +0200
description:
Expire superseded objects early
When lookup up a cache object, a grace object which we don't use
because we
have found an object with a valid ttl can be removed from the cache.
There is no additional lookup cost to do this and the additional time
spent
with the objhead lock held has been minimized.
Statistic: n_superseded / N superseded objects
diff -r 9772da39f505 -r 14dcf86fc4f3
src/varnish-2.0.3_VSLP/bin/varnishd/cache_hash.c
--- a/src/varnish-2.0.3_VSLP/bin/varnishd/cache_hash.c Tue Aug 06 09:48:21
2013 +0200
+++ b/src/varnish-2.0.3_VSLP/bin/varnishd/cache_hash.c Tue Aug 06 16:33:30
2013 +0200
@@ -225,7 +225,7 @@
{
struct worker *w;
struct objhead *oh;
- struct object *o, *busy_o, *grace_o;
+ struct object *o, *busy_o, *grace_o, *sup_o;
CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
CHECK_OBJ_NOTNULL(sp->wrk, WORKER_MAGIC);
@@ -279,6 +279,24 @@
}
/*
+ * If we have found an object we like _and_ a grace object along the
+ * way, we know that we wont't need the grace object any more
+ */
+ sup_o = NULL;
+ if (o != NULL && grace_o != NULL && o != grace_o) {
+ sup_o = grace_o;
+ grace_o == NULL;
+
+ /*
+ * as we have the oh locked already, avoid taking it another
+ * time in exp_timer->HSH_Deref
+ */
+ VTAILQ_REMOVE(&oh->objects, sup_o, list);
+ sup_o->objhead = NULL;
+ /* do the rest on way out outside the oh lock */
+ }
+
+ /*
* If we have a object in grace
* - and req.grace is also satisfied
* - and we're currently fetching the object
@@ -302,7 +320,7 @@
Lck_Unlock(&oh->mtx);
(void)hash->deref(oh);
- return (o);
+ goto out;
}
if (busy_o != NULL) {
@@ -323,7 +341,8 @@
sp->objhead = oh;
Lck_Unlock(&oh->mtx);
- return (NULL);
+ o = NULL;
+ goto out;
}
/* Insert (precreated) object in objecthead */
@@ -349,6 +364,23 @@
* XXX: possibly move to when we commit to have it in the cache.
*/
BAN_NewObj(o);
+ /* return with looking after the sup_o we removed outside oh->mtx */
+
+ out:
+ if (sup_o) {
+ /* this finalizes sup_o->objhead = NULL, but outside the oh
lock */
+ (void) hash->deref(oh);
+
+ /*
+ * exp_timer will pick it up and call HSH_Deref, but because we
have
+ * unset sup_o->objhead above, we won't need the oh lock again
+ */
+ VSL_stats->n_superseded++;
+ VSL_stats->n_expired--;
+ sup_o->grace = 0;
+ EXP_Rearm(sup_o);
+ }
+
return (o);
}
_______________________________________________
varnish-dev mailing list
[email protected]
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
