[squid-dev] [PATCH] SBuf API improvements

[Kinkie]

On Wed, Dec 30, 2015 at 1:01 AM, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> On 12/29/2015 02:59 PM, Kinkie wrote:
>> On Mon, Dec 28, 2015 at 10:27 PM, Alex Rousskov wrote:
>>> On 12/28/2015 06:41 AM, Kinkie wrote:
>>>> It is debateable whether reserveCapacity should be just renamed
>>>> instead of aliased; feedback is welcome.
>
>>> I do not have a strong opinion on this. reserveCapacity() is not called
>>> reserve() today because we also have reserveSpace() and knowing that
>>> there are two different methods is important for some callers. Once you
>>> add plain reserve(), the probability that a caller will notice
>>> reserveSpace() that the caller needs will go down.
>
>> We could leave both and mark in the documentation that it's meant for
>> API compatibility and should not be used by code calling SBuf. what do
>> you think?
>
> Having three methods (reserve, reserveCapacity, and reserveSpace) is a
> poor solution to the problem that the two methods (reserveCapacity and
> reserveSpace) were solving. Yes, adding a not-to-be-used third name
> might be better than renaming the existing one, but, again, the overall
> direction of your changes eliminates good options, forcing you to pick
> among the bad ones.

Ok, next option. reserveSpace(n) is actually just
reserveCapacity(n+length()); what about simply getting rid of
reserveSpace and rename reserveCapacity to reserve? Minimum surprise
here for users here.

>>>> The objective of this change will be clear in the followup rfc3986
>>>> patch, which is templatized in order to be available both to helpers
>>>> who use std::string and to squid using SBuf.
>
>>> SBuf is already suffering from being too many things to too many
>>> different callers. I suspect that requiring API compatibility with
>>> std::string [in certain poorly defined areas] will make the situation
>>> worse, but I cannot prove that. Same for writing templated code that can
>>> work with both std::string and SBuf.
>>>
>>> If helpers really need SBuf, give them SBuf. If helpers want to use
>>> std::string, they already can. Assuring that one can be substituted with
>>> another feels like a false goal that would bring more trouble in the
>>> long run than it would help in the short term. Again, I cannot prove
>>> that and will not veto these changes.
>
>
>> helpers do not need SBuf, in fact they don't want it, they need
>> percent-encoding and a c++ string management system.
>
> I doubt that all helpers are the same with regard to string needs. Many
> helpers do not need C++ at all. I assume that you are thinking about
> helpers maintained by the Squid Project and helpers that we decided to
> make very efficient and tightly integrated with Squid code.

Yes. Although I wouldn't call them "tightly integrated" but simply
"bundled". What I'm trying to do is to make them more readable, secure
and adaptable first by using c++ in place of the c-with-thin-c++-paint
they currently are coded as. Making them more maintainable, hopefully
robust and readable is the main goal. In fact, as we can assume that
they will be used as reference by others writing their own helpers,
it's in everyone's best interests if they are as readable and compact
as possible.

>> The main benefit of SBuf over std::string is its integration with
>> support infrastructure in squid proper (mempools and cachemgr), which
>> is definitely too much to add to helpers as well. We'd end up filling
>> them with stubs which would make maintaining the build system
>> complicated - see how much effort it is to do so for unit tests.
>
> I certainly agree that it is easier to add a few methods to SBuf and
> craft a templated encoding loop than it is to make SBufs independent
> from memory pools [and cache manager]. The ease of a small change does
> not necessarily make it the best long-term solution though. There are
> other factors to consider.
>
> You use existing unit test build difficulties as an argument to avoid
> using SBufs in helpers [because helpers will face similar difficulties].
> That argument is valid _if_ those build difficulties are inherent in the
> SBuf concept. However, if those difficulties are simply SBuf
> implementation bugs/deficiencies, then it would be better to address
> those SBuf deficiencies than to work around [and exacerbate!] them in
> one more place.

Let's consider mempools for instance. Having a build-time, static,
mechanism which can prevent linking against mempools is IMO tricky and
fragile; and mempools carry with them a lot of extra depedencies (e.g.
squid_curtime - it's simple but pervasive).
Currently the minimum set for linking against SBuf outside Squid is:
base/CharacterSet.{h,cc}
base/InstanceId.h
MemBlob.{h,cc}
OutOfBoundsException.h
SBuf.{h,cc}
SBufExceptions.{h,cc}
tests/stub_debug.cc
tests/stub_libmem.cc
tests/stub_SBufDetailedStats.cc
base/libbase.la


>> So IMO helpers need std::string,
>
> and Perl $string, and Javascript String. There are many different
> helpers with very different needs...

right. Let me restrict the scope to "the C-ish and C++ helpers we bundle"

>> and templated code is the best
>> solution I could come up with to avoid having to maintain three
>> different implementations of percent-encoding. Suggestions on
>> alternate paths to obtain the same objective are of course welcome.
>
> Since Squid is (or should be) using standard encodings, I suspect there
> are libraries and modules (for various programming languages) that
> already provide appropriate encoding support. We would not have to
> maintain any of those implementations.

There are. However:
- the ones I could find seem to be quite generic, we have different
needs in different contexts
- impedence mismatch. These are either C or use std::string; this
generally means two more data-copies if we wish to use SBuf (or
std::string with a custom allocator)
- finding one which is generally available on all OSes we care about
seems not trivial.
- if we don't find one, IIRC bundling was already discarded as an
option when talking about relying on boost.


> However, let's assume that our ultimate goal[1] is to ship fast,
> production-quality helpers that are built without external dependencies
> (i.e., built using Squid libraries) and that Squid code is (or will be)
> using SBuf for most string and buffering operations.
>
> Quality helpers are likely to need a variety of string and buffering
> operations. Which of the following approaches is better long-term?
>
> A. Support a growing number of templated algorithms while making SBuf
> API more and more interchangeable with std::string (while at the same
> time providing a growing number of SBuf methods for raw buffer operations).
>
> B. Make SBufs and SBuf-driven algorithms easily available in helpers by
> making SBuf storage/memory backing (and any other current dependencies
> on Squid core) easily replaceable with something that does not need any
> integration with Squid core.
>
> To me, option B looks like the right overall direction.

Could be, but we'd then need to redesign mempools to be stub-able, or
templatize SBuf with an allocator and isolate it from mempools (I
focus on these as these are the dependency bringing in most baggage.

Given the fact that we won't probably require many common algorithms,
I believe that A is not a bad an option as you do.


-- 
    Francesco