llvm dev - Sep 2018 - LLVMContext: Threads and Ownership.

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/50cbb31c/attachment-0001.html>

>
> 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/17731b7f/attachment.html>

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/7f21bf6e/attachment.html>