Re: StringNg review: MemBlob

On Wed, Sep 22, 2010 at 5:42 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 09/22/2010 03:30 AM, Kinkie wrote:
>>
>> On Tue, Sep 21, 2010 at 4:18 AM, Alex Rousskov wrote:
>>>
>>> On 09/03/2010 09:21 AM, Kinkie wrote:
>>>
>>>> You'll find the branch at lp:~kinkie/squid/stringng
>
>
>>>> /**
>>>>  * Low-level buffer. It's responsible for allocating and managing
>>>>  * the blocks of memory. It's refcounted, but it doesn't know
>>>>  * which SBufs hold references to it.
>>>>  */
>>>
>>>
>>> Consider:
>>> /// A refcounted fixed-size buffer. Tracks the portion used by opaque
>>> content.
>>
>> How about:
>> "Refcounted memory buffer, content-agnostic. Meant to only be used by
>> SBuf." ?
>
> Fixed-size is an key property unless you think it may change soon.
> Content-agnostic is fine, I guess.

Done.

> As for "meant to only be used by Foo", I am still against such restrictions
> on use in general and in this context in particular. We have discussed this
> before. Use-cases may determine the initial design sketch and optimizations,
> but the final APIs should be usable in any context that needs them rather
> than restricted to one specific user. MemBlob is no exception IMO.

Removed the comment.
I hope to avoid overdesign.

>>>> class MemBlob: public RefCountable {
>>>>     public:
>>>>     typedef RefCount<MemBlob>  Pointer;
>>>>     typedef unsigned int size_type;
>>>
>>> Would be nice to guarantee support for 5GB and larger blobs.
>>
>> Do we need them now? It'll be trivial to extend when needed.
>
> I may need them soon. Based on past experience, I am not convinced that
> replacing 'int' with some other type will be trivial.

I just tried it: changed the typedef in MemBlob and SBuf to int64_t
and adapted SQUIDSBUFPRINT, found one glitch but after fixing it build
and test-suite worked without a hitch.
Until the need arises, I'd prefer to stick to int32_t in order to
avoid overdesign, performance penalties (64-bit math is said to be bad
on many 32-bit platforms) and memory waste.

>>> I would not leave these data members public, but it is your call.
>>
>> This class originally was private to SBuf. I've now clarified in the
>> class definition that it's not meant for general use; its only client
>> should be SBuf.
>
> Which is a wrong restriction made worse by wrongly guessed implications. As
> SBuf review shows, we are already paying the price for the lack of proper
> boundaries between the two classes. It makes some areas of the SBuf code
> confusing at best.

Removed all such implications; I agree that the boundary betweeh
MemBlob and SBuf is blurry in some places; the decisions were not
taken lightly and in my very humble opinion pay off in terms of
performance gains. I hope that the documentation improvements you're
suggesting will clarify the situation. Once the API is finalized, I'll
further document in the Wiki (there is already a design blueprint, but
it is out of date)

>>>>     _SQUID_INLINE_ void deleteSelf();
>>>
>>> I hope this can be removed. If not, it must be documented. The
>>> corresponding
>>> comments in .cci do not make sense to me and do not match RefCount.h in
>>> trunk, as far as I can tell.
>>
>> http://old.squid-cache.org/Doc/Prog-Guide/prog-guide-24.html
>> http://markmail.org/message/kt7357pwttmzrey3
>>
>> Maybe the documentaiton is outdated?
>
> That old documentation is wrong. Use the source code. Remove deleteSelf().

Done.The only references to such construct right now are in BasicUser,
NegotiateUser and NTLMUser (out of scope for me)

>>>>     _SQUID_INLINE_ size_type freeSpace() const;
>>>
>>> Missing documentation.
>>
>> Renamed to getAvailableSpace. Documentation was in the .cci, moved it
>
> Wrong name. The method does not give the caller the available space, only
> its size. Here are some combinations that work better:
>
>   size(), spaceSize(), capacity()
>   contentSize(), spaceSize(), capacity()
>   usedSize() and availableSize(), capacity()

availableSize() it is then :)

>>>> MemBlobStats MemBlob::stats;
>>>
>>> If some static code wants to allocate MemBlob, will its stats not get
>>> counted, depending on static initialization order?
>>
>> Yes. I don't think that's much of an issue;
>
> It becomes an issue when we get bug reports about negative or too-big
> counters. Or when somebody adds an assert() that the counter is correct.
> Both have happened before.

all counters are unsigned. I'm adding documentation telling not to
rely on them. The only way I can think of to disenfranchise from the
static allocation trap is to use a singleton, which would make
bookkeeping a relatively complex operation (much more complex than
incrementing an integer, anyways). Once we put this in context that
I'd remove most counters once SBuf is fully in...

>> stats are mostly meant to
>>
>> help us diagnose the effectiveness of the solution, and in fact I
>> wouldn't be against #ifdef-ing them out in production code.
>
> Sure, nobody cares about effectiveness of the code in production as long as
> it was effective in the lab :-).

lol!
We have invented Schroedinger's effectiveness. We should get a patent
on it; maybe we can apply in time for next Apr 1st..

>>>> /**
>>>>  * constructor, creates an empty MemBlob of size>= requested
>>>>  * \param size the minimum size to be guarranteed from the MemBlob
>>>>  */
>>>> MemBlob::MemBlob(const MemBlob::size_type size)
>>>> {
>>>>     debugs(MEMBLOB_DEBUGSECTION,9,HERE<<  "size="<<  size);
>>>>     memAlloc(size);
>>>> }
>>>
>>> I would recommend initializing mem and sizes to NULL/0s explicitly in all
>>> the constructors.  It does not cost much and may save a few hours of
>>> debugging if things go wrong.
>>
>> Isn't setting them in memAlloc enough? Besides the actual size may be
>> different (or would you mean to set them twice?)
>
> Initialize them to NULLs and 0s before calling memAlloc. Such
> initializations cost little and avoid having an object with uninitialized
> members.

Done.

-- 
    /kinkie