Solution for memory leak in tblgen

Sun Sep 21 20:03:24 PDT 2014

Hello David, Andrew:

I spent this weekend in designing the solution using shared_ptr, I have changed tons of code, but always found some object had been released prematurely.

Seem I had mistakenly made two shared_ptr holding the same object, but I got lost in the huge code change.

The biggest difference between the self defined "shared_ptr" and std::shared_ptr is: for former the reference counter intrudes into the Tree Node, while for latter

the reference counter is at different place. So for the self defined "shared_ptr", it is safe to use the pointer to Tree Node directly in most time, except in case of 

pointer self-assignment, for example, in method SimplifyTree. But using std::shared_ptr, it needs take care of all pointer assignments. Seems only using shared_ptr

in place of all Tree Node pointer is safe, but it will result in huge code change and a not-easy implementation. Especially in some method, "this" pointer will be passed as 

parameter to other function or even as parameter of recursion, which is difficult to handle and error-prone if using std::shared_ptr.

That is why I insist on using self-defined "shared_ptr".

As for reclaiming cyclic referred objects, I have below explanation:

+ Here we can't just delete the nodes inside the container. Consider this cycle:
+  //         A          
+  //       7|  \       if we directly delete node A, then node C will be freed as
+  //      /     \      no reference now, next B. But at time B returning its memory,
+  //     /      _\|    it finds the pointer to node A and try to free it as B is the
+  //    B <------ C    last referrer, that comes the double delete!
+  //                   So the correct way is to break the reference link, for example,
+  // set the pointer to A in B to null which is reference counted, then B will free A
+  // as it is the last referrer. In turn C and B will be freed but not double delete.
+  //

I have tidied my code, please have a look.

Thanks & Regards

Hui Wu

在 2014-09-19 00:04:00，"David Blaikie" <dblaikie at gmail.com> 写道：

On Sun, Sep 7, 2014 at 4:48 PM, Andrew Trick <atrick at apple.com> wrote:

On Sep 2, 2014, at 8:08 PM, wuhui1973 <wuhui1973 at 163.com> wrote:

Hi Andrew:

Please find the anwser on line.

At 2014-09-03 10:54:21, "Andrew Trick" <atrick at apple.com> wrote:

On Sep 2, 2014, at 7:22 PM, wuhui1973 <wuhui1973 at 163.com> wrote:

Hello Andrew, Hal:

I have updated my solution. In these days, I have tried BumpPtrAlloc, but the biggest problem of it is BumpPtrAlloc won't invoke dtor before releasing the memory.

What does the dtor do beside release memory?

[huiwu] it will invoke dtor of its member. For example, in TreePattern object, its dtor will invoke dtor of following members:

Trees (type std::vector<TreePatternNode*>)
NamedNodes (type StringMap<SmallVector<TreePatternNode*,1> >)
Args (type std::vector<std::string>)

So if dtor if TreePattern is not invoked, only just memory occupied by this TreePattern instance is freeed, memory allocated on heap by all these containers leak!

I see how it might be hard to allocate all the referenced data using BumpPtrAllocator. That’s unfortunate, because it would be a good way to design this sort of thing.

Given that you will continue using normal heap allocation, there is still something I don’t understand about the design. It looks like your adding references to all the nodes to a global container. So if there is one point in the code where no TreePatterns should be alive, such as the point where you would call ReclaimCycle, or destroy the BumpPtrAllocator, why don’t you just explicitly delete all the TreePatterns referenced from your container at that point? There would be no need to reference counting or cycle detection.

Yeah - I'm confused by the complexity and have the same question.

-Andy

And I also consider std::shared_ptr, but not use it for following findings:

1> Nodes of TreePatternNode & TreePattern form cycles in reference, I have designed a simple method to recycle them, which can't be used with shared_ptr.
2> After adding the grammar sugar operator-> & operator T(), now most source files using TreePatternHolder & TreePatternNodeHolder now needn't be changed. We can do that because we check reference count on every reference counted objects in every related methods. So any abuse can be found earily as possible.

Thanks.

 In attachment, there are two kinds of suffix:

*.review is for review
*.fix is for submit

You can run clang-format before your patch, submit that as a separate patch (no need for a separate review), then submit your patch on top of it.

-Andy

The reason why need these files is because I had run clang-format on the related source files which results in too many unrelated differences

At 2014-08-07 11:59:44, "Andrew Trick" <atrick at apple.com> wrote:

On Aug 7, 2014, at 2:29 AM, wuhui1973 <wuhui1973 at 163.com> wrote:

>> [hui] I am new to tablgen, I hope I can explain it clearly. At some stage in building llvm, tablgen will compile .td file into .inc file (C++ source), it will compile .td file one after one till the stage is done. As now the tree nodes are just allocated but not freed, they get lost between handling .td files. So the patch aims to reclaim the memory when finishing one .td file. I think pool allocator can be an option, but as I know, pool allocator is just another sub-project under llvm, has it been built into llvm? And it needs be done by the DSA (data structure analysis) alogrithm?! Is there any example in llvm?

Sorry, I wasn’t clear what I was talking about. In LLVM, we have a class called BumpPtrAllocator. You can use that to allocate a set of nodes, then free them all between passes. If that accomplishes our goals for TableGen, then I think it’s the ideal solution.

-Andy

<DAGISelMatcherGen.cpp.review><TableGen.cpp.fix><DAGISelMatcherGen.cpp.fix><CodeGenDAGPatterns.cpp.fix><CodeGenDAGPatterns.h.fix><CodeGenDAGPatterns.h.review><CodeGenDAGPatterns.cpp.review>

_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140922/a990bd9a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: TableGen.cpp.patch
Type: application/octet-stream
Size: 7299 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140922/a990bd9a/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CodeGenDAGPatterns.h.patch
Type: application/octet-stream
Size: 41892 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140922/a990bd9a/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DAGISelMatcherGen.cpp.patch
Type: application/octet-stream
Size: 42570 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140922/a990bd9a/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CodeGenDAGPatterns.cpp.patch
Type: application/octet-stream
Size: 152056 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140922/a990bd9a/attachment-0003.obj>