[PATCH] Add reference counting support to SourceMgr

[Alp Toker]

On 27/06/2014 00:49, David Blaikie wrote:
> On Wed, Jun 25, 2014 at 12:46 AM, Alp Toker <alp at nuanti.com> wrote:
>> On 25/06/2014 08:39, David Blaikie wrote:
>>> Not sure where is best to have the discussion, but I'd like to
>>> understand why this refcounting needs to be /intrusive/. It always
>>> seems like a wart if code is taking ownership without documenting that
>>> contract through the type system (and this is case where intrusive
>>> reference counting is required/used - a raw pointer is passed around
>>> and some subsystem takes ownership of that, while sharing it with some
>>> other, unknown, owner).
>>
>> That's the theory, but in practice we're just passing around or storing a
>> MemoryBuffer/bool pair in various parts of clang, and doing it
>> inconsistently. The moment we hit a function without a
>> DoesNotFree/Owns/DontRelease/CallerOwns/FreeAtEnd parameter, we end up
>> copying the memory buffer as a workaround.
>>
>> Intrusive reference counting encapsulated within SourceMgr/SourceManager
>> gives us a chance to fix all the known memory issues in one shot without a
>> lot of surgery and without exposing the ownership detail to consumers, which
>> is cool.
> This, I think, is where we disagree. I don't like hiding/pretending
> that that ownership isn't what it is. It makes clients
> subtle/confusing.

The hiding is completely intentional. I'm aiming to fix all the 
redundant file stats and re-reads, and the first step is to get some 
transfer going between layers. Right now the ownership is either subtle, 
managed by bool "isOwned" flags, or non-existent -- files just get read 
from disk into memory repeatedly all the time so it's not like we're in 
a good place now.

Once the first step is done, we'll need to centralize file loading to 
SourceMgr or a similar class. At that point we can tear away the 
reference counting and the parameters will be plain pointers again, with 
buffer ownership managed by a central LRU.

What we really don't want is to change hundreds of function prototypes 
between std::unique_ptr<>, IntrusiveRefCntPtr<> and plain pointers every 
time the underlying scheme changes -- and it's going to change a few times.

It's possible to implement this transparently without churn and I'm 
pretty certain this is the best way to get it done incrementally.

Alp.


>
>> I think it actually models and optimizes what the code is trying to
>> do right now, even if it doesn't model what we ultimately want:
>>
>> Some day it'll be great to introduce a centralised source manager that's
>> smart enough not only to handle storage, but also to unique requests for
>> memory mapped files and manage OS resources responsibly, as well as
>> coordinating with the stat cache. But that's some way off.
>>
>> Alp.
>>
>>
>>> - David
>>>
>>> On Tue, Jun 24, 2014 at 10:27 PM, Alp Toker <alp at nuanti.com> wrote:
>>>> The attached patch switches SourceMgr to use reference counting
>>>> internally
>>>> by making MemoryBuffer optionally refcounted.
>>>>
>>>> With this change it becomes possible to share ownership of memory buffers
>>>> between multiple source managers in the backend and frontend instead of
>>>> copying file contents into memory or re-reading from disk repeatedly.
>>>>
>>>> These are supporting changes for clang with no functional change on the
>>>> LLVM
>>>> side.
>>>>
>>>> See thread "[PATCH] Don't duplicate entire file contents in memory"
>>>> posted
>>>> to cfe-commits for details.
>>>>
>>>> Alp.
>>>>
>>>> --
>>>> http://www.nuanti.com
>>>> the browser experts
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>> --
>> http://www.nuanti.com
>> the browser experts
>>

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