Re: [reSIProcate] memory management and asynchronous activity in TransactionState.cxx
On 14/11/13 21:03, Byron Campen wrote:
> On 11/14/13 11:47 AM, Scott Godin wrote:
>> ...
>>
>>
>> On Thu, Nov 14, 2013 at 2:26 PM, Byron Campen <docfaraday@xxxxxxxxx
>> <mailto: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 <mailto: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. : )
>>
> Yeah, the boost sharedptrs have gotten waaaaaay better; they just
> use low-level lock-free atomic stuff now (I assume the c++11 shared_ptr
> stuff is basically identical). If we want to make more use of these, we
> may want to consider updating our copy, use it in place of c++11
> shared_ptr when that isn't available (with the same name), and maybe
> provide the old SharedPtr alias to keep source compat for downstream.
> I'm not quite sure what this would mean for ABI compat though. However,
> so much of resip assumes strong ownership that I don't think that
> sharing anything across, say, the stack/TU boundary is going to be a
> good idea. auto_ptr I'm totally fine with.
>
Just one note about the memory management for my specific case here: I
don't imagine that the decorator should be calling delete or anything
like that itself. My only concern was making sure that no other part of
the stack can delete the message at the wrong time. Using SharedPtr may
provide more certainty that this change won't have other side effects
but it is not essential.