[master] f778ad5b9 gzip: Allocate gzip buffers from storage
Dridi Boukelmoune
dridi.boukelmoune at gmail.com
Mon Apr 14 15:49:06 UTC 2025
commit f778ad5b986fadaa22dbec3a0ac49cab3076c81b
Author: Dridi Boukelmoune <dridi.boukelmoune at gmail.com>
Date: Thu Mar 13 10:35:20 2025 +0100
gzip: Allocate gzip buffers from storage
Arbitrary allocation from storage was introduced to solve the h2
head-of-line blocking caused by early DATA frames, allowed by the
initial control flow window of new streams. The decision could have
been made then to allocate the window buffer from the heap, but it
was tied to storage instead, offering better control over the global
memory footprint of the cache process.
The gzip buffer is tied to a task, but too large to allocate from
workspace without posing a risk. Even worse, there may be multiple
concurrent gzip operations in a single task. For example a gunzip
needed to parse ESI followed by a gzip to store a compressed body.
For a similar reason, it was not opportune to allocate the h2 stream
window buffer from workspace, despite being tied to the task.
Following the same logic, the gzip allocation can be performed from
storage to remove one wild card in our heap consumption. Another
possibility could have been the addition of a mempool, and it could
also have been an alternative for the h2 stream window, but transient
storage seemed more appropriate and it matches the on-demand malloc
behavior.
The gzip buffer logic could have been better encapsulated, but the
amount of direct access to the m_buf field would have resulted in a
lot of noise. Most of the noise here is caused by the two function
signatures changed to take a worker argument.
diff --git a/bin/varnishd/cache/cache_esi_fetch.c b/bin/varnishd/cache/cache_esi_fetch.c
index 465b432bd..74771b7d3 100644
--- a/bin/varnishd/cache/cache_esi_fetch.c
+++ b/bin/varnishd/cache/cache_esi_fetch.c
@@ -146,7 +146,7 @@ vfp_esi_end(struct vfp_ctx *vc, struct vef_priv *vef,
if (vef->vgz != NULL) {
if (retval == VFP_END)
VGZ_UpdateObj(vc, vef->vgz, VGZ_END);
- if (VGZ_Destroy(&vef->vgz) != VGZ_END)
+ if (VGZ_Destroy(vc->wrk, &vef->vgz) != VGZ_END)
retval = VFP_Error(vc,
"ESI+Gzip Failed at the very end");
}
diff --git a/bin/varnishd/cache/cache_gzip.c b/bin/varnishd/cache/cache_gzip.c
index 06eb692ec..4f7205c92 100644
--- a/bin/varnishd/cache/cache_gzip.c
+++ b/bin/varnishd/cache/cache_gzip.c
@@ -46,6 +46,9 @@
#include "cache_filter.h"
#include "cache_objhead.h"
#include "cache_vgz.h"
+
+#include "storage/storage.h"
+
#include "vend.h"
#include "vgz.h"
@@ -59,6 +62,7 @@ struct vgz {
int last_i;
enum vgz_flag flag;
+ struct stv_buffer *stvbuf;
char *m_buf;
ssize_t m_sz;
ssize_t m_len;
@@ -151,20 +155,24 @@ VGZ_NewGzip(struct vsl_log *vsl, const char *id)
*/
static int
-vgz_getmbuf(struct vgz *vg)
+vgz_getmbuf(struct worker *wrk, struct vgz *vg)
{
+ size_t sz;
+ CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
CHECK_OBJ_NOTNULL(vg, VGZ_MAGIC);
AZ(vg->m_sz);
AZ(vg->m_len);
AZ(vg->m_buf);
+ AZ(vg->stvbuf);
- vg->m_sz = cache_param->gzip_buffer;
- vg->m_buf = malloc(vg->m_sz);
- if (vg->m_buf == NULL) {
- vg->m_sz = 0;
+ vg->stvbuf = STV_AllocBuf(wrk, stv_transient, cache_param->gzip_buffer);
+ if (vg->stvbuf == NULL)
return (-1);
- }
+ vg->m_buf = STV_GetBufPtr(vg->stvbuf, &sz);
+ vg->m_sz = sz;
+ AN(vg->m_buf);
+ assert(vg->m_sz > 0);
return (0);
}
@@ -311,8 +319,8 @@ vdp_gunzip_init(VRT_CTX, struct vdp_ctx *vdc, void **priv)
vg = VGZ_NewGunzip(vdc->vsl, "U D -");
AN(vg);
- if (vgz_getmbuf(vg)) {
- (void)VGZ_Destroy(&vg);
+ if (vgz_getmbuf(vdc->wrk, vg)) {
+ (void)VGZ_Destroy(vdc->wrk, &vg);
return (-1);
}
@@ -352,7 +360,7 @@ vdp_gunzip_fini(struct vdp_ctx *vdc, void **priv)
(void)vdc;
TAKE_OBJ_NOTNULL(vg, priv, VGZ_MAGIC);
AN(vg->m_buf);
- (void)VGZ_Destroy(&vg);
+ (void)VGZ_Destroy(vdc->wrk, &vg);
return (0);
}
@@ -439,12 +447,13 @@ VGZ_UpdateObj(const struct vfp_ctx *vc, struct vgz *vg, enum vgzret_e e)
*/
enum vgzret_e
-VGZ_Destroy(struct vgz **vgp)
+VGZ_Destroy(struct worker *wrk, struct vgz **vgp)
{
struct vgz *vg;
enum vgzret_e vr;
int i;
+ CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
TAKE_OBJ_NOTNULL(vg, vgp, VGZ_MAGIC);
AN(vg->id);
VSLb(vg->vsl, SLT_Gzip, "%s %jd %jd %jd %jd %jd",
@@ -460,8 +469,11 @@ VGZ_Destroy(struct vgz **vgp)
i = inflateEnd(&vg->vz);
if (vg->last_i == Z_STREAM_END && i == Z_OK)
i = Z_STREAM_END;
- if (vg->m_buf)
- free(vg->m_buf);
+ if (vg->m_buf != NULL) {
+ AN(vg->stvbuf);
+ STV_FreeBuf(wrk, &vg->stvbuf);
+ }
+ AZ(vg->stvbuf);
if (i == Z_OK)
vr = VGZ_OK;
else if (i == Z_STREAM_END)
@@ -514,7 +526,7 @@ vfp_gzip_init(VRT_CTX, struct vfp_ctx *vc, struct vfp_entry *vfe)
}
AN(vg);
vfe->priv1 = vg;
- if (vgz_getmbuf(vg))
+ if (vgz_getmbuf(vc->wrk, vg))
return (VFP_ERROR);
VGZ_Ibuf(vg, vg->m_buf, 0);
AZ(vg->m_len);
@@ -699,7 +711,7 @@ vfp_gzip_fini(struct vfp_ctx *vc, struct vfp_entry *vfe)
if (vfe->priv1 != NULL) {
TAKE_OBJ_NOTNULL(vg, &vfe->priv1, VGZ_MAGIC);
- (void)VGZ_Destroy(&vg);
+ (void)VGZ_Destroy(vc->wrk, &vg);
}
}
diff --git a/bin/varnishd/cache/cache_vgz.h b/bin/varnishd/cache/cache_vgz.h
index 0464bc7f1..386d31bb6 100644
--- a/bin/varnishd/cache/cache_vgz.h
+++ b/bin/varnishd/cache/cache_vgz.h
@@ -53,6 +53,6 @@ int VGZ_ObufFull(const struct vgz *vg);
enum vgzret_e VGZ_Gzip(struct vgz *, const void **, ssize_t *len,
enum vgz_flag);
// enum vgzret_e VGZ_Gunzip(struct vgz *, const void **, ssize_t *len);
-enum vgzret_e VGZ_Destroy(struct vgz **);
+enum vgzret_e VGZ_Destroy(struct worker *wrk, struct vgz **);
void VGZ_UpdateObj(const struct vfp_ctx *, struct vgz*, enum vgzret_e);
diff --git a/include/tbl/params.h b/include/tbl/params.h
index 7c2c7a667..c66c3e8f4 100644
--- a/include/tbl/params.h
+++ b/include/tbl/params.h
@@ -538,10 +538,12 @@ PARAM_SIMPLE(
/* units */ "bytes",
/* descr */
"Size of malloc buffer used for gzip processing.\n"
+ "Size of buffer used for gzip processing.\n"
"These buffers are used for in-transit data, for instance "
- "gunzip'ed data being sent to a client.Making this space to small "
+ "gunzip'ed data being sent to a client. Making this space to small "
"results in more overhead, writes to sockets etc, making it too "
- "big is probably just a waste of memory.",
+ "big is probably just a waste of memory.\n"
+ "Gzip buffers are allocated from Transient storage.",
/* flags */ EXPERIMENTAL
)
More information about the varnish-commit
mailing list