<div dir="ltr"><div class="gmail_default" style>Sorry for the delay, and thanks for correcting my understanding of your expected use case... I have a couple more questions below:</div><div class="gmail_default" style><br></div>
<div class="gmail_default" style>On Wed, Jan 2, 2013 at 1:54 PM, Jakob Stoklund Olesen <span dir="ltr"><<a href="mailto:jolesen@apple.com" target="_blank" class="cremed">jolesen@apple.com</a>></span> wrote:<br></div>
<div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div class="im"><div>On Jan 2, 2013, at 1:01 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank" class="cremed">chandlerc@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt"><div dir="ltr"><div>On Wed, Jan 2, 2013 at 10:38 AM, Jakob Stoklund Olesen <span dir="ltr"><<a href="mailto:jolesen@apple.com" target="_blank" class="cremed">jolesen@apple.com</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi,<br>
<br>
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.<br>
<br>
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.<br>
</blockquote><div><br></div><div>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.</div>
</div></div></div></div></blockquote><div><br></div></div><div>The purpose of the function is to deliberately break the contract specified by ilist_node_traits:</div><div><br></div><div><div>template<typename NodeTy></div>
<div>struct ilist_node_traits {</div><div> static NodeTy *createNode(const NodeTy &V) { return new NodeTy(V); }</div><div> static void deleteNode(NodeTy *V) { delete V; }</div><div><br></div><div> void addNodeToList(NodeTy *) {}</div>
<div> void removeNodeFromList(NodeTy *) {}</div><div> void transferNodesFromList(ilist_node_traits & /*SrcTraits*/,</div><div> ilist_iterator<NodeTy> /*first*/,</div><div> ilist_iterator<NodeTy> /*last*/) {}</div>
<div>};</div><div><br></div><div>I specifically don't want to call removeNodeFromList() and deleteNode(), even if they're nontrivial.</div><div><br></div><div>The destructor is not part of the contract, it is always hidden behind the traits deleteNode() function.</div>
</div></div></div></blockquote><div><br></div><div style>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...</div>
<div style><br></div><div style>Anyways... That said, I was still misunderstanding your use case, thanks for the clarification below:</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><div><br></div></div><div class="im"><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><span style="font-size:10pt">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.</span></div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"></blockquote>
<div><br></div><div>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?</div><div><br></div><div>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.</div>
<div><br></div><div>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.</div></div></div></div></div></blockquote>
<div><br></div></div><div>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:</div>
<div><br></div><div><div>void ilist_traits<MachineInstr>::deleteNode(MachineInstr* MI) {</div><div> assert(!MI->getParent() && "MI is still in a block!");</div><div> Parent->getParent()->DeleteMachineInstr(MI);</div>
<div>}</div><div><br></div></div><div>This is desirable behavior, except immediately before deallocating the entire MachineFunction.</div></div></div></blockquote><div><br></div><div style>Ahh, ok, I was going down the wrong path here. Thanks for that.</div>
<div style><br></div><div style>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?</div>
<div style><br></div><div style>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.</div><div style>
<br></div><div style><br></div><div style>Anyways, don't get held up here, we can iterate on the exact design as needed in post-commit review.</div></div></div></div>