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