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

[Jakob Stoklund Olesen]

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.

> 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.

> Second, specialize Recycler::clear(BumpPtrAllocator) to avoid doing the same thing with the free list.
> 
> If the above works, this might tag along for free?

No, all objects must be handed back to, say, a MallocAllocator. It's only the BumpPtrAllocator that doesn't care.

/jakob

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130102/cdaeb7d4/attachment.html>