[llvm-dev] LLVMContext: Threads and Ownership.

[Lang Hames via llvm-dev]

> auto C = std::make_shared<LLVMContext>();
> struct ModuleAndSharedContextDeleter { std::shared_ptr<LLVMContext> C;
> operator()(Module *M) { delete M; } /* ctor to init C */};
> std::unique_ptr<Module, ModuleAndSharedDeleter> M(new Module(C.get()),
> ModuleAndSharedContextDeleter(C));


> (or invert this and traffic in structs that have a unique_ptr<Module> and
> a shared_ptr<LLVMContext> as members - though the above example has the
> advantage that it mostly looks like unique_ptr, and you can change
> ownership (pass the unique_ptr<Module, CustomDeleter> to a shared_ptr and
> it'll correctly propagate the deleter, etc))


I believe this would work, but it certainly has a suboptimal feel to it.

For what it is worth, the C API contains some helpful documentation (see
include/llvm-c/LLVMContext.h):

/**
 * Destroy a module instance.
 *
 * This must be called for every created module or memory will be
 * leaked.
 */
void LLVMDisposeModule(LLVMModuleRef M);

So based on the C-API's documentation we are at least free to remove this
strange fallback-ownership model of LLVMContext.

Cheers,
Lang.




On Sun, Sep 16, 2018 at 4:55 PM David Blaikie <dblaikie at gmail.com> wrote:

> In the most basic case, I'd imagine something like this:
>
> auto C = std::make_shared<LLVMContext>();
> struct ModuleAndSharedContextDeleter { std::shared_ptr<LLVMContext> C;
> operator()(Module *M) { delete M; } /* ctor to init C */};
> std::unique_ptr<Module, ModuleAndSharedDeleter> M(new Module(C.get()),
> ModuleAndSharedContextDeleter(C));
>
> (or invert this and traffic in structs that have a unique_ptr<Module> and
> a shared_ptr<LLVMContext> as members - though the above example has the
> advantage that it mostly looks like unique_ptr, and you can change
> ownership (pass the unique_ptr<Module, CustomDeleter> to a shared_ptr and
> it'll correctly propagate the deleter, etc))
> ...
>
> But it sounds like you're concerned with a situation in which there are a
> wide variety of things making Modules that are maybe outside the purview of
> Orc but need this ownership model? That would seem unfortunate & I'm not
> quite sure I'm picturing the situation you have in mind.
>
> On Sun, Sep 16, 2018 at 4:27 PM Lang Hames <lhames at gmail.com> wrote:
>
>> I'd think/suggest ref-counted LLVMContext ownership would be done by
>>> wrapping/external tracking in this use case.
>>
>>
>> What kind of wrapping are you imagining? I'm worried that maintaining an
>> external context ref-count is going to be awkward and error prone, but
>> perhaps there are idioms that would make this easier than I am imagining.
>>
>> I think it may be simpler and safer to expose the number of Modules
>> managed by an LLVMContext. That way the "ref-count" always reflects the
>> number of modules using the context, and tracking can be set up and managed
>> entirely from the LLVMContext creation site (by adding the context to an
>> 'automatically managed' set), rather than intruding to every Module
>> creation point.
>>
>> We would still need to add a mutex to LLVMContext to make this thread
>> safe though.
>>
>> -- Lang.
>>
>> On Sun, Sep 16, 2018 at 10:22 AM David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>> Agreed, the existing ownership seems sub-optimal. I wouldn't say broken,
>>> but subtle at least - looks like you get the choice to either manage the
>>> ownership of the Module object yourself, or let the context handle it (eg:
>>> currently it'd be valid to just do "{ LLVMContext C; new Module(C); new
>>> Module(C); }" - Modules end up owned by the context and cleaned up there).
>>>
>>> Might be hard to migrate existing users away from this without silently
>>> introducing memory leaks... maybe with some significant API breakage - move
>>> Module construction to a factory/helper that returns a
>>> std::unique_ptr<Module> - requiring every Module construction to be
>>> revisited, and users relying on LLVMContext based ownership/cleanup to
>>> redesign their code.
>>>
>>> As to the original question - gut reaction: this doesn't seem like
>>> something that's general-purpose enough to be implemented in the
>>> LLVMContext/Module itself. I think a reasonable ownership model for
>>> LLVMContext/Module is that the user is required to ensure the LLVMContext
>>> outlives all Modules created within it (same way a user of std::vector is
>>> required to ensure that the vector is not reallocated so long as they're
>>> keeping pointers/references to elements in it). I'd think/suggest
>>> ref-counted LLVMContext ownership would be done by wrapping/external
>>> tracking in this use case.
>>>
>>> - Dave
>>>
>>> On Sat, Sep 15, 2018 at 9:30 PM Lang Hames via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> Actually, looking at the destructors for LLVMContext and Module I do
>>>> not think the current ownership scheme makes sense, so this might be a good
>>>> opportunity to re-think it.
>>>>
>>>> Right now an LLVMContext owns a list of modules (see
>>>> LLVMContextImpl::OwnedModules) that it destroys when its destructor is
>>>> called.  Modules remove themselves from this list if they are destructed
>>>> before the context:
>>>>
>>>> Module::~Module() {
>>>>   Context.removeModule(this);
>>>>   ...
>>>>
>>>> LLVMContextImpl::~LLVMContextImpl() {
>>>>   // NOTE: We need to delete the contents of OwnedModules, but Module's
>>>> dtor
>>>>   // will call LLVMContextImpl::removeModule, thus invalidating
>>>> iterators into
>>>>   // the container. Avoid iterators during this operation:
>>>>   while (!OwnedModules.empty())
>>>>     delete *OwnedModules.begin();
>>>>     ...
>>>>
>>>> This makes it unsafe to hold a unique_ptr to a Module: If any Module is
>>>> still alive when its context goes out of scope it will be double freed,
>>>> first by the LLVMContextImpl destructor and then again by the unique ptr.
>>>> Idiomatic scoping means that we tend not to see this in practice  (Module
>>>> takes an LLVMContext reference, meaning we always declare the context
>>>> first, so it goes out of scope last), but makes the context ownership
>>>> redundant: the modules are always freed first via their unique_ptr's.
>>>>
>>>> I don't think it makes sense for LLVMContext to own Modules. I think
>>>> that Modules should share ownership of their LLVMContext via a shared_ptr.
>>>>
>>>> Thoughts?
>>>>
>>>> Cheers,
>>>> Lang.
>>>>
>>>> On Sat, Sep 15, 2018 at 4:14 PM Lang Hames <lhames at gmail.com> wrote:
>>>>
>>>>> Hi All,
>>>>>
>>>>> ORC's new concurrent compilation model generates some interesting
>>>>> lifetime and thread safety questions around LLVMContext: We need multiple
>>>>> LLVMContexts (one per module in the simplest case, but at least one per
>>>>> thread), and the lifetime of each context depends on the execution path of
>>>>> the JIT'd code. We would like to deallocate contexts once all modules
>>>>> associated with them have been compiled, but there is no safe or easy way
>>>>> to check that condition at the moment as LLVMContext does not expose how
>>>>> many modules are associated with it.
>>>>>
>>>>> One way to fix this would be to add a mutex to LLVMContext, and expose
>>>>> this and the module count. Then in the IR-compiling layer of the JIT we
>>>>> could have something like:
>>>>>
>>>>> // Compile finished, time to deallocate the module.
>>>>> // Explicit deletes used for clarity, we would use unique_ptrs in
>>>>> practice.
>>>>> auto &Ctx = Mod->getContext();
>>>>> delete Mod;
>>>>> std::lock_guard<std::mutex> Lock(Ctx->getMutex());
>>>>> if (Ctx.getNumModules())
>>>>>   delete Ctx;
>>>>>
>>>>> Another option would be to invert the ownership model and say that
>>>>> each Module shares ownership of its LLVMContext. That way LLVMContexts
>>>>> would be automatically deallocated when the last module using them is
>>>>> destructed (providing no other shared_ptrs to the context are held
>>>>> elsewhere).
>>>>>
>>>>> There are other possible approaches (e.g. side tables for the mutex
>>>>> and module count) but before I spent too much time on it I wanted to see
>>>>> whether anyone else has encountered these issues or has opinions on
>>>>> solutions.
>>>>>
>>>>> Cheers,
>>>>> Lang.
>>>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180916/ee631597/attachment.html>