Re: CVS update: subversion/subversion/libsvn_fs rev-table.c

[Ben Collins-Sussman <su...@newton.ch.collab.net>]

jimb@tigris.org writes:

>   * include/svn_fs.h (svn_fs_free_file_contents): Function deleted.
>   svn_fs_file_contents no longer returns a baton; instead, it returns an
>   svn_stream_t, which should be freed using svn_stream_close.

Let's talk about that last sentence.

Here's a quote from svn_io.h, where GregH is decribing our generic
stream interface:

   The creator of a stream sets functions to handle read and write.
   Both of these handlers accept a baton whose value is determined at
   stream creation time; this baton can point to a structure
   containing data associated with the stream.  If a caller attempts
   to invoke a handler which has not been set, it will generate a
   runtime assertion failure.  The creator can also set a handler for
   close requests so it can clean up data associated with the baton;
   if a close handler is not specified, a close request on the stream
   will simply be ignored.  Note that svn_stream_close() does not
   deallocate the memory associated with the stream; destroy the pool
   you created the stream in to do that.

So in GregH's last sentence, what does "memory associated with the
stream" mean?  Does it mean "the memory in which the stream structure
is allocated"?  Or does it also mean "any memory used in the process
of accessing the stream"?

And what does his phrase "data associated with the baton" mean?
Remember that users calling svn_stream_close() have no idea that any
kind of stream-baton was created under the hood.  Most of them will
think what Jim assumed in his commit message: that svn_stream_close
will somehow free the stream object itself.

In any case, I'm wondering how these promises relate to the actual
current implementation of svn_fs_file_contents() and the read-stream
it returns.  Right now, the stream object is being allocated in the
provided pool, *and* the secret "baton" (which contains a copy of the
entire contents of the file) is also being allocated there.

So -- my interpretation of this interface is that svn_stream_close()
should destroy only the invisible baton containing file contents.
To deallocate the stream object, just free the original pool.

(Of course, why would anyone *want* that kind of control?  What use
would the stream object be if its hidden baton were destroyed?  Why
would you even want the stream object around anymore?)

Thoughts?  Is this interpretation correct?

Re: pool usage (was: Re: CVS update: subversion/subversion/libsvn_fs rev-table.c)

[Jim Blandy <ji...@zwingli.cygnus.com>]

Greg Stein <gs...@lyra.org> writes:
> I believe the hardest question is how the caller should determine their own
> behavior. Let's say the caller has an unbounded loop, creating streams. They
> cannot simply pass the same pool to each svn_stream_create() function,
> because they could end up with a billion streams in that one pool. Thus, the
> guy doing the loop will create a subpool, then call svn_stream_create
> passing the subpool, then clear the subpool and loop. If the create function
> then decides to create a subpool, then we've now got two subpools per
> iteration.
> 
> So... this means that option (1) is almost always the proper style. Defer to
> your caller to let them decide how to manage memory. If they are going to
> call you many times, then they may use subpools. Or they simply give you
> their own.

This is pretty persuasive to me.  Subpools are for managing loops, so
they should be created by the code doing the loop.  Perhaps svn_fs.h
should be revised to match this.

pool usage (was: Re: CVS update: subversion/subversion/libsvn_fs rev-table.c)

[Greg Stein <gs...@lyra.org>]

On Tue, Mar 06, 2001 at 12:42:47PM -0500, Greg Hudson wrote:
> Ben wrote:
> 
> >    Note that svn_stream_close() does not deallocate the memory
> >    associated with the stream; destroy the pool you created the
> >    stream in to do that.
> 
> > So in GregH's last sentence, what does "memory associated with the
> > stream" mean?
> 
> Sorry to be unclear.  I meant the memory allocated for the stream
> structure itself.  I was just trying to say that, per Greg Stein's
> discipline, stream structures are not allocated in a subpool.  No
> memory is allocated by the stream abstraction in the process of
> accessing the stream.

To clarify, I recommend one of two approaches:

1) allocate the object in the provided pool
2) create a subpool, alloc the object in the subpool

Now, that GregH is saying is that (2) shouldn't be used in this case because
the subpool is never used later on. The corollary is that a subpool in this
case would be a fine-grained alloc/free mechanism.

If the allocation is going to be large, then a subpool could be nice. When
somebody calls svn_stream_close() (*if* they do so), then we can get rid of
a big chunk of memory for them. If they never call it, then it'll go away
when they toss their pool.

I believe the hardest question is how the caller should determine their own
behavior. Let's say the caller has an unbounded loop, creating streams. They
cannot simply pass the same pool to each svn_stream_create() function,
because they could end up with a billion streams in that one pool. Thus, the
guy doing the loop will create a subpool, then call svn_stream_create
passing the subpool, then clear the subpool and loop. If the create function
then decides to create a subpool, then we've now got two subpools per
iteration.

So... this means that option (1) is almost always the proper style. Defer to
your caller to let them decide how to manage memory. If they are going to
call you many times, then they may use subpools. Or they simply give you
their own.

An "object" should create a subpool for the things that *it* calls. If my
stream is going to call down into the FS for each read() call, then I'll
probably want to create a subpool at create time, pass it to the FS during
the read, then clear it before returning.

>...
> > (Of course, why would anyone *want* that kind of control?  What use
> > would the stream object be if its hidden baton were destroyed?  Why
> > would you even want the stream object around anymore?)
> 
> It's not a question of control.  It's a question of not creating
> subpools when it's not necessary.  We don't create subpools for
> svn_string_t objects either, not because we want control but because
> it's simpler.

Exactly. We create too many, and as a result, our overall memory policy is
shaky and uneven. Subpools are good for unbounded loops, when you want to
clear the results of each iteration.

> The point of svn_stream_close is not to free memory.  It is to say
> "I'm done {reading/writing} this stream now," which is important for
> things like svndiffs.

Exactly. Comments in the header about "freeing" usually point to a potential
problem. An abstraction should not talk about freeing its memory. The caller
gives it a pool, and that pool defines the lifetime. The object may choose
to use a subpool for internal, unbounded loops, but that is *internal*, so
it wouldn't show up in the API docs.

> Our pool management system means that an
> explicit destruction method is not necessary for every data type we
> have, but for some reason we don't seem to all be on the same page
> about that.

Hmm. Maybe this stuff can go into the HACKING document?

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/