[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [Libevent-users] not make full use of evbuffer_chain
On Fri, Oct 22, 2010 at 8:37 PM, lin yan <sandbox2006@xxxxxxxxx> wrote:
> In libevent-2.0.8-rc, buffer.c: evbuffer_add()
> Code was:
> 1470 in evbuffer_add(struct evbuffer *buf, const void *data_in, size_t
> datlen)
> 1471 {
> ....
> 1505 } else if ((size_t)chain->misalign >= datlen && !CHAIN_PINNED(chain)) {
> ....
> 1551 }
> I think it does not make full use of misalign space in front of
> evbuffer_chain in line 1505.
> It is more likely should be as follow:
> 1470 in evbuffer_add(struct evbuffer *buf, const void *data_in, size_t
> datlen)
> 1471 {
> ....
> 1505 } else if ((size_t)(chain->buffer_len - chain->off) >= datlen &&
> !CHAIN_PINNED(chain)) {
> ....
> 1551 }
> Am I correct?
> Thanks,
> Yan Lin
You're right, and actually we want to be a little more fancy than
that. Check out the rule as implemented in
evbuffer_expand_singlechain: we only want to realign a chain if it
involves moving not-too-much data around. (Doing a 16kb memmove to
save 1 byte of space would be a poor choice indeed!)
Patch attached, under consideration for 2.0.9.
thanks,
--
Nick
From e4f34e8a0fec22a0300d6d529a5899ced3d0582a Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@xxxxxxxxxxxxxx>
Date: Mon, 25 Oct 2010 22:36:23 -0400
Subject: [PATCH] Correct logic for realigning a chain in evbuffer_add
The old logic was both too eager to realign (it would move a whole
chain to save a byte) and too reluctant to realign (it would only
realign when data would fit into the misaligned portion, without
considering the space at the end of the chain).
The new logic matches that from evbuffer_expand_singlechain: it only
realigns a chain when not much data is to be moved, and there's a
bunch of space to be regained.
Spotted by Yan Lin.
---
buffer.c | 27 +++++++++++++++++++--------
1 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/buffer.c b/buffer.c
index eb2d84b..0e6c776 100644
--- a/buffer.c
+++ b/buffer.c
@@ -136,6 +136,8 @@ static int use_mmap = 1;
#define CHAIN_PINNED_R(ch) (((ch)->flags & EVBUFFER_MEM_PINNED_R) != 0)
static void evbuffer_chain_align(struct evbuffer_chain *chain);
+static int evbuffer_chain_should_realign(struct evbuffer_chain *chain,
+ size_t datalen);
static void evbuffer_deferred_callback(struct deferred_cb *cb, void *arg);
static int evbuffer_ptr_memcmp(const struct evbuffer *buf,
const struct evbuffer_ptr *pos, const char *mem, size_t len);
@@ -1503,7 +1505,8 @@ evbuffer_add(struct evbuffer *buf, const void *data_in, size_t datlen)
buf->total_len += datlen;
buf->n_add_for_cb += datlen;
goto out;
- } else if ((size_t)chain->misalign >= datlen && !CHAIN_PINNED(chain)) {
+ } else if (!CHAIN_PINNED(chain) &&
+ evbuffer_chain_should_realign(chain, datlen)) {
/* we can fit the data into the misalignment */
evbuffer_chain_align(chain);
@@ -1638,6 +1641,17 @@ evbuffer_chain_align(struct evbuffer_chain *chain)
#define MAX_TO_COPY_IN_EXPAND 4096
#define MAX_TO_REALIGN_IN_EXPAND 2048
+/** Helper: return true iff we should realign chain to fit datalen bytes of
+ data in it. */
+static int
+evbuffer_chain_should_realign(struct evbuffer_chain *chain,
+ size_t datlen)
+{
+ return chain->buffer_len - chain->off >= datlen &&
+ (chain->off < chain->buffer_len / 2) &&
+ (chain->off <= MAX_TO_REALIGN_IN_EXPAND);
+}
+
/* Expands the available space in the event buffer to at least datlen, all in
* a single chunk. Return that chunk. */
static struct evbuffer_chain *
@@ -1680,14 +1694,11 @@ evbuffer_expand_singlechain(struct evbuffer *buf, size_t datlen)
/* If the misalignment plus the remaining space fulfills our data
* needs, we could just force an alignment to happen. Afterwards, we
- * have enough space. But only do this if we're saving at least 1/2
- * the chain size and moving no more than MAX_TO_REALIGN_IN_EXPAND
- * bytes. Otherwise the space savings are probably offset by the time
- * lost in copying.
+ * have enough space. But only do this if we're saving a lot of space
+ * and not moving too much data. Otherwise the space savings are
+ * probably offset by the time lost in copying.
*/
- if (chain->buffer_len - chain->off >= datlen &&
- (chain->off < chain->buffer_len / 2) &&
- (chain->off <= MAX_TO_REALIGN_IN_EXPAND)) {
+ if (evbuffer_chain_should_realign(chain, datlen)) {
evbuffer_chain_align(chain);
result = chain;
goto ok;
--
1.7.2.3