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