<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">If you just want to use shared_ptr everywhere (for both the context and the modules), the complexity would leak into fewer APIs, because shared_ptr type erases its deleter.</blockquote><div><br></div><div>I do not think there's a motivation for changing Module's ownership, but owning LLVMContext by a shared_ptr makes sense if you think of LLVMContext as a 'pool' data structure that can be referred to by multiple Modules, rather than as a container for modules.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">But yeah, feels weird to me to change the ownership of LLVMContext for this one use case </blockquote><div><br></div><div>I think of this discussion as being about the "right" ownership model for LLVMContext, with this use case as a motivating example of the problems with the current model. The question to ask is: If we were designing this ownership model over, would we do it the same way? Of course it is reasonable to answer: "No we would not, but the cost of switching to a more sensible model now is prohibitive".</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Also would come back to some discussion (maybe easier in person or over IRC, etc, not sure) about whether there's a way to design things so shared ownership isn't needed even for ORC.</blockquote><div><br></div><div>When it comes to LLVMContext we already have the following constraints: (1) Multiple modules must be able to refer to the same LLVMContext, (2) Modules must never out-live their context. If you add the desire to de-allocate contexts once they're no longer referenced then any scheme you come up with is going to be shared-ptr by another name/mechanism.</div><div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Based on the documentation, yeah - though I'd expect it'd still break some folks - and it's the C++ API users I'd expect we have more of at this level & likely to introduce some quiet leaks.</blockquote></div><div><br></div><div>On its own I don't think potential C++ API leaks are much of a barrier: As you said, we could change the construction API to always return unique_ptr if that is the model we wanted to enforce.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Be nice to fail hard (though we don't have much way to do that - assert fail, but that's only in assert builds, etc) - could keep the list of modules around for a while & assert if it's non-empty when the context is destroyed. Not perfect, but at least it's something.</blockquote><div><br></div><div>If we were to switch to shared ownership of contexts then the invariant could never be broken, at the expense of some predictability about the lifetimes: Scenarios that would previously have been a violation of contract would now implicitly extend the LLVMContext lifetime. That said, if you're still holding a Module and doing something with it, I'm 99% confident that what you really wanted was for its context to remain alive.</div><div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div><div class="gmail_quote"><div dir="ltr">On Sun, Sep 16, 2018 at 5:53 PM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Sun, Sep 16, 2018 at 5:38 PM Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">auto C = std::make_shared<LLVMContext>();<br>struct ModuleAndSharedContextDeleter { std::shared_ptr<LLVMContext> C; operator()(Module *M) { delete M; } /* ctor to init C */};<br>std::unique_ptr<Module, ModuleAndSharedDeleter> M(new Module(C.get()), ModuleAndSharedContextDeleter(C));</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><br>(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))</blockquote><div> </div></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>I believe this would work, but it certainly has a suboptimal feel to it.</div></div></div></div></blockquote><div><br>If you just want to use shared_ptr everywhere (for both the context and the modules), the complexity would leak into fewer APIs, because shared_ptr type erases its deleter.<br><br>But yeah, feels weird to me to change the ownership of LLVMContext for this one use case - if there were more users that wanted to have it, I could see building it in in a more convenient way (but maybe still in the form of a typedef common for unique_ptr<Module, CustomDeleter>, built-in factory functions/helpers/etc to make that more mainstream).<br><br>Also would come back to some discussion (maybe easier in person or over IRC, etc, not sure) about whether there's a way to design things so shared ownership isn't needed even for ORC - I don't have enough info to fully understand where/how it'd be used here/whether there's some good alternatives.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>For what it is worth, the C API contains some helpful documentation (see include/llvm-c/LLVMContext.h):</div><div><br></div><div><div><font face="monospace, monospace">/**</font></div><div><span style="font-family:monospace,monospace"> * Destroy a module instance.</span></div><div><span style="font-family:monospace,monospace"> *</span></div><div><span style="font-family:monospace,monospace"> * This must be called for every created module or memory will be</span></div><div><font face="monospace, monospace"> * leaked.</font></div><div><span style="font-family:monospace,monospace"> */</span></div><div><span style="font-family:monospace,monospace">void LLVMDisposeModule(LLVMModuleRef M);</span></div></div><div><br></div><div>So based on the C-API's documentation we are at least free to remove this strange fallback-ownership model of LLVMContext.</div></div></div></div></blockquote><div><br>Based on the documentation, yeah - though I'd expect it'd still break some folks - and it's the C++ API users I'd expect we have more of at this level & likely to introduce some quiet leaks.<br><br>Be nice to fail hard (though we don't have much way to do that - assert fail, but that's only in assert builds, etc) - could keep the list of modules around for a while & assert if it's non-empty when the context is destroyed. Not perfect, but at least it's something.<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br></div><div>Cheers,</div><div>Lang. </div><div><br></div><div> </div><div dir="ltr"><br></div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Sun, Sep 16, 2018 at 4:55 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">In the most basic case, I'd imagine something like this:<br><br>auto C = std::make_shared<LLVMContext>();<br>struct ModuleAndSharedContextDeleter { std::shared_ptr<LLVMContext> C; operator()(Module *M) { delete M; } /* ctor to init C */};<br>std::unique_ptr<Module, ModuleAndSharedDeleter> M(new Module(C.get()), ModuleAndSharedContextDeleter(C));<br><br>(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))<br>...<br><br>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. </div><br><div class="gmail_quote"><div dir="ltr">On Sun, Sep 16, 2018 at 4:27 PM Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">I'd think/suggest ref-counted LLVMContext ownership would be done by wrapping/external tracking in this use case.</blockquote><div dir="ltr"><br></div></div><div dir="ltr"><div>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.</div><div><br></div><div>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.</div><div><br></div><div>We would still need to add a mutex to LLVMContext to make this thread safe though.</div></div><div dir="ltr"><div><br></div><div>-- Lang.</div></div><br><div class="gmail_quote"><div dir="ltr">On Sun, Sep 16, 2018 at 10:22 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">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). <br><br>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.<br><br>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.<br><br>- Dave</div><br><div class="gmail_quote"><div dir="ltr">On Sat, Sep 15, 2018 at 9:30 PM Lang Hames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">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.<div><br></div><div>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:</div><div><br></div><div><div><font face="monospace, monospace">Module::~Module() {</font></div><div><font face="monospace, monospace">  Context.removeModule(this);</font></div></div><div><font face="monospace, monospace">  ...</font></div><div><br></div><div><font face="monospace, monospace">LLVMContextImpl::~LLVMContextImpl() {</font></div><div><font face="monospace, monospace">  // NOTE: We need to delete the contents of OwnedModules, but Module's dtor</font></div><div><font face="monospace, monospace">  // will call LLVMContextImpl::removeModule, thus invalidating iterators into</font></div><div><font face="monospace, monospace">  // the container. Avoid iterators during this operation:</font></div><div><font face="monospace, monospace">  while (!OwnedModules.empty())</font></div><div><font face="monospace, monospace">    delete *OwnedModules.begin();</font></div><div><font face="monospace, monospace">    ...</font></div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Thoughts?</div><div><br></div><div>Cheers,</div><div>Lang.</div></div></div></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Sat, Sep 15, 2018 at 4:14 PM Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi All,<div><br></div><div>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.</div><div><br></div><div>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:</div><div><br></div><div><font face="monospace, monospace">// Compile finished, time to deallocate the module.</font></div><div><font face="monospace, monospace">// Explicit deletes used for clarity, we would use unique_ptrs in practice.</font></div><div><font face="monospace, monospace">auto &Ctx = Mod->getContext();</font></div><div><font face="monospace, monospace">delete Mod;</font></div><div><font face="monospace, monospace">std::lock_guard<std::mutex> Lock(Ctx->getMutex());</font></div><div><font face="monospace, monospace">if (Ctx.getNumModules())</font></div><div><font face="monospace, monospace">  delete Ctx;</font></div><div><br></div><div>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).</div><div><br></div><div>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.</div><div><br></div><div>Cheers,</div><div>Lang.</div></div>
</blockquote></div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div></div>
</blockquote></div></div></div></div></div></div></div></div></div>