Masterwork From Distant Lands
ActivePublic
Actions

Authored by ProdPasteBot on Jan 22 2021, 1:28 PM.

Tags

None

Referenced Files

	F34011837: Masterwork From Distant Lands
	Jan 22 2021, 1:28 PM

Subscribers

None

	Description: revert #1799 changes introduced between varnish 6.0.0 and 6.0.1
	We suspect that the changes introduced with
	https://github.com/varnishcache/varnish-cache/pull/2705 between varnish 6.0.0
	and 6.0.1 might be responsible for the performance regression described in
	https://phabricator.wikimedia.org/T264398
	Author: Emanuele Rocca <ema@wikimedia.org>

	Index: varnish-6.0.1/bin/varnishd/cache/cache.h
	===================================================================
	--- varnish-6.0.1.orig/bin/varnishd/cache/cache.h
	+++ varnish-6.0.1/bin/varnishd/cache/cache.h
	@@ -470,7 +470,6 @@ struct req {
	uint8_t digest[DIGEST_LEN];

	double d_ttl;
	- double d_grace;

	ssize_t req_bodybytes; /* Parsed req bodybytes */
	const struct stevedore *storage;
	Index: varnish-6.0.1/bin/varnishd/cache/cache_expire.c
	===================================================================
	--- varnish-6.0.1.orig/bin/varnishd/cache/cache_expire.c
	+++ varnish-6.0.1/bin/varnishd/cache/cache_expire.c
	@@ -75,24 +75,6 @@ EXP_Ttl(const struct req *req, const str
	}

	/*--------------------------------------------------------------------
	- * Calculate an object's effective ttl+grace time, taking req.grace into
	- * account if it is available.
	- */
	-
	-double
	-EXP_Ttl_grace(const struct req req, const struct objcore oc)
	-{
	- double g;
	-
	- CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
	-
	- g = oc->grace;
	- if (req != NULL && req->d_grace >= 0. && req->d_grace < g)
	- g = req->d_grace;
	- return (EXP_Ttl(req, oc) + g);
	-}
	-
	-/*--------------------------------------------------------------------
	* Post an objcore to the exp_thread's inbox.
	*/

	Index: varnish-6.0.1/bin/varnishd/cache/cache_hash.c
	===================================================================
	--- varnish-6.0.1.orig/bin/varnishd/cache/cache_hash.c
	+++ varnish-6.0.1/bin/varnishd/cache/cache_hash.c
	@@ -478,45 +478,44 @@ HSH_Lookup(struct req *req, struct objco
	busy_found = 0;
	}

	- if (!busy_found) {
	- /* Insert objcore in objecthead */
	- *bocp = hsh_insert_busyobj(wrk, oh);
	+ if (exp_oc != NULL) {
	+ assert(oh->refcnt > 1);
	+ assert(exp_oc->objhead == oh);

	- if (exp_oc != NULL) {
	- assert(oh->refcnt > 1);
	- assert(exp_oc->objhead == oh);
	+ if (!busy_found) {
	+ *bocp = hsh_insert_busyobj(wrk, oh);
	+ retval = HSH_EXPBUSY;
	+ } else {
	+ AZ(req->hash_ignore_busy);
	+ /*
	+ * here we have a busy object, but if the stale object
	+ * is not under grace we go to the waiting list
	+ * instead of returning the stale object
	+ */
	+ if (EXP_Ttl(req, exp_oc) + exp_oc->grace < req->t_req)
	+ retval = HSH_BUSY;
	+ else
	+ retval = HSH_EXP;
	+ }
	+ if (retval != HSH_BUSY) {
	exp_oc->refcnt++;
	- Lck_Unlock(&oh->mtx);
	- *ocp = exp_oc;

	- if (EXP_Ttl_grace(req, exp_oc) < req->t_req) {
	- retval = HSH_MISS;
	- } else {
	- if (exp_oc->hits < LONG_MAX)
	- exp_oc->hits++;
	- retval = HSH_EXPBUSY;
	- }
	- } else {
	+ if (exp_oc->hits < LONG_MAX)
	+ exp_oc->hits++;
	Lck_Unlock(&oh->mtx);
	- retval = HSH_MISS;
	+ if (retval == HSH_EXP)
	+ assert(HSH_DerefObjHead(wrk, &oh));
	+ *ocp = exp_oc;
	+ return (retval);
	}
	-
	- return (retval);
	}

	- AN(busy_found);
	- if (exp_oc != NULL && EXP_Ttl_grace(req, exp_oc) >= req->t_req) {
	- /* we do not wait on the busy object if in grace */
	- assert(oh->refcnt > 1);
	- assert(exp_oc->objhead == oh);
	- exp_oc->refcnt++;
	+ if (!busy_found) {
	+ /* Insert objcore in objecthead and release mutex */
	+ *bocp = hsh_insert_busyobj(wrk, oh);
	+ /* NB: no deref of objhead, new object inherits reference */
	Lck_Unlock(&oh->mtx);
	- *ocp = exp_oc;
	-
	- assert(HSH_DerefObjHead(wrk, &oh));
	- if (exp_oc->hits < LONG_MAX)
	- exp_oc->hits++;
	- return (HSH_EXP);
	+ return (HSH_MISS);
	}

	/* There are one or more busy objects, wait for them */
	Index: varnish-6.0.1/bin/varnishd/cache/cache_req_fsm.c
	===================================================================
	--- varnish-6.0.1.orig/bin/varnishd/cache/cache_req_fsm.c
	+++ varnish-6.0.1/bin/varnishd/cache/cache_req_fsm.c
	@@ -508,15 +508,13 @@ cnt_lookup(struct worker *wrk, struct re

	AZ(req->objcore);
	if (lr == HSH_MISS) {
	+ /* Found nothing */
	+ AZ(oc);
	if (busy != NULL) {
	- /* hitmiss, out-of-grace or ordinary miss */
	AN(busy->flags & OC_F_BUSY);
	req->objcore = busy;
	- req->stale_oc = oc;
	req->req_step = R_STP_MISS;
	} else {
	- /* hitpass */
	- AZ(oc);
	req->req_step = R_STP_PASS;
	}
	return (REQ_FSM_MORE);
	@@ -807,7 +805,6 @@ cnt_recv_prep(struct req *req, const cha
	AN(req->director_hint);

	req->d_ttl = -1;
	- req->d_grace = -1;
	req->disable_esi = 0;
	req->hash_always_miss = 0;
	req->hash_ignore_busy = 0;
	Index: varnish-6.0.1/bin/varnishd/cache/cache_vrt_var.c
	===================================================================
	--- varnish-6.0.1.orig/bin/varnishd/cache/cache_vrt_var.c
	+++ varnish-6.0.1/bin/varnishd/cache/cache_vrt_var.c
	@@ -460,8 +460,6 @@ REQ_VAR_L(backend_hint, director_hint, V
	REQ_VAR_R(backend_hint, director_hint, VCL_BACKEND)
	REQ_VAR_L(ttl, d_ttl, VCL_DURATION, if (!(arg>0.0)) arg = 0;)
	REQ_VAR_R(ttl, d_ttl, VCL_DURATION)
	-REQ_VAR_L(grace, d_grace, VCL_DURATION, if (!(arg>0.0)) arg = 0;)
	-REQ_VAR_R(grace, d_grace, VCL_DURATION)

	/--------------------------------------------------------------------/

Event Timeline

ProdPasteBot edited the content of this paste. (Show Details)Jan 22 2021, 1:28 PM

ProdPasteBot changed the title of this paste from untitled to Masterwork From Distant Lands.

• ema mentioned this in T264398: 8-10% response start regression (Varnish 5.1.3-1wm15 -> 6.0.6-1wm1).Jan 22 2021, 1:39 PM

Masterwork From Distant LandsActivePublicActions

Event Timeline

Masterwork From Distant Lands
ActivePublic
Actions