Re: StringNg review: MemBlob

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.

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.

>>> 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 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.

>>> _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().

>>> _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()

>>> 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.

> 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 :-).

>>> /**
>>> * 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.

Thank you,

Alex.
Received on Wed Sep 22 2010 - 15:42:16 MDT