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

[Scott Godin <sgodin@xxxxxxxxxxxxxxx>]

...

On Thu, Nov 14, 2013 at 2:26 PM, Byron Campen <docfaraday@xxxxxxxxx> 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@xxxxxxxxxxxxx> 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