[squid-dev] [PATCH] PackableStream for cachemgr reports

[Amos Jeffries]

On 8/08/2015 5:04 p.m., Alex Rousskov wrote:
> On 08/05/2015 10:24 AM, Amos Jeffries wrote:
>> Adds a PackableStream class which provides std::ostream semantics for
>> writing data to a cachemgr report.
> 
> 
>> +    PackableStreamBuf(Packable *out) : outBuf(out) {}
> 
> Missing "explicit" or a comment justifying implicit conversions.
> 
> 
>> +    ~PackableStreamBuf() {}
> 
> Please add "virtual".
> 

Fixed both. Darn cut-n-paste.

Also fixing the StoreEntryStream.h where these originated.

> 
>> +    virtual int_type overflow(int_type aChar = traits_type::eof()) {
> 
>> +    virtual int sync() {
> 
>> +    virtual std::streamsize xsputn(const char * chars, std::streamsize number) {
> 
> Please add "override" to each overridden virtual method from the
> std::streambuf API.

I dont quite follow you here.

Is that a request to add comments?
 or an attribute I was not aware of?

> 
>> +            // NP: cast because GCC promotes int_type to 32-bit type
>> +            //     std::basic_streambuf<char>::int_type {aka int}
>> +            //     despite the definition with 8-bit type value.
>> +            char chars[1] = {char(aChar)};
> 
> Please use static_cast (or another appropriately named cast) to cast.
> 
> Do not declare "chars" when you do not need them.

The comment says 'cast' because that is essentially what is being done.

However the char(int_type) type constructor is necessary because this is
the fundamental int_type being passed in. All the available
*_cast<char>() operators are built to convert a 32-bit int_type value
into an array of 4 bytes before dealing with the byte->char conversion
part. Which then incorrectly takes the lowest-8 bits rather than the
first-8.

The int_type being passed in actually has only 8-bits of length, not
32-bits. Which screws over those cast operators memory accesses. They
typically end up casting the value at 2nd or 4th byte of a 1-byte stack
object to char.


If you are aware of a foo_cast<>() operator that correctly converts in
the presence of that traits weirdness I am interested.


> 
> Make "chars" constant if you can.

Trying.

> 
>> +                outBuf->append(chars, 1);
> ...
>> +            outBuf->append(pbase(), pending);
> ...
>> +            outBuf->append(chars, number);
> 
> To reduce code duplication and to assist with debugging, it would be
> nice to move these three into a single dedicated private write() method
> or similar.
> 

Calling xsputn() instead now. Although note that trampolining off a
virtual function (non-inlinable in some compilers) actually makes it
less efficient.


> 
>> +        if (pending)
>> +            outBuf->append(pbase(), pending);
> ...
>> +        if (number)
>> +            outBuf->append(chars, number);
> 
> Are you sure you are saving more cycles by bypassing zero-size appends
> than you are wasting on the size check? If you are not, do not waste
> cycles and ink on that check.

The MemBuf and SBuf implementations of append() check for 0 themselves.
But the StoreEntry assumed non-0 and does some relatively heavy
allocation. PackableStream(StoreEntry) is expected to be the common use
case so this is a net performance gain until StoreEntry itself gets
optimized.


> 
>> +private:
>> +    Packable *outBuf;
>> +};
> 
> Why is this a pointer? The class code seems to require a non-null
> pointer. If you can make this a reference, please do.
> 
> If this member remains a pointer, we ought to check that it is not null.

It was pointer in StoreEntyStream. Trying reference.

> 
>> +    PackableStream(Packable *entry): std::ostream(0), theBuffer(entry) {
> 
> Missing "explicit" or a comment justifying implicit conversions.

Fixed.

> 
> If you do not want this class to be responsible for checking "entry" for
> being nil (after the PackableStreamBuf fixes above), then use a
> reference instead of a pointer.
> 
> Please s/entry/packable/ or something like that. We should not imply
> that this is StoreEntry even if it [usually] is today.

Nod. Using p like everywhere else.

> 
> 
>> +    /* create a stream for writing text etc into theEntry */
> 
> No theEntry here.

Fixed.

>> +    virtual void dump(std::ostream &) {}
> 
> This should either throw (if it must not be called unless overridden) or
> provide some debugging (if it is OK to leave it a no-op).
> 

It is okay to be a no-op. It should not be called if not overridden.
Making it dump "X is NOT Implemented" to the stream for now.


> 
>> +    virtual void dump(std::ostream &) {}
>> +
>> +    // magic wrapper.
>> +    // old code will override this instead of the new ostream dumper.
>> +    virtual void dump(StoreEntry *e);
> 
> IIRC, it is not possible to override just one dump() without hiding the
> other. I do not know whether we enable compiler warnings about hiding
> virtual methods by default, but we _should_ enable them if we do not
> already.

I thought both symbols had a vtable entry under their different mangling.

Running some extra tests to check that now.


> 
> Perhaps more importantly, why do we actually need this [black] magic
> here? Can the new method just use a different name, like dumpToStream(),
> until the transition is over?

The only black magic bit is if both virtuals get hidden when one is
overridden. And that is anti-useful. So if it happens I will definitely
rename.

Otherwise it is six of one, half a dozen of the other.

> 
>> * convert old Action classes to new API
>> * refactor old C-code report generators to be Action classes
>>  - fixing display output syntax to minimal YAML as we go on both the
>> above. It mostly is already, but some reports have wrong syntax.
> 
> It feels wrong that you want Actions to output using strict syntax but
> give them an "free form" or "anything goes" std::ostream as a tool to
> accomplish that. Should not Action dumping methods receive a specialized
> class that makes it easy to produce the output you want and hard (if not
> impossible) to make syntax mistakes?
> 

Action itself is currently that class you speak about AFAICT. Maybe in
future the "update Action API to receive client desired format" will be
a formatter class rather than just header value.

For now its just a free-form mix in plain text. To which a stream is
suited. And possible something the formatter classes could use better
than a direct Packable object pointer/reference.

Given the 4 years it has taken to get this far I'm expecting the stream
and cut-down YAML will be used for a long time before we get around to
debating better formatter syntax(es) again.


> 
> Finally, "make check" fails in my tests with several errors like:
> 
> 
>> tests/stub_libmgr.cc:65:6: error: prototype for ‘void Mgr::RotateAction::dump(StoreEntry*)’ does not match any in class ‘Mgr::RotateAction’
>>  void Mgr::RotateAction::dump(StoreEntry *entry) STUB
>>       ^
>> In file included from tests/stub_libmgr.cc:48:0:
>> ../src/mgr/BasicActions.h:78:18: error: candidate is: virtual void Mgr::RotateAction::dump(std::ostream&)
>>      virtual void dump(std::ostream &);
>>                   ^
> 

Fixing.

Followup patch to come when the virtual experiments and re-tests are
completed.

Amos