Re: Sbuf review at r9331
- check that the function names in debugs() statements match their actual name (there's been quite a lot of shuffling) Please use debugs(..., HERE blah) for that sort of thing at levels 1. I've also just added MYNAME for prettier but identical use on the top two levels. Now done. -- /kinkie
Re: Sbuf review at r9331
Further improvements:
* SBufStore(const SBuf S);
SBufStore must not know about SBuf.
Done. It's now a first-class citizen, without any SBuf knowledge. In
the process I've removed some methods and migrated others to
char*-based.
* Why is Sbuf MemPooled?! We are not supposed to allocate SBuf dynamically,
right? Did you mean to pool SbufStore instead?
You're right. SBufStore and its smaller storage sizes should be pooled.
* If you have introduced size_type, use it. Do not use size_t instead except
for size_type typedef.
Ok. Used it everywhere.
* I doubt clear() should free the storage by default. Usually, we clear to
fill again. You even do that yourself in SBuf::assign(). Deallocating and
allocating would cost more than doing nothing. Let's not deallocate by
default, for now.
I've commented the line releasing the storage. Worst-case, it will
just realloc lazily which is still ok.
* remove dumpStats. Just give access to Stats object(s) that can dump.
Done. Now both SBuf and SBufStore have const getters instead.
* Why is cow() public?
Now private, as discussed.
And check other members for naming style. Use trailing underscores and
mixedCase consistently, everywhere.
Done. I hope I didn't miss anything, will recheck.
* Check that assignment operator sets everything. I think nreallocs was
forgotten.
It was intentional as discussed, but I've now added also that to the
assignment operation.
It's an optimization anyways so its usefulness is debatable until we
get some hard numbers..
* I do not understand why you cannot keep the experted message without
duplicating it in the code below. Can you explain, please?
msg=b.exportCopy();
message=xstrdup(msg); //double-copy to avoid mismatching malloc and new
delete msg;
Fixed. Even though I'll admit I don't like the innards of xstrndup
much so I had to basically reimplement it in client-code (there's a
strlen() too much in xstrndup).
* No need to check for NULL before delete in
if (mem != NULL) {
delete mem; //might have been freed by unref
mem=NULL;
}
Done. Now it only performs a (then-unimplemented) isLiteral check.
You are duplicating the code in the common case.
* Declare when needed, for example inside for():
size_type j;
for (j=0;jlen_;j++) {
Do you mean that this should read
for (size_type j=0;jlen_;++j)
?
* I think you should merge realloc and preAlloc. I believe there are already
calls to realloc() where you should have used preAlloc().
Done. The merged call is now called grow().
* Your append(std::string) reverses the arguments of
std::string::append(string), which many developers would find extremely
confusing, especially since those arguments have defaults. Please fix.
Please double check other standard methods for similar problems.
I have now reduced the number of (constructor/assign/append) functions
by one by grouping the char*-based ones and using default arguments.
The whole thing now looks much more polished and consistent.
Next steps, mempooling SBufStore and its backing storage.
--
/kinkie
Re: Sbuf review at r9331
On 02/23/2009 10:52 AM, Kinkie wrote:
* Declare when needed, for example inside for():
size_type j;
for (j=0;jlen_;j++) {
Do you mean that this should read
for (size_type j=0;jlen_;++j)
?
Yes, I do. IIRC, there are many other examples where variables are
declared too early. This is a minor flaw, with no real effect in most
cases, but it is a good habit to develop and becomes important when you
deal with non-integral variables.
Next steps, mempooling SBufStore and its backing storage.
MemPooling is needed but does not affect the design or most of the
implementation code. There is no serious rush to implement this, IMO.
It looks like all the big items in the previews review are closed. Do
you want me to do another review soon or should I wait for some other
milestone/event?
Thank you,
Alex.
Re: Sbuf review at r9331
On Mon, Feb 23, 2009 at 11:58 PM, Alex Rousskov
rouss...@measurement-factory.com wrote:
On 02/23/2009 10:52 AM, Kinkie wrote:
* Declare when needed, for example inside for():
size_type j;
for (j=0;jlen_;j++) {
Do you mean that this should read
for (size_type j=0;jlen_;++j)
?
Yes, I do. IIRC, there are many other examples where variables are
declared too early. This is a minor flaw, with no real effect in most
cases, but it is a good habit to develop and becomes important when you
deal with non-integral variables.
Next steps, mempooling SBufStore and its backing storage.
MemPooling is needed but does not affect the design or most of the
implementation code. There is no serious rush to implement this, IMO.
Already done in rev 9366. Had to add a second bool flag to MemBlob,
but that's fine.
It looks like all the big items in the previews review are closed. Do
you want me to do another review soon or should I wait for some other
milestone/event?
Any time you want and have the time to spare, I leave it to your judgement.
There's some pending cleanup: the current TODO list on top of my head is:
- rename SBufStore to MemBlob and give it its new home
- rename SBuf to Buffer (I don't remember if we had agreed on a
different name, there were quite a few that flew around) and rename
files accordingly
- rename testSBuf files and classes.
- check that the function names in debugs() statements match their
actual name (there's been quite a lot of shuffling)
- define a few more MemPool String sizes and match them with those in
estimateCapacity
- maybe shuffle function implementations around SBuf.cc(i) to match
their definition order in SBuf.h
Thanks,
--
/kinkie
Re: Sbuf review at r9331
On Mon, Feb 23, 2009 at 11:58 PM, Alex Rousskov
rouss...@measurement-factory.com wrote:
On 02/23/2009 10:52 AM, Kinkie wrote:
* Declare when needed, for example inside for():
 size_type j;
 for (j=0;jlen_;j++) {
Do you mean that this should read
for (size_type j=0;jlen_;++j)
?
Yes, I do. IIRC, there are many other examples where variables are
declared too early. This is a minor flaw, with no real effect in most
cases, but it is a good habit to develop and becomes important when you
deal with non-integral variables.
Next steps, mempooling SBufStore and its backing storage.
MemPooling is needed but does not affect the design or most of the
implementation code. There is no serious rush to implement this, IMO.
Already done in rev 9366. Had to add a second bool flag to MemBlob,
but that's fine.
It looks like all the big items in the previews review are closed. Do
you want me to do another review soon or should I wait for some other
milestone/event?
Any time you want and have the time to spare, I leave it to your
judgement.
There's some pending cleanup: the current TODO list on top of my head is:
- rename SBufStore to MemBlob and give it its new home
- rename SBuf to Buffer (I don't remember if we had agreed on a
different name, there were quite a few that flew around) and rename
files accordingly
- rename testSBuf files and classes.
- check that the function names in debugs() statements match their
actual name (there's been quite a lot of shuffling)
Please use debugs(..., HERE blah) for that sort of thing at levels 1.
I've also just added MYNAME for prettier but identical use on the top two
levels.
They can save a lot of tracing confusion.
- define a few more MemPool String sizes and match them with those in
estimateCapacity
- maybe shuffle function implementations around SBuf.cc(i) to match
their definition order in SBuf.h
Amos
Re: Sbuf review at r9331
On Fri, Feb 20, 2009 at 5:56 PM, Alex Rousskov rouss...@measurement-factory.com wrote: On 02/20/2009 05:16 AM, Kinkie wrote: On Sun, Feb 1, 2009 at 9:08 PM, Alex Rousskov rouss...@measurement-factory.com wrote: Here are a few Buffer comments based on r9331 of lp:~kinkie/squid/stringng Executive summary: Many problems fixed, thank you. We are making progress. Three known big issues remain: 1) SBufStore should be a stand-alone class (see the first two related bullets below). 2) Confusion regarding pooling of SBuf objects is suspected (discussed in a few bullets below). Given your explanation it is now a certainty. On the other hand, SBufStore objects would probably benefit from it, wouldn't they? 3) Lack of detailed low-level review -- waiting for the resolution of the above issues. If you really hate doing changes related to (1) and (2), I can take over once the other fixes are implemented. I am not excited about fixing this, but I would rather do extra work than see you depressed. If this happens, you can still be responsible for testing, integrating, and polishing the result, but you might be happier finding my bugs than listening to my complaints about yours :-) Now this is what I call a positive attitude! ;-) Seriously: the code looks in a much better shape now than it did. Thanks for prodding me. Details below. * Move SbufStore outside of SBuf. You are not buying any extra security or protection by embedding it, but making the code more complex and less reusable. This can be addressed in three ways that I can think of: 1- leaving as-is 2- bringing that out, with private constructors and friendship to select who can instantiate what. 3- bringing that out with big red signs in the class declaration not to (ab)use it. would 2. be acceptable to you? 4. Bring it out as a regular class. There is no good reason to try to hide the store class or try to make its use by others more difficult. Make the class reusable instead of making it ugly and prohibiting its reuse. You are not making SBuf any better by keeping SBufStore inside either. You are making it worse! Just treat SBufStore as a regular class, with a specific purpose -- providing refcountable and growable continues memory storage. You have convinced me. I just ask you for the future, whenever you have objections about something I design, please show me a code mock-up. My resistance to change is almost often motivated by me not seeing a different way to do things. OTOH, I think I have demonstrated a reasonable willingness to change my mind. Whether there will be other users for SBufStore is unknown and unimportant for now. We should probably call the storage class MemBlob to emphasize its purpose and disassociate it from one specific user (SBuf). Sure, no problem. BTW, I am not sure the current set of SBufStore methods is correct. I have a feeling the store is much smarter than it should be, duplicating some of the SBuf functionality. Such problems are typical when two classes are too tightly coupled -- they tend to lose their clear identity and purpose. I will revisit this later, after MemBlob is isolated. It is smarter than needed, mostly for convenience. And yes, they are tightly coupled.. MemBlob is a glorified struct, meant to serve the Buffer... * SBufStore(const SBuf S); SBufStore must not know about SBuf. It knowing about SBuf is a serious design help wrt SBuf::consume() If it didn't, we'd have to take a trip through char*. If it can't be accepted, I'll do so, but if it's possible at all I'd like to keep it. Just like with store issue above, I fundamentally disagree here. The only information stored in SBuf and not in SBufStore is offset and length. You can pass that information to the constructor and other members, as needed. Ok. Whether trips through char* are a bad thing in this context is debatable, BTW. You can pass SBufStore instead of char* but you would still need very similar methods where raw char* is passed. I am fine with two copies of methods, one char*-based and one SBufStore-based, but I would not be surprised if keeping it simple with just char* would be a better solution overall. Simplest way to know is to just try it. It's not something that can't be done in one or two hours probably. * \throw TextException always I've tried to be more specific, all more-specific exceptions inherit from TextException. I'd like to understand what's the benefit in having only one exception kind. I do not think the above bullet was my review comment. I think it was a part of your source code, copy-pasted to provide context for the How about simply not implementing the not-to-be-used methods? comment. Ok. I do not object to specific exceptions. * Why is Sbuf MemPooled?! We are not supposed to allocate SBuf dynamically, right? Did you mean to pool SbufStore instead? SBuf is a fixed-size object. MemPooling avoids heap fragmentation, doesn't
Sbuf review at r9331
Hi Kinkie,
Here are a few Buffer comments based on r9331 of
lp:~kinkie/squid/stringng
The review is incomplete as there is too much in the way to do a full
high-level and low-level review. Some of the comments below will help
to clean up the way for a full review. The common theme is:
- Simplify to the bare minimum. We will optimize and add bells and
whistles later.
- Mimic standard interfaces and double check that you mimic them correctly.
Specific comments:
* Sbuf.h does not use *Exception classes. Most Buffer users would not
use them either. Move them and their prerequisites to
SbufExceptions.{cc,h} or something like that. As you know, I doubt most
of those exception classes are really needed/useful.
* class SBufStats {
friend class SBuf;
protected:
Make members public, remove friendship.
* Remove all *Stats::get() methods. C++ classes have copy constructors.
* The following comment about registration is a lie.
SBufStats(); ///constructor. Also registers with CacheManager
Note that it would probably be a bad idea to register with CacheManager
from the default constructor.
* Either all *Stats classes should have a constructor that initializes
its members or none.
* Move SbufStore outside of SBuf. You are not buying any extra security
or protection by embedding it, but making the code more complex and less
reusable.
* s/memalloc/memAlloc/ s/memblock/memBlock/ etc.
No need to add a third (fourth?) naming scheme. It is bad enough that we
have to mix size_type and canGrow styles.
* SBufStore(const SBuf S);
SBufStore must not know about SBuf.
* SBufStore copy constructor and assignment operator are still not
declared correctly. Please please please review your code for const
correctness, once and for all.
* How about simply not implementing the not-to-be-used methods?
* copy-contstructor. Not to be used.
* \throw TextException always
If there is no implementation, the compiler and not the end-user will
complain if the method is actually used.
* Why is Sbuf MemPooled?! We are not supposed to allocate SBuf
dynamically, right? Did you mean to pool SbufStore instead?
* isNull is still public
* If you have introduced size_type, use it. Do not use size_t instead
except for size_type typedef.
* I doubt clear() should free the storage by default. Usually, we clear
to fill again. You even do that yourself in SBuf::assign(). Deallocating
and allocating would cost more than doing nothing. Let's not deallocate
by default, for now.
* Once again, please remove all exception specifications (I think you
have only empty throw()s left). For rationale, see my earlier reviews or
Item 13 A pragmatic look at exception specifications in Sutter's
Exceptional C++ Style. Most if it should also be available at
http://www.gotw.ca/publications/mill22.htm
* See my earlier StringNg comments regarding truncateSelf, chopSelf,
import*, export*, substrSelf, and headUntil* naming and existence. Trim
them down to basics, use standard names, and remove Self.
* remove dumpStats. Just give access to Stats object(s) that can dump.
* Why is cow() public?
* s/preAlloc/reserve/
* s/memhandle/store_/
And check other members for naming style. Use trailing underscores and
mixedCase consistently, everywhere.
* s/alloc_strategy/calcCapacity/ or similar. The method does not
allocate, change, or return any strategy.
* Initialize Sbuf members in the constructor consistently. For example,
nreallocs is _sometimes_ initialized inside the constructor body. If
there are no performance concerns, initialize before the body and set
later if needed.
* Check that assignment operator sets everything. I think nreallocs was
forgotten.
* Parameter should be a const reference:
SBuf SBuf::append(const SBuf S)
* There are too many similar-but-different append() implementations.
Can't you have one or two and have others call it with the right parameters?
* The following affects operator semantics and, hence, should be illegal
based on our last agreement regarding isNull:
if (isNull() || S.isNull()) {
debugs(SBUF_DEBUGSECTION,9,\tone null);
stats.qget++;
return false;
}
Not to mention that it feels wrong because it is asymmetric.
* Please change your substr() semantics (as implemented) to match that
of std::string when from is too big.
* I do not understand why you cannot keep the experted message without
duplicating it in the code below. Can you explain, please?
msg=b.exportCopy();
message=xstrdup(msg); //double-copy to avoid mismatching malloc and new
delete msg;
* No need to check for NULL before delete in
if (mem != NULL) {
delete mem; //might have been freed by unref
mem=NULL;
}
You are duplicating the code in the common case.
* Declare when needed, for example inside for():
size_type j;
for (j=0;jlen_;j++) {
* I think you should merge realloc and preAlloc. I believe there are
already
