On Fri, Jan 20, 2012 at 12:42 AM, Greg Stein <gstein_at_gmail.com> wrote:
>
> On Jan 19, 2012 11:02 PM, "Daniel Shahaf" <danielsh_at_elego.de> wrote:
>>
>> Hyrum K Wright wrote on Thu, Jan 19, 2012 at 21:47:51 -0600:
>> > On Thu, Jan 19, 2012 at 8:40 PM, Daniel Shahaf <danielsh_at_elego.de>
>> > wrote:
>> > > I do wonder if the "to disk" threshold should be in the public
>> > > signature, but don't have offhand a use-case justifying that.
>> >
>> > We could, but I figured callers who needed finer-grain control over
>> > the size of the buffer would use the underlying private API which
>> > gives them that ability. And as I noted in the code, the choice of
>> > 100k was completely arbitrary, the result of a lot of squinting and
>> > hand waving. If somebody has better guestimates (Stefan F.?) as to
>> > what would be better, feel free to improve upon it.
>>
>> FWIW, my point was that the caller (of this now-public API) may be able
>> to have a better guesstimate as they'd know the use-case, the number of
>> concurrent spillbuf objects, the typical length (`wc -c`) of the stream,
>> etc.
>
> Right. We always assume "caller knows best", and we try to document our APIs
> to give them the needed info.
>
> I would recommend exposure, with two DEFAULT constants. I'd be -0 with
> making them 0 and documenting their remapping to "suitable defaults". (ie.
> allowing callers to use 0 instead of a 20-char symbol)
But some callers don't care; those that do already have a way of
having finer-grained control: the private API. The blocksize and
maxsize parameters to the spillbuf code are an implementation detail
of a more generic paradigm, so exposing them through the primary API
is inappropriate. It's the "generic buffered stream" abstraction that
callers should be interested in, and letting us improve the
implementation as we see fit. For callers who want or need the
specific spillbuf semantics: use the spillbuf-specific private API.
As implemented, using svn_stream_buffered() is no worse than
unconditionally spooling data to disk (and is in many usecases *much*
better). Callers get a free benefit now, so let's not worry about
giving them the additional control at the expense of additional
complexity until it's shown they need it.
> I'm also a bit concerned with putting these in the public API. I guess our
> prevalence of streams kind of demands this "memory shortcut", but I worry
> about doing this earlier rather than later. Maybe delay one release? (I'm
> confident in the code; unclear on what we want to sign up for long-term)
I was really surprised we didn't already have a stream which
implemented these types of semantics. We can remove it if you want,
but the abstraction is valid, and one whose usefulness is apparent. I
think we should leave the no-parameter generic API in.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Received on 2012-01-20 14:00:20 CET