[MOAB-dev] Discussion on return type of MOAB routines

[Tim Tautges (ANL)]

[Moving discussion to moab-dev, so others outside ANL can participate...]

The problem here is sometimes a non-success condition is expected, and other times it isn't.  Therefore, printing the 
error in the lowest-level function isn't the right thing to do.  And in those same cases, when it is in fact an error, 
there may be extra information that can only come from the lower-level function (e.g. in tag_get_handle, when the tag 
name corresponds to an already-allocated tag but with different properties, this would allow you to pass which 
properties were in conflict).

On 2) below, a global error is insufficient to store error information, because we want to support multi-threaded calls 
in the future.

Finally, as I've said before, the size of these objects being returned will be small except in non-success conditions, 
and most expected non-success conditions we know of right now are in non-performance-sensitive parts of code (getting 
tag handles).  So the biggest question is what's the normal size of std::string and std::ostrstring, and are those 
"large" compared to normal stack information?

- tim

On 01/20/2014 02:14 PM, Vijay S. Mahadevan wrote:
> +1. This was my suggestion. No need to store the error message
> anywhere. If the user wants to add some message at any of the levels,
> it will be printed along-side what gets thrown from the inbuilt call
> anyway (Line number, Function name, File name, etc)
>
> Simplifies design and no need to return objects. It does not break any
> existing user-code either nor does it need to change existing method
> signatures.
>
> Vijay
>
> On Mon, Jan 20, 2014 at 2:05 PM, Wu, Danqing <wuda at mcs.anl.gov> wrote:
>> My point is, I do not see why we need an ErrorInfo class to replace ErrorCode. With enum type ErrorCode, the stack trace can still be supported (as PETSc). So what is the benefit (that we can eve ignore the function return overhead)
>>
>> 1) If we want to support a feature to build error messages with C++ streaming (that is why ostringstream object was proposed in the design), the operation does not have to be embedded in ErrorInfo class.
>> It can be wrapped like this:
>> #define SET_ERR_USE_STREAM(errCode, errMsgStream) { std::ostringstream _ostream; \
>> _ostream << errMsgStream; \
>> return MBError(__LINE__, __func__, __FILE__, _ostream.str(), MB_ERROR_TYPE_NEW); }
>>
>> 2) If we want to store error messages inside ErrorInfo class
>> a) We are not going to store a stack of all level of error messages, otherwise returning this class has more overhead
>> b) If we use std::ostringstream or std::string as one member, it is actually only the last error message being set. In this case, a global variable that holds the last error message (like existing one that is inside Core) is sufficient, if user would like to extract it.
>> c) For both a) and b), all level of messages are already printed out in the stack trace for the user. No need to store the error messages somewhere. This should be the same reason for PETSc, which does not store error messages at all.
>>
>> Unless there are some other features that justify the necessary using of ErrorInfo class, I would still prefer to use ErrorCode.
>> ________________________________________
>> From: Tautges, Timothy J.
>> Sent: Monday, January 20, 2014 1:37 PM
>> To: Wu, Danqing; Vijay S. Mahadevan
>> Cc: fathom at lists.mcs.anl.gov
>> Subject: Re: Discussion on return type of MOAB routines
>>
>> I think we're placing too much emphasis here on cost of instantiating ErrorInfo.  I think it's a fair question, and the
>> usual problem is to assume it's too cheap to instantiate objects, but I also think this case is an exception.
>>
>> Before we make any decisions on this, we definitely need benchmarking.  I don't think instantiating a std::string is
>> going to be any cheaper than doing same with ostrstring, since the latter will just add functions and small data
>> members.  If it is much different, then I suspect the << stream function could be built around an existing std::string.
>>
>> - tim
>>
>> On 01/17/2014 01:28 PM, Wu, Danqing wrote:
>>> Now I am thinking of one question:
>>> Do we really need to replace ErrorCode enum type with ErrorInfo class as the return type for MOAB? For PETSc, it still returns enum type PetscErrorCode and seems to work well. The support of stack trace does not require this return type change.
>>>
>>> Cons:
>>> 1) Function return has overhead with copy constructors called. Vijay first raised concerns on the stringstream member of ErrorInfo class. He would like to replace it with std::string. He also suggests using const string for further optimization. The overhead will be reduced, but still exists. After all, return an enum (integer) always has least overhead.
>>> 2) A lot of existing MOAB routines have to be updated with new signatures. iMesh or some other 3rd party projects that consume MOAB routines could be affected
>>>
>>> Pros:
>>> 1) For building up error messages with C++ stream, a class can define << operator
>>> However, this be extracted out of the class, as Vijay suggested, with something like
>>> #define SET_ERR_STREAM(errCode, errMsgStream) { std::ostringstream _ostream; \
>>> _ostream << errMsgStream; MBError(__LINE__, __func__, __FILE__, _ostream.str(), MB_ERROR_TYPE_NEW); }
>>>
>>> 2) When A calls B, A can get the error message from the ErrorInfo object returned
>>> A()
>>> {
>>>       ErrorInfo err_info = B();
>>>       // err_info.get_msg()
>>> }
>>> Not sure if this feature is of much use:
>>> First, the error message is only the last message that was set by B, or by other routines in the call hierarchy
>>> Note, error message is initially set when something fails, and a higher level function in the call stack might change it (Vijay, I think this is different from your understanding)
>>> Second, all levels of error messages are already printed out in the stack trace (maybe this is why PETSc does not store error messages like our design)
>>>
>>> Please suggest whether we should use ErrorInfo as the return type anyway, or we keep existing ErrorCode as PETSc does.
>>>
>>> Best
>>> Danqing
>>> ________________________________________
>>> From: Vijay S. Mahadevan [vijay.m at gmail.com]
>>> Sent: Thursday, January 16, 2014 4:08 PM
>>> To: Wu, Danqing
>>> Cc: fathom at lists.mcs.anl.gov; Tautges, Timothy J.
>>> Subject: Re: Discussion on return type of MOAB routines
>>>
>>>> 1) What is purpose to make mErrorMsg as const? You mentioned that it could
>>>> make the return copy optimized by the compiler?
>>>> The issue is that is it constant, so it can only be initialized by the
>>>> constructor once.
>>>> One requirement is, if we want to change the error string in the caller
>>>> after checking returned error, we might need something like
>>>> err_info = MOABRoutine(...)
>>>> CHK_ERR1(err_info, "string to pass back if err_info is not success")
>>>> However, as mErrorMsg is const, we can only create a new ErrorInfo object
>>>> for this case, instead of setting it on the existing err_info object.
>>>
>>> If the error message is a const string, it lets the compiler optimize
>>> away places where the string is explicitly null. If I understand
>>> right, your mErrorMsg member only stores the actual message when
>>> something fails. Once this is set, a higher level function in the
>>> stack is not going to change it anymore. Right ? This lets const
>>> string mErrorMsg to not have any allocated bytes explicitly when you
>>> are not failing. If you do fail, you can always call
>>> SETERR(MB_FAILURE, "my custom error message)
>>>
>>>> 2) If ostringstream is used by the user and passed by reference to the
>>>> ErrorInfo class to get the string, we even do not need the second
>>>> constructor, the first constructor is enough:
>>>>       ostringstream err_str;
>>>>       err_str << "n = " << n << ", created a contrived MB_NOT_IMPLEMENTED";
>>>>       ErrorInfo err_info(MB_NOT_IMPLEMENTED, err_str.str());
>>>>       SET_ERR(err_info);
>>>
>>>
>>> This is why I created an overload. If you are going to be passing a
>>> stream directly, your macro wrapper should call
>>>
>>> SET_ERR_STR(MB_NOT_IMPLEMENTED, "n = " << n << ", created a contrived
>>> MB_NOT_IMPLEMENTED") which internally gets propagated as a creation of
>>> ErrorInfo object as
>>>
>>> #define SET_ERR_STR(errCode,errMsgStr) { std::stringstream _ostr;
>>> _ostr << errMsgStr; return ErrorInfo(errCode,errMsgStr); }
>>>
>>> where the constructor: ErrorInfo(ErrorCode err_code,
>>> std::ostringstream& err_str) will be called.
>>>
>>> I'd rather just print out the relevant data in SET_ERR directly than
>>> creating this ErrorInfo object at all since it is a thin wrapper
>>> around an ErrorCode and a message (if the user sets one). But that's
>>> your call.
>>>
>>> Vijay
>>>
>>>
>>> On Thu, Jan 16, 2014 at 3:14 PM, Wu, Danqing <wuda at mcs.anl.gov> wrote:
>>>> Vijay raised a good concern on the new return type of MOAB routines. Now it
>>>> is ErrorInfo class type instead of ErrorCode enum type, with ErrorCode and
>>>> stringstream members. We might need some bench mark test to see if this will
>>>> cause issues when the call hierarchy involve many functions.
>>>>
>>>> The goal is to make the return type as lightweight as possible so that
>>>> copying is relatively efficient, especially for function return. Copy of
>>>> stringstream object inside ErrorInfo might cause some overhead, depending on
>>>> the implementation of stringstream class.
>>>>
>>>> The reason that stringstream is a member of ErrorInfo class is for the
>>>> requirement "define IO stream operators such that we can use C++ streaming
>>>> IO calls to build up error messages".
>>>>
>>>> Vijay has one suggestion to move stringstream out of ErrorInfo class, and
>>>> use std::string instead. It will be something like this:
>>>>
>>>> class ErrorInfo {
>>>>     ErrorCode mErrorCode;
>>>>     const std::string mErrorMsg;
>>>>
>>>> public:
>>>>     ErrorInfo(ErrorCode err_code = MB_FAILURE, const char* err_msg = "") :
>>>> mErrorCode(err_code), mErrorMsg(err_msg)
>>>>     {
>>>>     }
>>>>
>>>>     ErrorInfo(ErrorCode err_code, std::ostringstream& err_str) :
>>>> mErrorCode(err_code), mErrorMsg(err_str.str())
>>>>     {
>>>>     }
>>>>
>>>>      ..
>>>> }
>>>>
>>>> So when building error message with stream, it is out of ErrorInfo class,
>>>> and pass the stream by reference to the constructor of ErrorInfo
>>>>       ostringstream err_str;
>>>>       err_str << "n = " << n << ", created a contrived MB_NOT_IMPLEMENTED.";
>>>>       ErrorInfo err_info(MB_NOT_IMPLEMENTED, err_str);
>>>>       SET_ERR(err_info);
>>>>
>>>> Vijay, I have two questions so far regarding your proposed change:
>>>> 1) What is purpose to make mErrorMsg as const? You mentioned that it could
>>>> make the return copy optimized by the compiler?
>>>> The issue is that is it constant, so it can only be initialized by the
>>>> constructor once.
>>>> One requirement is, if we want to change the error string in the caller
>>>> after checking returned error, we might need something like
>>>> err_info = MOABRoutine(...)
>>>> CHK_ERR1(err_info, "string to pass back if err_info is not success")
>>>> However, as mErrorMsg is const, we can only create a new ErrorInfo object
>>>> for this case, instead of setting it on the existing err_info object.
>>>>
>>>> 2) If ostringstream is used by the user and passed by reference to the
>>>> ErrorInfo class to get the string, we even do not need the second
>>>> constructor, the first constructor is enough:
>>>>       ostringstream err_str;
>>>>       err_str << "n = " << n << ", created a contrived MB_NOT_IMPLEMENTED";
>>>>       ErrorInfo err_info(MB_NOT_IMPLEMENTED, err_str.str());
>>>>       SET_ERR(err_info);
>>>>
>>>> Best
>>>> Danqing
>>>
>>
>> --
>> ================================================================
>> "You will keep in perfect peace him whose mind is
>>     steadfast, because he trusts in you."               Isaiah 26:3
>>
>>                Tim Tautges            Argonne National Laboratory
>>            (tautges at mcs.anl.gov)      (telecommuting from UW-Madison)
>>    phone (gvoice): (608) 354-1459      1500 Engineering Dr.
>>               fax: (608) 263-4499      Madison, WI 53706
>>

-- 
================================================================
"You will keep in perfect peace him whose mind is
   steadfast, because he trusts in you."               Isaiah 26:3

              Tim Tautges            Argonne National Laboratory
          (tautges at mcs.anl.gov)      (telecommuting from UW-Madison)
  phone (gvoice): (608) 354-1459      1500 Engineering Dr.
             fax: (608) 263-4499      Madison, WI 53706