On Sun, Jul 15, 2012 at 11:43 AM, Jakob Stoklund Olesen <span dir="ltr"><<a href="mailto:stoklund@2pi.dk" target="_blank" class="cremed">stoklund@2pi.dk</a>></span> wrote:<br><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 class="im"><br>
On Jul 15, 2012, at 5:00 AM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" class="cremed">chandlerc@gmail.com</a>> wrote:<br>
<br>
> These two callsites to addRange showed up as fairly hot, taking about 10% of some executions with many basic blocks and variables.<br>
<br>
</div>You mean the asan test case, right?</blockquote><div><br></div><div>Yes, but only after I've switched it to produce more "normal" looking functions. They now look essentially the same as metaprogramming-unrolled loops coming out of uses of Boost and other C++ libraries.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> It is important to pay attention to the performance of typical compilations which look very different.<br>
</blockquote><div><br></div><div>Very aware. I'm working on a bit of infrastructure here that will let me quickly test a patch out against our entire codebase and see if either the average time to compile or the shape of the distribution of compile times changes. I'm happy to hold back from too many more patches until that is done. We're planning on also running it nightly to track changes over time.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">> Exposing the merge interface and using it to batch each loop makes them vanish from the profile, dropping compilation time on one address sanitizer test case by 10%.<br>
<br>
</div>+ /// \brief Merge the specified array of LiveRanges into this interval.<br>
+ ///<br>
+ /// This merges with existing ranges as appropriate. Clients should use<br>
+ /// this interface if potentially many ranges are going to be merged in<br>
+ /// sequence rather than calling addRange repeatedly.<br>
+ void mergeRanges(ArrayRef<LiveRange> Ranges);<br>
<br>
The problem is, this interface exposes the innards of the LiveInterval representation and the merge algorithm. It doesn't fit the callers well, forcing them to copy their data structures just to fit the API. (And then the merge makes another copy).<br>
</blockquote><div><br></div><div>My expectation is that in the small case, the lack of allocation makes these copies not an issue, and in the large case, the actual merge is the dominant issue. The profiles I've seen thus far seem to back that up. I also don't think a batching interface is a huge imposition on the callers. It added two lines of code in both cases?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">It would be much better to wrap the merge algorithm state in a class with an add(start, end, valno) function.<br>
<br>
That would also help solve the second problem: The ranges to merge must be sorted, but the callers only have almost-sorted lists. SlotIndex numbering follows layout which is not the same as MBB numbering. The kill lists are probably layout-ordered most of the time, but it is not an invariant being maintained but the LV updaters.<br>
<br>
A merge algorithm class could easily detect ordering violations and restart itself. Restarting the merge is no slower than calling addRange(), and it will be very effective for the almost-sorted cases.</blockquote><div><br>
</div><div>I actually thought about this, but I think that ultimately a batch interface is *simpler* than an entire separate class to manage merging. The separate class would introduce state between calls to the LiveInterval, or would require exposing an RAII object to manage the process. The latter would probably be the cleanest, but would impose a further burden on callers to have a scope to represent the batch. My typical feelings are that as long as a batch interface consists of an input array, perhaps with order requirements, it's simpler to just expose the batch interface. When the batching requires still more logic or data structures to represent, it's better to expose a dedicated system for it.</div>
<div><br></div><div>Ultimately, I'm not sure its worth it to avoid the coupling here. It does not seem really that bad that LiveIntervalAnalysis is aware and takes advantage of the LiveInterval representation to achieve efficiency. Consider the simplicity of my patch versus the amount of code that would have to be added to support a merge class.<br>
</div><div><br></div><div>That said, clearly the almost-sorted order is a problem. I actually had a pile of test failures sitting in front of me that I failed to look at before hitting the 'send' button, my bad there. I suspect that a simple and fast solution is merely to call 'std::sort' on the vector and document the requirement.</div>
<div><br></div><div>Anyways, let me know if you find my arguments about batch interfaces versus a merge class unconvincing. ;] I'm not in any hurry with this patch as it's just a 10% or 20% drop in very edge-case compilations.</div>
</div></div>