[reSIProcate] memory management and asynchronous activity in TransactionState.cxx

[Scott Godin]

...


On Thu, Nov 14, 2013 at 2:26 PM, Byron Campen <docfaraday at gmail.com> wrote:

>  On 11/14/13 11:15 AM, Scott Godin wrote:
>
> ...some comments inline...
>
>
> On Thu, Nov 14, 2013 at 1:49 PM, Daniel Pocock <daniel at pocock.com.au>wrote:
>
>>
>>
>>
>>
>> I've been looking over TransactionState.cxx with the intention of adding
>> more asynchronous possibilities, for example, allowing MessageDecorator
>> to do some async jobs
>>
>> A few things stand out:
>>
>> - One problem area is memory management, making sure that the SipMessage
>> is not inadvertently deleted too soon (while async in progress) and not
>> left hanging around either.  I notice that the SipMessage
>> (mNextTransmission) is just a regular pointer - is there any reason not
>> to use SharedPtr or auto_ptr for this?
>>
>
>  [Scott]  The original stack developers intentionally kept SharedPtr's
> out of the core stack for performance reasons.  Each SharedPtr access
> requires a mutex lock.  Not sure if the same argument applies to auto_ptr.
>  There was a commit years ago by a company that was contributing to resip
> to convert all SipMessage storage to use SharedPtr's.  This checkin was
> reverted, due to the reasons I listed.
>
>
>      Are we still using that terrible SharedPtr implementation? Newer
> versions of that are _much_ more efficient. As for auto_ptr, it is
> extremely efficient. As for that commit years ago, that was violently
> rejected due to the threadsafety nastiness it invited.
>
>
[Scott] I lifted the SharedPtr implementation from boost.  Are you saying
that boost had a bad implementation that has been improved since?  Do you
support the use of SharedPtr's or auto pointers in the stack then?  I would
defer to you, since you did so much of the stack optimization work in the
past.  : )

   - the last async activity that takes place before decoration is the DNS
>> activity (mDnsResult) so I thought that might provide a useful model for
>> other async activities.  Should this be further generalised, e.g.
>> instead of having a bool mWaitingForDnsResult, keeping some enum that
>> represents the state?  Or is it better to keep extra flags, e.g. bool
>> mWaitingForDecorator?
>>
>
>  [Scott]  Either way is fine with me.
>
>
>> - are there any other asynchronous activities that anybody can envisage
>> adding to this part of the code in future?
>>
>> - to avoid disruption to existing MessageDecorator users, I'm likely to
>> add some virtual method with a default implementation that calls the
>> existing synchronous decorateMessage(), e.g.
>>
>>       virtual bool decorateMessage(SipMessage &msg,
>>                                   const Tuple &source,
>>                                   const Tuple &destination,
>>                                   const Data& sigcompId,
>>                                   Fifo<TransactionMessage>&
>> asyncHandlerFifo)
>>       {
>>          decorateMessage(msg, source, destination, sigcompId);
>>          return true;
>>       }
>>
>> and anybody who wants to do something more lengthy can override that
>> method, start a thread, return false and post the result back on the
>> fifo when ready.  Does this seem like a reasonable way to avoid API
>> disruption?
>>
>
>  [Scott]  it would be better if existing applications didn't have any
> performance penalties introduced as a result of adding async support.
>
>
>      An extra level of vtable indirection isn't going to hurt performance
> here, at least not to any detectable degree. And the compiler _might_ even
> be able to avoid it in this case because it is possible to determine the
> concrete class, I think.
>

[Scott]  I agree if that's what Daniel is actually proposing.   I was
assuming it was going to cause the creation of an async message that would
be driven back into the processing loop and the decorator wouldn't be
executed inline any longer.


>
> Best regards,
> Byron Campen
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20131114/1c0be37e/attachment.htm>