[llvm-commits] [PATCH] Add an iplist::clearAndLeakNodesUnsafely() function, speed up Recyler:clear()

[Chandler Carruth]

Sorry for the delay, and thanks for correcting my understanding of your
expected use case... I have a couple more questions below:

On Wed, Jan 2, 2013 at 1:54 PM, Jakob Stoklund Olesen <jolesen at apple.com>wrote:

>
> On Jan 2, 2013, at 1:01 PM, Chandler Carruth <chandlerc at google.com> wrote:
>
> On Wed, Jan 2, 2013 at 10:38 AM, Jakob Stoklund Olesen <jolesen at apple.com>wrote:
>
>> Hi,
>>
>> I am trying to speed up the ~MachineFunction destructor. We are currently
>> wasting a lot of time and memory bandwidth by pointlessly visiting and
>> changing all the instructions in the function before the BumpPtrAllocator
>> gives its pages back to the OS.
>>
>> I am eventually going to make all MachineInstr memory be allocated from
>> the same BumpPtrAllocator so the whole MachineFunction data structure can
>> be taken down by flushing the allocator. This is similar to Apache's memory
>> pools.
>>
>
> This makes total sense to me with one exception: I think we need to
> predicate it on the node's destructor being trivial. Otherwise we're
> violating the design contract of the class, even if we correctly deallocate
> the memory backing the objects.
>
>
> The purpose of the function is to deliberately break the contract
> specified by ilist_node_traits:
>
> template<typename NodeTy>
> struct ilist_node_traits {
>   static NodeTy *createNode(const NodeTy &V) { return new NodeTy(V); }
>   static void deleteNode(NodeTy *V) { delete V; }
>
>   void addNodeToList(NodeTy *) {}
>   void removeNodeFromList(NodeTy *) {}
>   void transferNodesFromList(ilist_node_traits &    /*SrcTraits*/,
>                              ilist_iterator<NodeTy> /*first*/,
>                              ilist_iterator<NodeTy> /*last*/) {}
> };
>
> I specifically don't want to call removeNodeFromList() and deleteNode(),
> even if they're nontrivial.
>
> The destructor is not part of the contract, it is always hidden behind the
> traits deleteNode() function.
>

Yes, but not running the destructor breaks my typical expectation of the
C++ language's contract, so if we don't call deleteNode, and have it call
the destructor, this may trip over tools like UBSan that try to check these
types of language contracts..... But on talking to Richard, this is (to me
terrifyingly) not the case. C++ *doesn't* require you to call destructors
for objects when you release the storage some other way, as long as the
program doesn't depend on the destructors running, which clearly it doesn't
here...

Anyways... That said, I was still misunderstanding your use case, thanks
for the clarification below:


> As a first step, I need to convince an iplist to let go of its contents
> without pulling every object into cache (and dirtying all the cache lines
> as a bonus!). The clearAndLeakNodesUnsafely() function added in the first
> patch does that.
>
>>
> I feel like I would like this more if it were more generically applied.
> What do you think about making this a compile time policy decision for the
> ilist?
>
> I'm imagining a bool template parameter which essentially says
> "LeakSubobjects" and which is a compile error if used in conjunction with a
> value type with non-trivial destructor.
>
> Then, clear(), erase(), replace(), and other methods on ilist which
> currently spend time walking the ilist and deallocating can all benefit
> from the fast-path.
>
>
> That won't work. Look at how we define ilist_node_traits for
> MachineInstrs. The addNodeToList() and removeNodeFromList() hooks do useful
> work when updating the Parent pointer. Similarly, deleteNode() puts the
> object back in the Recycler for MachineInstrs:
>
> void ilist_traits<MachineInstr>::deleteNode(MachineInstr* MI) {
>   assert(!MI->getParent() && "MI is still in a block!");
>   Parent->getParent()->DeleteMachineInstr(MI);
> }
>
> This is desirable behavior, except immediately before deallocating the
> entire MachineFunction.
>

Ahh, ok, I was going down the wrong path here. Thanks for that.

It sounds like you really want to make the destructor if the ilist itself
be the no-op? Is there ever a case where you want the no-op clear that you
aren't about to destroy the ilist?

If I've understood that correctly, I still think making this a template
policy is the right way to go, and check that policy in the destructor
before clearing the ilist.


Anyways, don't get held up here, we can iterate on the exact design as
needed in post-commit review.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130104/a9f86660/attachment.html>