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?

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