Hi Wen-Jun,
Are you planning for general cleanup or attempting to solve a specific problem?
If it is latter, can you please describe the problem? I might be able to help
you to resolve it possibly differently. This suggestion is based on the
similar fear outlined by Dave's comment earlier.
Below is what I have understood how reference count used in Trafodion.
An object gets a refCount_ only when it is derived from IpcMessageObj. Only
the objects that would be shipped between processes need to be derived from
IpcMessageObj. ComDiagsArea is one such object. You can look at any object
derived from IpcMessageObj to determine how reference count is used.
See the code snippet below
this << *cqReply;
if (!(didAttemptCancel ||cancelByPid))
{
if (diags == NULL)
{
diags = ComDiagsArea::allocate(getHeap());
*diags << DgSqlCode(-EXE_CANCEL_QID_NOT_FOUND);
}
*this << *diags;
}
send(FALSE);
cqReply->decrRefCount();
if (diags)
diags->decrRefCount();
request->decrRefCount();
In the above code, the concatenate operator << increments the reference count.
The “send” queues in the request to be sent. When decrRefCount sees
reference count as 0, it deallocates the IpcMessageObj. "Send" also calls
decrRefCount. Whichever decrRefCount happens last either in "send" or in this
function deletes the IpcMessageObj to avoid memory leak. It is also possible
if you don't manage reference count correctly, you could end up looking at a
deleted object.
Also, in general, in executor layer passing pointers between different object
doesn't involve reference count management because refCount_ member variable is
available only when the object would be shipped between processes.
You can also consult the comments about reference count in
export/IpcMessageObj.h
Selva
-----Original Message-----
From: Qifan Chen <[email protected]>
Sent: Wednesday, August 15, 2018 6:13 AM
To: [email protected]
Subject: Re: refCount on ComDiagsArea
Hi Wen-Jun,
Thanks for the info.
I wonder if you can also provide an example of a ComDiagArea object that is
mistakenly deleted (see the two relevant methods below) due to incorrectly
maintained reference count.
Thanks --Qifan
383 IpcMessageRefCount ComDiagsArea::decrRefCount()
384 {
385 if (getRefCount() == 1)
386 {
387 deAllocate();
388 return 0;
389 }
390
391 // Let base class do the work.
392 return this->IpcMessageObj::decrRefCount();
393 }
1513 inline
1514 void ComDiagsArea::deAllocate()
1515 {
1516 if (collHeapPtr_ == NULL)
1517 delete this;
1518 else {
1519 // save collHeapPtr, because destroyMe() sets it to NULL
1520 // Better solution: derive ComDiagsArea from NABasicObject and get
1521 // rid of allocate() / deAllocate()
1522 CollHeap * p = collHeapPtr_;
1523 destroyMe();
1524 p->deallocateMemory(this);
1525 };
1526 }
1527
________________________________
From: Zhu, Wen-Jun <[email protected]>
Sent: Tuesday, August 14, 2018 9:51:40 PM
To: [email protected]
Subject: 答复: refCount on ComDiagsArea
Hi,
It is true that there are some other functions incrementing the reference
count, like in atp_struct::copyAtp:
if (from->getDiagsArea())
from->getDiagsArea()->incrRefCount();
setDiagsArea(from->getDiagsArea());
incrRefCount() is called to increase the ref count.
But when I check the result of command
grep setDiagsArea * -B4 -A4 -rw
I find lots of code have not invoked incrRefCount(), like
core/sql/exp/exp_eval.cpp:
479 if (retcode == ex_expr::EXPR_ERROR)
480 {
481 if (diagsArea != atp1->getDiagsArea())
482 atp1->setDiagsArea(diagsArea);
483
484 return retcode;
485 }
There's bug here.
So, how about to wrap operator= invocation in atp_struct::setDiagsArea(), and
in operator= we deal with the ref count, like Qifan suggested?
-----邮件原件-----
发件人: Selva Govindarajan <[email protected]>
发送时间: 2018年8月15日 0:23
收件人: [email protected]
主题: RE: refCount on ComDiagsArea
I second Dave's comment. I was about to comment in similar lines. When I saw
Dave's message, I had no second thoughts to second it.
Selva
-----Original Message-----
From: Qifan Chen <[email protected]>
Sent: Tuesday, August 14, 2018 9:11 AM
To: [email protected]
Subject: Re: refCount on ComDiagsArea
Hi,
I personally also like the 2nd option which is to overload the operator= for
ComDiagsArea.
But because of high number of calls to atp_struct::setDiagsArea() in the
executor, it is better to hide the call to operator=(ComDiagsArea&) inside
atp_struct::setDiagsArea().
Also keep in mind that we can not ubiquitously use C11 structs such as
shared_ptr or make_shared in our code base yet since we still need to support
platforms such as CentOS6 that do not have C11 support package.
Thanks --Qifan
________________________________
From: Dave Birdsall <[email protected]>
Sent: Tuesday, August 14, 2018 10:10:43 AM
To: [email protected]
Subject: RE: refCount on ComDiagsArea
Hi,
I have not researched this area, but it strikes me as one that could be very
delicate. It may be that in most code paths it is assumed that some other
function is incrementing the reference count. Great care should be taken in
modifying this otherwise it may lead to memory leaks. I am hoping others who
are more knowledgeable will add to this discussion.
Can you give more insight into what problem led you here?
Dave
-----Original Message-----
From: Zhu, Wen-Jun <[email protected]>
Sent: Tuesday, August 14, 2018 4:11 AM
To: [email protected]
Subject: refCount on ComDiagsArea
hi,
When setting a ComDiagsArea, I find the refCount did not increase, in function
atp_struct::setDiagsArea of file core/sql/exp/ExpAtp.h:
inline void atp_struct::setDiagsArea(ComDiagsArea* diagsArea) {
if (diagsArea_)
diagsArea_->decrRefCount();
diagsArea_ = diagsArea;
}
I guess this is a problem.
There are two solutions to fix this:
1. Invoke incrRefCount for ComDiagsArea to increase, just after the
assignment.
2. Overload the operator= for ComDiagsArea, to increase within the
operator=,
I find operator= is declared in ComDiags.h, but there is no implementation.
The 2nd solution may be better, as the both increment for the left-hand side
ComDiagsArea and the decrement for the right-hand side ComDiagsAre can be
handled within a single operator=, which is friendly to users, like shared_ptr
in C++.
Regards,
Wenjun Zhu
