[llvm-commits] PATCH: Fix quadratic behavior in PR12652

[Chandler Carruth]

On Fri, Jul 6, 2012 at 3:40 PM, Chandler Carruth <chandlerc at gmail.com>wrote:

> On Fri, Jul 6, 2012 at 9:10 AM, Jakob Stoklund Olesen <stoklund at 2pi.dk>wrote:
>
>>
>> On Jul 6, 2012, at 1:28 AM, Chandler Carruth <chandlerc at gmail.com> wrote:
>>
>> > This patch adds what seems to me to be a very efficient merge operation
>> for LiveInterval to address the quadratic behavior seen in PR12652 when
>> merging extremely large, extremely distant disjoint intervals created by
>> ASan's strange code layout.
>>
>> Thanks for fixing this, Chandler.
>>
>> It's good to get rid of the quadratic algorithm, but I am a bit concerned
>> about the typical case because you are a) copying the whole thing on every
>> call, and b) calling malloc/free at least once per call.
>>
>> Could you measure the compile time impact on something mundane, like
>> compiling clang, please?
>>
>
> Yea, I'll dig a bit into the common case.
>
>
>>
>> > There is one other user of addRangeFrom left (join) and I think I can
>> refactor this code a bit and share the merge logic for that routine as
>> well. But I didn't want to do that in this patch.
>>
>> I am not sure that join() should use the same algorithm. It gets called a
>> lot with a very small RHS interval that merges into the existing large LHS
>> without shifting things around. Your algorithm would turn O(logN) into O(N)
>> in that case. (Plus the malloc thing).
>>
>> To exploit this common case (LHS large, RHS tiny, LHS.size() doesn't
>> change), you could:
>>
>
> Ah, I didn't realize that size not changing was the common case! It makes
> sense though. If size had been changing each time, just building a new
> vector doesn't seem so bad.
>
>
>>
>> 1. Start with a binary search in case RHS starts in the middle of a huge
>> LHS.
>>
>> 2. Merge directly into LHS whenever possible, use NewRanges only as an
>> overflow area.
>>
>> 3. Grow LHS to the required size, and going backwards, merge NewRanges
>> into LHS.
>>
>
> Yea, I'll look at this (or at least something quite similar) so that we do
> as much as we can to leave the LHS in place when it doesn't need to grow.
>
> I'll integrate this (as wel as your and Ben's comments) into a fresh patch
> shortly for you to look at before I commit.
>


This isn't nearly finished, and its still missing both some cleanups I
suspect and it has a few bugs that I haven't had time to track down, but
wanted to see if this is more along the lines you were thinking of
algorithm-wise. New patch attached.

If (algorithmically) you're liking this direction, then after cleaning up,
fixing bugs, and testing performance in both cases, is this a good initial
step to commit? Not precluding any further work to make join() use the same
logic, and ways of factoring the value number manipulation out of the merge
logic....
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120706/967ad30e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr12652.patch
Type: application/octet-stream
Size: 7105 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120706/967ad30e/attachment.obj>