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::strin
Re: SBuf review at r9370
On 03/04/2009 06:09 AM, Kinkie wrote: > 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? > Only "absorb frees" path makes sense, as you pointed out yourself. This is why the proposed method is called absorb :-). > 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 > Sure, but all those are under-the-hood things that cow() should not worry about. > estimateCapacity() which right now statically suggests a 20% increase in sizes > I expect 50% or 100% increase would work better, but this is pure speculation on my part and not a big deal. Let's see what others suggest. > 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 > You may want to add the above plan as a source comment. >> 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__) > Ah, I see now. Not sure why I missed that before, sorry. BTW, __FILE__ should be replaced with the new DebugBaseName() or whatever we called that thing, I guess. One more reason to enhance Throw() so that it can correctly throw custom exceptions. > Hm.. How about > - slice > - shorten > - chop > - range > - cut > Let's use chop() or, if you prefer, zoom(). HTH, Alex.
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
e
online configuration manuals messed up?
Browsing through http://www.squid-cache.org/Versions/v2/2.7/cfgman/ and the 2.6 manual, it seems like the argument types for all config directives are missing... has something changed? -- Mark Nottingham m...@yahoo-inc.com
Re: online configuration manuals messed up?
> Browsing through >http://www.squid-cache.org/Versions/v2/2.7/cfgman/ > and the 2.6 manual, it seems like the argument types for all config > directives are missing... has something changed? > > > -- > Mark Nottingham m...@yahoo-inc.com > I don't think the argument types have ever been in that particular view of the manual. Amos
