Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Buffer references #24

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

fancycode commented Aug 29, 2011

Here are my latest patches to support non-destructive adds to buffers.

Owner

nmathewson commented Aug 29, 2011

Thanks! Will review soon. (I have a big work deadline coming up at the end of the month)

Owner

nmathewson commented Sep 10, 2011

Hm. I think this could be right. So every multicast chain holds a reference to the canonical copy of that chain, and to the evbuffer that contains it? And a chain's reference count implicitly includes the main evbuffer, so that a chain with refcnt 0 is freed only if the parent evbuffer is freed? And because the multicast chains hold a reference to the other evbuffer, the evbuffer won't get freed until every chain that points to it is freed, so its lock will definitely stay around as long as is needed?

This seems workable, but it's pretty subtle. It could really use some cmments in the source to explain these rules (if I have them right) or the real rules (if I have them wrong) so we can't mess them up in the future.

One think I'm wondering too: if we hit evbuffer_chain_free on a chain, but we don't free it then because some other chain is referring to it... when do we free it? Its refcnt could hit 0, but nothing else checks for a refcnt that becomes 0 as far as I can see.

Contributor

fancycode commented Oct 17, 2011

Sorry for the late reply, I have been really busy at work. You got the rules right, referenced chains are refcounted with the buffers they are originally stored at. So a referenced chain is only freed if its own refcount and the refcount of its original buffer reach zero.

That's why there is no additional check required on referenced chains - their refcount reaches zero when the last referencing chain is freed (which always have a refcount of 0) and they then get freed by the call to _evbuffer_decref_and_unlock when the last referencing chain is freed by evbuffer_chain_free.

Hope that makes it a bit more clear, I'll try to write some comments in the code.

Contributor

fancycode commented Oct 17, 2011

More comments in ba24f61, is this enough or do you want more clarification?

@fancycode fancycode closed this Oct 17, 2011

@fancycode fancycode reopened this Oct 17, 2011

Contributor

fancycode commented Oct 17, 2011

Whoops, shouldn't have closed this...

Owner

nmathewson commented Oct 17, 2011

(Sorry for delay in reply; will be out of town and unable to do much more
than mail till thu/fri probably. I hope to respond then.)

Contributor

fancycode commented Nov 24, 2011

Anything else I can to do on this, in order to get it merged sometime?

Owner

nmathewson commented Nov 24, 2011

Sorry; please keep reminding me. I am swamped with work this month, but I hope I can deal with some of my libevent backlog soon too.

Owner

nmathewson commented Dec 7, 2011

Okay, just took another look at it. I still don't think that the reference counting on chains can be right.

Here's an example of what I mean:

  1. Suppose "src" is a buffer with one chain (call it C), and "ref" is an empty buffer.
  2. We do evbuffer_add_buffer_reference(ref, src). Now the reference count for C is 1 and the reference count for src is 2.
  3. We do evbuffer_drain(src, evbuffer_get_length(src). Now src is empty. The reference count for C is still 1, so it doesn't get freed; the reference count for src is 2. src no longer has a pointer to C.
  4. We do evbuffer_drain(ref, evbuffer_get_length(ref)). Now ref is empty. evbuffer_chain_free calls evbuffer_chain_decref on C, so now its reference count is 0. It also calls evbuffer_decref_and_unlock(src), so now the refcnt of src is 1.
  5. We call evbuffer_free(src). Now src gets freed. But src no longer has a pointer to C, so C never actually gets freed.

What am I getting wrong here?

Contributor

fancycode commented Dec 7, 2011

Ah now I see. You're right, that's a problem. It probably can be solved by using the pin/unpin mechanism, by pinning the chain once it is referenced and unpin when the refcount reaches 0. Then it's freed if it has the "dangling" flag set (which I think would be the case for step 3 if pinned before).

I can change the code accordingly if you think this is this a valid solution.

Owner

nmathewson commented Dec 7, 2011

I wonder if you could just simply use reference counts for this: treat the original reference from src to C as reference 1, and then free C as soon as its reference count hits zero.

(Right now, your patch only implements the "don't free C as long as it has references" part, when reference counting usually also includes a step for "free C as soon as it has no more references.")

Contributor

fancycode commented Dec 7, 2011

With my latest commit, the refcounting is changed as you suggested. I also added your example as test to be able to check with valgrind that the chain C is freed.

Owner

nmathewson commented Dec 8, 2011

Looks good; I've merged it to master. Thanks for the code, and for your patience!

@nmathewson nmathewson closed this Dec 8, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment