[PATCH] LLD: Use std::vector to store SimpleReferences instead of linked list.

[Jean-Daniel Dupas]

> Le 17 mars 2015 à 21:03, Duncan P. N. Exon Smith <dexonsmith at apple.com> a écrit :
> 
> +llvm-commites
> 
>> On 2015-Mar-17, at 12:47, Rui Ueyama <ruiu at google.com> wrote:
>> 
>>> On Tue, Mar 17, 2015 at 10:19 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>> 
>>>> On 2015-Mar-15, at 23:07, Rui Ueyama <ruiu at google.com> wrote:
>>>> 
>>>> I think SmallVector guarantees the array it contains is contiguous in memory. That data structure does not work well with BumpPtrAllocator because garbage after extending the array is not reclaimed until the whole buffer is discarded. That makes memory consumption of a small vector from O(n) to O(n^2).
>>>> 
>>>> That being said, that behavior might be acceptable if we rarely extend SmallVectors.
>>> 
>>> This sounds dangerous.
>>> 
>> Agreed.
>> 
>>>> So, maybe it's not a bad idea, but I don't know if it's good enough to add an additional parameter to SmallVector's constructor.
>>> 
>>> If anything, what you want is a recycling allocator.  If you just have one
>>> vector, then you still get N^2 memory usage, but if you have at least O(N)
>>> vectors, it should be amortized O(N) memory per vector.  (I don't know
>>> whether that's true for this problem.)
>>> 
>>> But I'm not sure what the benefit is anyway.  Has anyone actually observed
>>> a memory issue here?
>> 
>> Since currently we don't leak memory, there's no memory issue. It's more like API issue that we use a memory allocator that doesn't call destructors for allocated objects so we can't do anything that depends on destructors.
> 
> That's not true.  The allocator isn't in charge of calling destructors;
> it's just in charge of freeing memory.  You can call destructors if you'd
> like to.
> 
>> 
>> What if we just use new instead of BumpPtrAllcoator? Would it really make the linker slow? We surely do something tricky here to use BumpPtrAllocator, which can be eliminated if we simply use new.
> 
> I don't know this code well enough to know whether it makes the linker
> slower, but I imagine you'd want to profile this with large inputs before
> changing it.

If you do some profiling, don’t forget to mesure the impact of calling destructor on many thousand of references. While it is not an issue for single lld invocation, we should not forget that lld is designed to be used as a library.

And before spending too much time on that ‘non’ issue, we should probably start by fixing other real memory leaks. Actually LLD is unusable as a library due to a massive memory leak caused by « File » objects. Files don’t have a proper owner and are just allocated and release in the wild (see FileArchive for instance which just release() unique pointer without passing ownership).