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