Re: SBuf review at r9370

>>>> * Remove isImported. Copy and then free the buffer when importing
>>>> instead. Same motivation as in the isLiteral comment above.
>>>>
>>> This too has a IMO a very practical use: it allows us an easy path to
>>> get into the SBuf world i/o buffer which have been created by the i/o
>>> layer. If you're sure, I'll remove it.
>>>
>> I understand that it can be a useful optimization. I am sure we should
>> replace that optimization with copying (in absorb()) for now because we
>> already have enough bugs to worry about without this special case.
>> Special cases like this significantly increase the probability of a bug
>> left unnoticed by a reviewer, IMO.
>
> Who'd be in charge of managing the passed memory block?
> I see two choices:
> - the caller is in charge
> then absorb becomes an alias of assign() and has no reason to exist
> except create confusion
> - absorb frees
> this would make the behaviour more consistent with the eventual
> impementation, and a mismanaged pointer will bomb immediately.
>
> Do you have a path you'd prefer to see?

Path #2.

In A.absorb(B) .... A takes full control over the buffer of B.
Leaves B with NULL MemBlob pointer. Then A releases it's new temporary ptr
when finished doing the absorption. If MemBlob ref-counting does free or
not is irrelevant to both A and B, they no longer care or matter.

>>>> * cow() logic feels wrong because grow() does not just grow(). Grow is
>>>> discussed below. The cow() code should be something very simple like:
>> This approach is both clean and efficient.
>>
>> The copy constructor can optimize, but please do keep it simple.
>>> My suggestion: reintroduce reAlloc() which gets called by cow() and
>>> does the low-level work, and have both cow() and grow() perform
>>> checks, heuristics and call reAlloc(). How would you like this?
>>>
>> I believe the Copy approach above is superior as far as cow() is
>> concerned. I will have to revisit grow() separately.
>
>
> Done. Some resizing may still occur, because:
> 1- we're not copying the whole MemBlock, just the portion that interests
> us
> 2- rounding to memory boundaries due to (eventually) MemPools
>
>
>>>> * estimateCapacity() is too complex to understand its performance
>>>> effects, especially since nreallocs is a very strange animal. I
>>>> suggest
>>>> that we keep it very simple and add optimizations later, if needed.
>>>> Something like
>>>>
>>>> Â Â granularity = desired < PageSize ? 16 : PageSize;
>>>> Â Â return roundto(desired, granularity)
>>>>
>>>> is much easier to comprehend, analyze, and optimize. Let's postpone
>>>> complex optimizations.
>>>>
>>> I agree, with a SLIGHTLY more complex design to match the current
>>> standard MemPool String sizes.
>>> Stuff like:
>>> desired *= constant_growth_factor;
>>> if (desired< ShortStringsSize)
>>> Â return ShortStringsSize;
>>> if (desired < MediumStringsSize)
>>> Â return ShortStringsSize;
>>> if (desired < BigStringsSize)
>>> Â return BigStringsSize;
>>> if (desired < 2kBuffersSize)
>>> Â return BigStringsSize;
>>>
>> Can you add some static(?) SuggestMemSize() method to the String pool
>> and call it from SBuf? We should not duplicate the logic of
>> string-related memory size calculations, IMO.
>
> Drastically simplified.
> New approach:
> grow() calls {
> estimateCapacity() which right now statically suggests a 20% increase in
> sizes
> and then reAlloc which calls {
> MemBlob::memAlloc which calls {
> MemBlob::memAllocationPolicy which {
> adopts a stepping factor to avoid fragmentation
> (128-byte chunks up to 1kb, then 512-byte chunks up to 4kb,
> then round to nearest 4kb
> }
> and allocates using xmalloc
> }
> }
> }
> MemPools integration will see memAllocationPolicy be replaced by a
> call to Mem::memAllocateString which will choose the right pool and
> feed the new size back
>
>>> return roundto(desired,PageSize);
>>>
>>> We may want to discuss whether squidSystemPageSize should be static to
>>> this module (as it is now) or belongs to the global namespace.
>>>
>> Does any other Squid code use it? If yes, it would be nice to
>> store/calculate it in a single, shared location.
>
> Done. Now in Mem.h/mem.cc
>
>>>> * startsWith() is buggy -- it ignores the contents of its parameter's
>>>> buffer.
>>>>
>>>
>>> Does it?
>>>
>> It does. Â s/*this/S/.
>
> Fixed and implemented unit tests.
>
>>>> * SBuf::consume() returns a copy of SBuf. It should return a reference
>>>> to this or void.
>>>>
>>> Er.. no.
>>> Consume shortens the SBuf by chopping off the head and returns the
>>> chopped part.
>>>
>> Ah, I see, sorry. I would change the implementation to
>>
>> Â Â const SBuf rv(substr(0, actual));
>> Â Â ... // chop the beginning from "this" buffer as you do now
>> Â Â return rv;
>
> Yes. Done.
>
>> then.
>>>> * Exceptions thrown by SBuf will lack source code location info,
>>>> right?
>>>> I think we need to add a ThrowHere(ex) macro that supplies that to the
>>>> ex parameter.
>>>>
>>>
>>> No, their constructor passes it through.
>>>
>> But who supplies the file and line values to the constructor if we just
>> call throw?
>
> The same way TextException does:
> throw someException(__FILE__,__LINE__)
>
>>>> * Please rename substrSelf() as discussed.
>>>>
>>> I don't recall reaching a decision about the target name.
>>> We can decide to drop it entirely (make it private), but the last
>>> agreement I recall is that since it's not part of the std::string API,
>>> we're on our own in defining it, and we haven't really defined it.
>>>
>> I do not remember the two(?) replacement names that were proposed, but
>> they are all better than substrSelf. Give me a list if you want me to
>> pick one.
>
> Hm.. How about
> - slice
> - shorten
> - chop
> - range
> - cut
>
>>> * s/roundto/RoundTo/
>>>
>>>
>>> Now in libmiscutil. Should it be capitalized anyways?
>>>
>> If it is global or static, it should be capitalized (per Squid style).
>
> Done.
>
>
>
> --
> /kinkie
>
Received on Wed Mar 04 2009 - 23:18:12 MST