New Solution for memory leak in tblgen
David Blaikie
dblaikie at gmail.com
Tue Nov 18 22:24:37 PST 2014
On Tue, Nov 18, 2014 at 9:57 PM, wuhui1973 <wuhui1973 at 163.com> wrote:
> Hello David:
>
> Do you have any comment?
>
Did you see my email (on this thread) from yesterday where I mentioned my
commits r222183, r221926? I discuss some context there & I'm not sure
you've read them/have that context.
>
> Regards
> Hui Wu
>
>
> At 2014-11-12 10:22:22, "David Blaikie" <dblaikie at gmail.com> wrote:
>
> OK, so you're saying all the heap allocated TreePatterns and
> TreePatternNodes are currently leaked? And we create some on the stack
> (that obviously don't leak)?
>
> >> [huiwu] Yes, almost all. And quite a few TreePatterns are created on
> stack, which will not leak.
>
> And from previous discussions we've seen that TreePatterns and
> TreePatternNodes form unbreakable(?) cycles. (are they unbreakable - could
> we use weak_ptr or raw pointer in there somewhere to break them safely?
> I'll have to go back & look at the prior threads or you could copy/paste
> the summary of the cycles you saw? - though the only reason this would be
> worth understanding is if we actually destroy things at some point other
> than the end - I don't know anything about tablegen or when nodes are
> created and destroyed, I'm not sure if they're destroyed at any time other
> than "we've finished doing all the work, cleanup now")
>
> >> [huiwu]
> >> TreePatternNode has field PredicateFns , which contains PatFragRec,
> the pointer of TreePattern
>
> >> TreePattern has Trees which is std::vector<TreePatternNode*>
>
>
> >> The cycles exist among PatFragRec & Trees. May like this:
>
> >> TreePatternNode
> <---------------------------------------------------------------+
> >> PredicateFns (std::vector<TreePredicateFn>)
> |
> >> PatFragRec ------> TreePattern
> |
> >> ^ Trees
> (std::vector<TreePatternNode*>) +------------> Other TreePatternNode
> >> |
> >> Other TreePatternNode |
> >> PredicateFns |
> >> PatFragRec -----+
>
>
So the thing that's still unclear to me from this diagram is just where the
/ownership/ happens (or needs to happen, as we add it).
Do all the references to a TreePattern via PredicateFns/PatFragRecs come
from TreePatternNodes that are in that TreePattern?
Do TreePatterns ever need to be kept alive via the
PredicateFns/PatFragRecs, or are TreePatterns themselves always
appropriately kept alive by the CodeGenDAGPatterns? If they are kept alive
that way, then we don't need to deal with cycles in the ownership graph -
we just break it at PatFragRec, leaving PatFragRec unowning (raw pointer,
not a smart pointer) and having CodeGenDAGPatterns::Trees -> TreePattern::Trees
-> TreePatternNode::Children (with children having more children, and so
on). Which should all be unique_ptrs, since it's a tree.
>
> >> In previous solution, every TreePatternNode & TreePattern hold the
> reference count of objects pointed by PatFragRec & Trees. If they find
> the reference count reaches 0,
> >> the associated object will be freed.
> >> At the end of the processing, I found around 700 TreePatternNodes & 100
> TreePatterns (I forget the exact number) remains. Obviously, they form
> cycles which can't be
> >> broken by reference count.
> >> That is why we need FreeCycleRef, in which we can't explicit delete
> those objects, but just assign NULL to PatFragRec & Trees, their owner
> will do the rest for us.
>
> >> But in current design, all heap allocated TreePatternNode &
> TreePattern will be recorded, and there is the mechanism (the operator
> delete) to remove the record if the object is
> >> freed manually somewhere in the source. Then at the end of the process
> (the dtor of CodeGenDAGPatterns), these records are all leaked objects.
> We just go through both
> >> containers to free them.
> >> So as a requirement, TreePatternNode's dtor is not allowed to delete
> pointers within PatFragRec & Children (std::vector<TreePatternNode*>),
> and
> >> TreePattern's dtor is not allowed to delete pointers within Trees. All
> these objects are assumed to be deleted by MemReclaimer::reclaim.
>
>
> If that's all the case, then it seems obvious enough that we should just
> have a big list of all the heap allocated ones, that goes away when we're
> done.
>
> On Tue, Nov 11, 2014 at 6:07 PM, wuhui1973 <wuhui1973 at 163.com> wrote:
>
>> Sorry David:
>>
>> I make a mistake in yesterday explanation.
>>
>> Here is some data I collect during building Tablegen, the data is from
>> x86 target
>>
>> llvm[3]: Building X86.td instruction information with tblgen
>> NumTPNAllocCur: 120754, NumTPNAllocAccu: 121469 <-- it indicates only
>> 121469-120754=715 heap alloc TreePatternNodes get freed during processing
>> (~0.5%)
>> NumTPAllocCur: 9488, NumTPAllocAccu: 9488 <-- in fact *NO*
>> heap alloc TreePatterns get freed during processing
>> NumTPNAllocCur: 0, NumTPNDel: 121469
>> NumTPAllocCur: 0, NumTPDel: 9488
>> TPN dtorNum: 121469, TP dtorNum: 16821 <-- the diff "
>> NumTPAllocAccu"-"TPN dtorNum"=0 indicates that all TreePatternNodes are
>> heap allocated
>> TPN ctorNum: 121469, TP ctorNum: 16821 <-- the diff "TP
>> ctorNum"-"NumTPDel"=7333 indicates that 7333 TreePatterns are placed
>> into stack!
>>
>> llvm[3]: Building X86.td DAG instruction selector implementation with
>> tblgen
>> NumTPNAllocCur: 180382, NumTPNAllocAccu: 197494 <-- 17112
>> TreePatternNodes (~8%) get freed
>> NumTPAllocCur: 9488, NumTPAllocAccu: 9488 <-- *NO* heap
>> alloc TreePatterns get freed during processing
>> NumTPNAllocCur: 0, NumTPNDel: 197494 <-- "NumTPNDel"
>> == "TPN ctorNum": all TreePatternNodes all heap allocated
>> NumTPAllocCur: 0, NumTPDel: 9488
>> TPN dtorNum: 197494, TP dtorNum: 16821 <-- the diff 16821-
>> 9488=7333 indicates that 7333 TreePatterns are placed into stack!
>> TPN ctorNum: 197494, TP ctorNum: 16821
>>
>> So more than 43% TreePatterns are stack residue for both processing.
>>
>> And other target also gives similar data, for instance AMDGPU
>>
>> llvm[3]: Building AMDGPU.td instruction information with tblgen
>> NumTPNAllocCur: 28248, NumTPNAllocAccu: 28348 <-- 100 TreePatternNodes
>> (~0.3%) get freed
>> NumTPAllocCur: 2422, NumTPAllocAccu: 2422 <-- *NO* heap
>> alloc TreePatterns get freed during processing
>> NumTPNAllocCur: 0, NumTPNDel: 28348
>> NumTPAllocCur: 0, NumTPDel: 2422
>> TPN dtorNum: 28348, TP dtorNum: 3792
>> TPN ctorNum: 28348, TP ctorNum: 3792 <-- 1370
>> TreePatterns are inside stack (>36%)
>> llvm[3]: Building AMDGPU.td DAG instruction selector implementation with
>> tblgen
>> NumTPNAllocCur: 44076, NumTPNAllocAccu: 47030 <-- 2954
>> TreePatternNodes (~6%) get freed
>> NumTPAllocCur: 2422, NumTPAllocAccu: 2422 <-- *NO* heap
>> alloc TreePatterns get freed during processing
>> NumTPNAllocCur: 0, NumTPNDel: 47030
>> NumTPAllocCur: 0, NumTPDel: 2422
>> TPN dtorNum: 47030, TP dtorNum: 3792
>> TPN ctorNum: 47030, TP ctorNum: 3792 <-- 1370 TreePatterns
>> are inside stack (>36%)
>>
>> There are several functions in CodeGenDAGPatterns.cpp will allocate
>> TreePatternNodes & TreePatterns from heap.
>> ParsePatternFragments (5 spots allocate TreePatternNodes)
>> ParsePatterns (2 spots allocate TreePatternNodes, 2 spots allocate
>> TreePatterns)
>> CombineChildVariants (1 spot allocate TreePatternNodes)
>> clone (2 spots allocate TreePatternNodes)
>>
>> So I don't think there is any special construction path.
>>
>> Regards
>> Hui Wu
>>
>> 在 2014-11-12 06:33:37，"David Blaikie" <dblaikie at gmail.com> 写道：
>>
>>
>>
>> On Mon, Nov 10, 2014 at 11:36 PM, wuhui1973 <wuhui1973 at 163.com> wrote:
>>
>>> Hope I can explain it clear enough :-)
>>>
>>> > MemReclaimer has two containers to hold pointers of TreePatternNode &
>>> TreePattern which are allocated from heap.
>>> > Only one global instance of MemReclaimer is declared -- memReclaimer.
>>> > operator new & operator delete are defined for TreePatternNode &
>>> TreePattern
>>> new: push the new allocated object (pointer) into
>>> containers in memReclaimer.
>>> delete: remove the pointer from the containers in
>>> memReclaimer.
>>>
>>> So we can guarantee that objects allocated from heap can
>>> always be recorded by memReclaimer.
>>> And no double free may incur, as some places in the source
>>> delete some pointers manually (maybe we can remove these lines).
>>> > So memReclaimer only keeps the objects that are leaked in current
>>> design.
>>> > the author of CodeGenDAGPattern create nodes, but destroy part of
>>> them (for TreePattern, the nodes destroyed manually account for more
>>> than 30%,
>>>
>>
>> Could you provide stack traces/explanation for these case where the
>> TreePatterns are destroyed?
>>
>>
>>> for some target even over 50%; while for TreePatternNode only
>>> around 1%
>>>
>>
>> And for the TreePatternNodes?
>>
>> If they're mostly just on special case construction paths, perhaps we
>> could special case their removal from the list we could add to
>> CodeGenDAGPattern - or, if they're close enough to construction time, we
>> might be able to avoid adding them to such a list in the first place?
>>
>>
>>> ) and make the rest leaked deliberately.
>>> > dtor of CodeGenDAGPattern now is the place to destroy all not freed
>>> objects
>>> > move the functionality of MemReclaimer into CodeGenDAGPattern is
>>> definitely feasible I think.
>>>
>>
>>> Regards
>>> Hui Wu
>>>
>>>
>>> At 2014-11-11 14:20:25, "David Blaikie" <dblaikie at gmail.com> wrote:
>>>
>>> Could you describe the high level design here?
>>>
>>> It looks like there's a static pool
>>> (CodeGenDAGPatterns.cpp::memReclaimer) of instances that's used as some
>>> kind of last-chance cleanup? Instances of TreePatternNode mostly manage
>>> their own lifetime but then if any haven't been destroyed by the time
>>> the CodeGenDAGPatterns dtor runs, the remaining elements are destroyed.
>>> This is to handle cycles, I take it?
>>>
>>> It still seems like a bit of a GC-esque workaround to handle this case
>>> when there might be something better... but I don't know much about
>>> tablegen, perhaps there isn't.
>>>
>>> How often are nodes destroyed by themselves? Should we just give up
>>> owning these anywhere else & move all ownership into CodeGenDAGPatterns and
>>> clean them up in CodeGenDAGPatterns' dtor? (just have a vector of
>>> unique_ptrs (maybe even a list or deque of nodes directly owned, rather
>>> than via unique_ptr), only create nodes, never destroy them, then destroy
>>> them all at the end)
>>>
>>> On Mon, Nov 10, 2014 at 9:58 PM, wuhui1973 <wuhui1973 at 163.com> wrote:
>>>
>>>> Hi Andrew, David and Hal:
>>>>
>>>> I have made a new solution for this memory leak issue, which is much
>>>> simpler than previous one.
>>>>
>>>> I have tested it, it works well!
>>>>
>>>> Please have a look, and appreciate any comment.
>>>>
>>>> Thanks & Regards
>>>>
>>>> Hui Wu
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>>
>
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141118/56943a2c/attachment.html>
More information about the llvm-commits
mailing list