> On 2 Mar 2018, at 18:13, Christophe Fergeau <cferg...@redhat.com> wrote:
> On Fri, Mar 02, 2018 at 02:00:23PM +0100, Christophe de Dinechin wrote:
>>> On 2 Mar 2018, at 09:03, Frediano Ziglio <fzig...@redhat.com> wrote:
>>>>> On 1 Mar 2018, at 13:13, Frediano Ziglio <fzig...@redhat.com> wrote:
>>>>>> From: Christophe de Dinechin <dinec...@redhat.com>
>>>>>> Throwing 'runtime_error' directly should be reserved for the support
>>>>>> library.  Add an 'Error' class as a base class for all errors thrown
>>>>>> by the streaming agent, as well as subclasses used to discriminate
>>>>>> between categories of error.
>>>>>> The Error class offers:
>>>>>> - a 'format_message' member function that formats a message without
>>>>>> allocating memory, so that there is no risk of throwing another
>>>>>> exception at throw time.
>>>>> why not formatting the message constructing the object so before the 
>>>>> throw?
>>>> That requires dynamic allocation, which is entirely unnecessary and
>>>> potentially dangerous (replacing one exception kind with another).
>>> As explained by my string.c_str example your interface is prone to errors.
>> The interface is not “prone to errors” at all. The scenario you
>> describe makes no sense, since the whole interface is designed to
>> avoid dynamic strings. This is now explicitlyi documented (see
>> https://github.com/c3d/spice-streaming-agent/blob/c%2B%2B-refactoring/include/spice-streaming-agent/errors.hpp,
>> not yet shared, I’m not entirely done with the reviews). The
>> constructor comment now says:
>>      Constructor takes the base message returned by what()
>>      The message must remain valid over the whole lifetime of the exception.
>>      It is recommended to only use static C strings, since formatting of any
>>      dynamic argument is done by 'format_message' 
> This behaviour seems inconsistent with std::runtime_error(const char *)
> which as far as I can tell does not have this lifetime expectation.

You are correct, but please note that std::runtime_error takes a string as 
input argument, making it very clear that you are dealing with a ‘string’ 
underneath, not a const char*.

>> Anyway, your example is misleading. Anybody that passes the result of
>> string::c_str() to a const char * without proper consideration for the
>> lifetime is at fault. The const char * interface itself cannot be
>> faulted for that.
> For what it's worth, I usually expect const char * interfaces to make
> their own copy of the const char * arg if they need it after they
> return, and I've rarely seen exceptions to this expectation.

std::vector and all standard library containers? You do a push_back of a const 
char * in an std::vector<string>, you get a copy. If you do that in an 
std::vector<const char *>, you don’t.

In C++, raw pointers, including const char *, always have the explicit meaning 
that you manage the lifetime. So if you have “rarely seen exceptions”, you did 
not look hard enough IMHO ;-)

Let me give an example from *our own code*. ConcreteConfigureOption takes two 
const char * arguments. Does it make copies? No. Same thing with the related 
AddOption method. Anything particularly surprising there? I don’t think so.

Of course, we could switch to “strings everywhere” (which Lukas suggested at a 
point). But that’s wrong too, it causes unnecessary allocations and code bloat 
(see below what I’m talking about).

> If I understood properly, things are designed this way to avoid running out 
> of memory while building the exception. I'm definitely not in favour of 
> having an error-prone API to handle a very rare case which the rest of the 
> code is most likely not going to be able to cope with anyway.

Yet another iteration of the “bad_alloc” theory. You know that this is starting 
to get on my nerves? As stated *REPEATEDLY* before, bad_alloc is only one of 
the reasons for doing things that way.

To make this clear, let me elaborate a little on *one* other reason (I listed a 
few others), code bloat and runtime cost. Let’s say we want to store a write 
error with an errno. The two approaches under consideration are:

        A. throw WriteError(message, operation, errno);
        B. throw std::runtime_error(message + “ in “ + operation + “: “ + 

I’ll briefly mention, as a gentle reminder of some of the “other reasons”, that 
(A) is
1. shorter to type
2. easier toi read
3. less error prone
4. more likely to guarantee consistent messages

and I will instead focus on the code being generated in both cases.

In case A, the code is roughly equivalent to:

        tmp = WriteError_constructor(message, operation, errno)

So roughy two calls, passing a total of four register args, no exception 
landing pad.

In case B, the code is roughly equivalent to (where LPn is a “landing pad”, a 
piece of code that cleans up if an exception is thrown):

        tmp1 = string_constructor(message);             // LP1
        tmp2 = string_constructor(“ in “);                      // LP2
        tmp3 = string_operator_plus(tmp1, tmp2);        // LP3
        tmp4 = string_constructor(operation);           // LP4
        tmp5 = string_operator_plus(tmp3, tmp4);        // LP5
        tmp6 = strerror(errno);
        tmp7 = string_constructor(tmp6);                        // LP6
        tmp8 = string_operator_plus(tmp5, tmp7);        // LP7
        tmp9 = runtime_error_constructor(tmp8); // LP8

LP8:    string_delete(tmp8);    // Chain to next landing pad
LP7: string_delete(tmp7);
LP6: string_delete(tmp6);
LP5: string_delete(tmp5);
LP4: string_delete(tmp4);
LP3: string_delete(tmp3);
LP2: string_delete(tmp2);
LP1: string_delete(tmp1);
        runtime_rethrow();      // (__UnwindResume) to propagate bad_alloc

I omitted a few details for clarity, like the exception handling tables, etc. 
But the gist of it is that now *at each throw point* you have something like 
25+ calls being generated (some are inlined), passing a total of at least 25 
register args…

Well, 10+ times worse? That’s bad enough. But it gets better.

Let’s consider runtime cost now. What do these calls do?

In case A, three word-size copies in the constructor of WriteError, and I think 
that’s about it.

In case B, each of the strings allocates memory and copies the string in it. 
Let’s be optimistic and say that it’s one call per allocation, and that you get 
it from a free list (let’s say something like 5 loads and three stores, that’s 
rather optimistic). So now we have another 7 calls, and well over 50 load/store 
instructions just for the memory allocation itself. And then the base message 
is copied at least four times. Cache, cache, cache, buy me more cache!

In short, using B looks innocuous, but we replaced, roughly
        A. two calls, four register args, three memory copies
with, at runtime,
        B. At least 25 calls, at least 25 register args, and four memory copies 
of the original message.

So here too, at least an order of magnitude worse, probably closer to two.

Guess what. After writing all this, I realized I had forgotten one 
concatenation for “: “, so that all my numbers are understimated… Oh well…

Meta note: How long do you think it took me to write this reply, in a probably 
futile attempt to explain that there is more to it than bad_alloc? I’m sorry, 
but this is really grinding. Now, that is not an entire waste of time if you 
take the time to really understand what I explained, and if I get the benefit 
that you never mention bad_alloc ever again ;-)

Otherwise, if you only see that as some kind of “justification”, my time is 
wasted, so is yours, and everybody walks out of it unhappy.


> Christophe

Spice-devel mailing list

Reply via email to