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

[Alex Rousskov]

On 08/08/2015 02:03 AM, Amos Jeffries wrote:
> On 8/08/2015 5:04 p.m., Alex Rousskov wrote:
>> On 08/05/2015 10:24 AM, Amos Jeffries wrote:
>>> +    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?

It is a C++11 specifier that allows the compiler to check that you are
indeed overriding a parent method and tells the reader that you are
overriding a parent method:

  http://en.cppreference.com/w/cpp/language/override

It should be mandatory in new trunk code IMO.


>>> +            // 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.


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

Yes, I know, hence the request.


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

I am curious where you got the above information from, especially the
last sentence.

Here is the sequence of conversions for std::basic_streambuf<char>
AFAICT (where "c" is of type "char" and "int_type" is "int"):

1. sputc(c) or a similar method is called to write our character c.

2. sputc() calls traits_type::to_int_type(c) to convert c to int.

3. traits_type::to_int_type() returns static_cast<int_type>(c).

4. sputc() calls your overflow() with the int result of #3:
   this->overflow(traits_type::to_int_type(c));

In summary, there is only one conversion, and that conversion is
static_cast<int>(c).

If we cannot reverse that using static_cast<char> in your overflow(),
then there exist such a "char" c for which

   static_cast<char>(static_cast<int>(c)) != c

I cannot find such a character c.

Moreover, AFAICT, the existence of such a character would contradict the
standard requirement that integer values from the [minimum character,
maximum character] range are preserved during conversions from integer
to character:

> When a value with integer type is converted to another integer type
> other than Bool, if the value can be represented by the new type, it
> is unchanged.

There may be something else in this picture that I am missing, of
course, but it seems rather straightforward to me as long as we stay in
the std::basic_streambuf<char> context (which you and your code seem to
assume as well).


> The int_type being passed in actually has only 8-bits of length, not
> 32-bits.

AFAIK, the "int_type" being passes is just "int". I believe you have
said that yourself.


Also, do not declare "chars" until you need them. This is unrelated to
casting. It is related to where you declare "chars". You are declaring
them too early, inserting them in the wider context where they are not
needed AFAICT.


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

I do not see anything weird going on and expect static_cast<char>(int)
to reverse the effects of static_cast<int>(char).


>>> +                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.


This is exactly the reason I did not suggest to call xsputn(). Please do
not call that virtual method! Please add and call a dedicated private
write() method (or similar).


>>> +        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.

If zero-length appends are very rare, you would be wasting cycles
instead of saving them, despite StoreEntry "heavy" implementation.

Moreover, if StoreEntry does something important when handling
zero-length calls (e.g., calling invokeHandlers), then blocking that
code from running might actually introduce a well-hidden bug.


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


It is not. Using the same method name wasted yours and mine time
already. It will waste more time in the future when somebody wants to
search for one of the methods and not the other. It will make method
names in error message more difficult to interpret. It will make method
names in debugs() output more difficult to interpret. It is "black"
magic because few people know about this and many people are confused by
how it actually works.

This article explains name hiding I was thinking about (see the first
example; the second is different):
http://www.programmerinterview.com/index.php/c-cplusplus/c-name-hiding/

However, I was wrong that it is an optional GCC warning. It is actually
an error. Still, there are plenty of other reasons to avoid same-name
virtual methods in this context.


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

Mgr::Action is a cache manager command [processor]. Action knows where
to get the information we need to write. Formatting obtained information
is a very different matter. Yes, we could add formatting methods to
Action itself, but I am not sure that is a good idea.


> Maybe in
> future the "update Action API to receive client desired format" will be
> a formatter class rather than just header value.

Perhaps, but what is the value of replacing dump(StoreEntry) with
dump(ostream) now, when what we actually want is dump(void) or
dump(Formatter)? It feels like you want to rewrite the same code twice,
and I do not understand why (especially since the results of that code
are parsed by admin scripts making extra changes harmful).


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

The code for free-form formatting already exists. We do not want it in
the future. Why replace one free-form with a slightly different
free-form when what we need is YAML-form?


Thank you,

Alex.