[PATCH] Add reference counting support to SourceMgr

[David Blaikie]

On Thu, Jul 3, 2014 at 11:32 PM, Alp Toker <alp at nuanti.com> wrote:
>
> 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.

Is there a reason this centralization couldn't come first? Rather than
having the intermediate state of intrusive ref counting, etc?

> 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.

It might be - I'm just trying to understand it better. I hesitate to
add intrusive ref counting support to an already convoluted memory
management device (given MemoryBuffer's owning/non-owning on top of
various conditional ownership schemes of the MemoryBuffers
themselves... ).

>
> 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
>