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