[llvm] r211595 - Pass a unique_ptr<MemoryBuffer> to the constructors in the Binary hierarchy.

[Alp Toker]

On 05/07/2014 03:19, Rafael Espíndola wrote:
>> Agree, we don't need to refcount in many cases where a reference or pointer
>> will do. delete/unique_ptr continues to work in those cases, like the
>> snippet in my previous mail.
> You started this thread saying it was a problem to use unique_ptr. I
> am confused.

There is a problem. Your code is taking unique ownership of what is a 
shared resource.

MemoryBuffers can be used by any number of consumers if you pass them by 
pointer. A single mmap with one MemoryBuffer allocation can satisfy any 
number of objects that need to read into that file, so why bake 
std::unique_ptr so deep into all these interfaces and function parameters?

I haven't seen any single place in that code that requires each instance 
to have its own, unique memory buffer which is why I'm raising the question.

>
> Lets be clear, if the only case that needs reference counting are some
> files in clang, only those should get a reference count. Why not just
> use a shared_ptr in there? I *really* don't want to see reference
> counting being force into users that don't need it.

I started this thread to point out that passing around by unique_ptr 
doesn't make sense for interfaces that have no real ownership 
requirement, certainly not to force reference counting on anyone.

On the other hand it looks like you have exactly the same problem as the 
clang frontend in this code because it needlessly unmaps and re-mmaps 
files each time they're used, so why not work together on the solution?

Alp.

>
>>> Is there a case where we copy the buffer that is used for an ObjectFile?
>>
>> materializeAllPermanently() is called right now to avoid the copy. It avoids
>> memory duplication, but at a high cost of having to do lots of work early on
>> regardless of whether it's going to be used.
>>
>> The new scheme will get rid of that early materialization so construction
>> becomes a trivial operation, and any materialization can happen on-demand,
>> for instance in a in a separate compilation/linking phase.
>>
>> You know the details better, but I imagine in some cases the materialization
>> can probably be avoided completely for LTO.
> Not with ld64. That was added because of an specific optimation, it
> has nothing to do with avoid copies.
>
> Cheers,
> Rafael

-- 
http://www.nuanti.com
the browser experts