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

[Amos Jeffries]

[ I've re-ordered the discussion points so the important design part we
ended discussion with is now at the top for first consideration.
Depending on that the rest of it may or may not matter. ]



On 9/08/2015 1:14 p.m., Alex Rousskov wrote:
> On 08/08/2015 02:03 AM, Amos Jeffries wrote:
>
>>>> * 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.

... with children inheriting which do all the formatting display parts.


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

The last attempt was 4 years ago. The one before that 6 years prior again.

If you can agree that we start with a Mgr::Formatter class that does not
do anything initially but dump out a free-form "notice" box with a line
of text in it. The other reports output and formatter API/functionality
to be deferred to adding by other patches as need arises. Then I think
I/we can go forward with that.

A "notice" is all the basic actions altered in this patch need (apart
from menu, but that can revert). I will then drop PackableStream in
favour of that class going in now/shortly.
 [yes renaming dump(ostream&) to displayWith(Formatter&) ]


> and I do not understand why (especially since the results of that code
> are parsed by admin scripts making extra changes harmful).
>

Primarily that we can do the formatting cleanup without adding a
circular dependency between libbase and libmem in the current code
situation.

The memory allocator needs to display its part(s) of reports but is
depended on for allocating the objects used to do the dumping. Which if
passed those objects, would in turn depend on linking to them. Loop
problems then ensue between convenience libraries link ordering.


This cycle will exist unless we;
a) inherit the formatter from a totally external type of dumper,
b) make formatter fully an inline or template class, or
c) refactor the deepest memory allocators to split their working code
from their stats code

NP: the current code has already taken the (a) design path and uses
StoreEntryStream passed as ostream& to the memory allocator component.
Thats where PackableStream was taken from, and avoiding touching the
memory allocator yet again was a factor in the choice.


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

I want to see the reports actually looking like sets of tables instead
of some (ie. mgr:dns) being groups of wiggly columns or (store_io) a
jumbled set of differently sized tables when parsed according to the few
bits of published cachemgr syntax. Its quite horrible in some points and
others commenting on it every now and again is usually what keeps it
high-ish on my todo list.


For now I was taking the best approach from the currently existing but
incomplete designs in Squid code and picking the one that appears to be
most suited for completing. StoreEntryStream in current trunk appeared
to be the one that did not limit future display options.



 ... the remainder is specific to the proposed PackableStream or
StoreEntryStream code. Which would be dropped if you agree on the above
approach for how to get a Formatter class transition going.


On 9/08/2015 1:14 p.m., Alex Rousskov wrote:
> 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.
> 

Aha. Thank you. Agreed.

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

>From several long days testing why static_cast always 0-terminated the
strings when trying to fix this same thing in the same way for
StoreEntryStream years back. LP says rev.11605 (initial c++0x support).

Repeating the tests it seems to work now. Although I am building with
GCC-5 now. IIRC that was about GCC 4.6-ish.


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

Looking up the origins of the hack I found the chars being outside the
if() was result of a different patch by one Alex R when adding the if(). :-P

Fixing both PackableStream and StoreEntryStream.


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

So what is the point then?
If it is inline the compiler optimizes it away even in stack traces. No
use for debugging then.


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

Good point. Removing the if().

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

Ah. The hiding is an artifact of grandparenting relationships.

I just compiled and ran it several times with the proposed code. The
result was exactly what I was expecting:

* The caller mgr:menu calls dump(StoreEntry*) which calls the
dump(ostream&) override which displays the expected results.

* The caller mgr:info calls dump(StoreEntry*) which runs the
dump(StoreEntry*) override which displays the expected results.

Clearly dump(StoreEntry*) is not hidden when dump(ostream&) is
overridden. I presume the dump(ostream&) is not hidden in the reverse case.


Amos