Re: StringNG merge?

On Mon, Nov 12, 2012 at 9:21 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 11/03/2012 05:55 AM, Kinkie wrote:
>
>> Feature-branch is at lp:~kinkie/squid/stringng
>
>
> Found one or two more bugs and a few nits:
>
>
>> + int c=(*this)[j];
>> + if (isupper(c))
>> + rv.setAt(j,tolower(c)); //will cow() if needed
> ...
>> + int c=(*this)[j];
>
> Please make "c" const if possible.

done.

>> +#include "OutOfBoundsException.h"
>> +#include "SBufExceptions.h"
>> +#include "base/RefCount.h"
>
> These are out of order per current ordering rules. Please check other
> includes.

Fixed for all files added in this branch.

> I do not think reAlloc() should be inlined -- it is a heavy operation.

De-inlined.

>> +char *
>> +SBuf::rawSpace(size_type minsize)
>> +{
>> + cow(minsize); //passing default npos is handled by cow
>> + ++stats.rawAccess;
>> + return bufEnd();
>> +}
>
> I do not think this matches rawSpace() specification. "minsize" is
> referring to the size of space after content, not the total size of the
> storage buffer. When you call cow(), you should specify the total buffer
> size (length()+minsize?).

Right. Fixed.

>> + // we're not concerned about RefCounts here,
>> + // the store knows the last-used portion. If
>> + // it's available, we're effectively claiming ownership
>> + // of it. If it's not, we need to go away (realloc)
>> + if (store_->canAppend(off_+len_,howmuch)) {
>> + debugs(SBUF_DEBUGSECTION, 7, HERE << "not growing");
>> + return;
>> + }
>
> I think the above SBuf::reserve() may not offer what the developer
> expects it to offer. The above comments are misleading because the code
> does not claim any ownership (effectively or otherwise) -- the callers
> have to do that. The code may lead to the very inefficiencies that
> reserve() is meant to avoid! Here is an example:
>
> s2 = s1;
>
> ... many function calls later, a developer no longer knows that
> s1 and s2 share the same MemBlob B, but they do ...
>
> s1.reserve(64KB); // OK. B will be reAlloced if needed.
> s2.reserve(64KB); // No-op. B already has 32KB space.
> while (...) {
> s1.append(1 Byte); // OK, will use the reserved space.
> s2.append(1 Byte); // Will keep realloc-ing many times!
> }
>
> Can reserve() call cow(length() + howmuch) instead? That way, since the
> caller is reserving SBuf with the intent to change it, we are not
> creating more work, but we do make the reservation optimization work.
> Can you think of a counterexample where cow(length() + howmuch) will
> result in worth performance than the current non-reserving reserve()
> implementation?

It depends, what is the likelyhood of your example to be found in actual code?
Current approach is better fit to a "one writer, many readers" model,
taking a chance not to have any overhead.
Your suggestion adopts taking a certain performance hit in order to
get a predefined behaviour later on.
Either is fine by me. Haven't changed yet, but will if you think it'd be better.

>> + reserve(1);
>> + *(bufEnd())='\0';
>
> I think the above code in c_str() can be simplified as
>
> *rawSpace(1) = '\0';

yes. Changed.

>> +/** obtain prototype store
>> + *
>> + * Just-created SBufs all share to the same MemBlob.
>> + * This call instantiates and returns it.
>> + */
>> +
>> +MemBlob::Pointer
>> +SBuf::GetStorePrototype()
>
> I do not understand why this is called "prototype". It may return a
> "protostore" or "protobuffer", but this is not related to types or
> sketches. I suggest calling this Sbuf::DefaultStore() or
> Sbuf::InitialStore(), but you may find a better name.

What about ProtoStore? If not, InitialStore is good, changed.

>> + if (StorePrototype==0) {
>
> Please use !StorePrototype (preferred) or compare with NULL (if you like
> that a lot more).
>
> Please declare StorePrototype inside SBuf::GetStorePrototype() if
> possible. It should not be used outside that method, right? As a bonus,
> you would not have to document it then.

Ok, done.

>> + _SQUID_INLINE_ static MemBlob::Pointer GetStorePrototype(); //only used by anonymous constructor
>
> That comment is a lie although I am not sure what "anonymous
> constructor" is. The method is used by multiple constructors.

Comment removed.

>> + message = static_cast<char*>(xmalloc(buf.length()+1));
>> + buf.copy(message,buf.length());
>> + message[buf.length()]='\0';
>
> This code can be simplified by writing:
>
> message = xstrdup(buf.c_str());

Er.. I don't think so: c_str() may cow(), which with the current code
doesn't happen.
It may be not really relevant as exceptions should not happen often.
What is more valuable to you? Performance or code brevity?

>> + NullSBufException(const char *aFilename = 0, int aLineNo = -1);
>> + InvalidParamException(const char *aFilename = 0, int aLineNo = -1);
>> + SBufTooBigException(const char *aFilename = 0, int aLineNo = -1);
>
> If possible, please add "excplicit" to all of the above, to prevent
> silent conversion from integer to an Exception.

Done.

> src/ipc/TypedMsgHdr.cc appears to have changes unrelated to this project.

No changes appear appear with a file-based diff.. They were probably
performed in trunk but trunk wasn't merged yet.

> src/mem.cc has whitespace non-changes.

Reverted to match trunk.

> I assume you will drop StrList and friends from this submission so I did
> not continue to review them.

Yes. The only question is whether to leave what was done so far in
(#if 0-ed) or remove it.

> I have not reviewed Tokenizer-related changes yet. I will wait for all
> other issues to settle first. Feel free to exclude Tokenizer from the
> proposed commit scope. It may be best to review Tokenizer when we also
> have some Squid code written or adjusted to use it.

Sure, Tokenizer is not yet used. The same question arises as with
StrList, whether I can leave it in (#if 0-ed) or I should cherrypick
it out.

Thanks,

-- 
    /kinkie