On Thu, Sep 25, 2014 at 1:44 AM, Renato Golin <renato.golin at linaro.org> wrote:> On 25 September 2014 06:16, David Blaikie <dblaikie at gmail.com> wrote: > > I can go & dredge up some examples if we want to discuss the particular > > merits & whether each of those cases would be better solved in some other > > way, but it seemed pervasive enough in the current codebase that some > > incremental improvement could be gained by replacing these cases with a > more > > specific tool for the job. We might still consider this tool to be "not > > ideal". > > Having done that in another code base, I can see both merits and problems. > > Our smart pointer behaved more or less like it's described above > (explicit acquire/release)I'm not quite sure I follow you (or that you're following me). The goal isn't about being explicit about acquire/release (indeed I wouldn't mind more implicit acquisition from an always-owning pointer (unique_ptr) to a sometimes-owning pointer (whatever we end up calling it), though release is always going to be explicit I suspect).> and we've seen around 20% performance > improvement over shared_ptr due to savings on acquire/release > semantics.The existing use cases I'm interested in aren't using shared_ptr and wouldn't be ideally migrated to it (in some cases the existing ownership might be on the stack (so it can't be shared) or several layers up the call stack through which raw pointers have been passed (eg: build something, pass it through a few APIs, underlying thing 'wants' to take ownership, but it will be constructed/destroyed before this API returns - so we hack around it either by having a "OwnsThing" flag in the underlying thing, or having a "takeThing" we hope we call before the underlying thing dies and destroys the thing)). We're dealing with types that have raw pointer + bool indicating ownership members or worse, types which take ownership but we lie to about giving them ownership (so some API takes a non-owning T* (or T&), passes it as owning to some other thing, then is sure to take ownership back from that thing and call 'release()" on the unique_ptr to ensure ownership was never really taken).> We had three derivative types of pointers > (Shared/Owned/Linked) which would just differ on the default behaviour > of construction / destruction, but all of them could still explicitly > call getClear / getLink / etc. > > The downside was that almost every interaction with smart pointers had > to be carefully planned and there was a lot of room for errors, >My hope is that having a single construct for conditional ownership will be less confusing than the existing ad-hoc solutions in many places. It's possible that the right answer is to remove the conditional ownership in these APIs entirely, but I'm not sure that's possible/practical/desired (it might be - which is why I've been hesitant to write this abstraction myself just yet - sort of letting it stew in my head a bit to see what feels right).> especially from people that didn't know the context. In the end, the > amount of work that had to be put to make it work was similar than > when dealing with normal pointers,I rather hope that's not the case - these conditionally owning raw pointers are pretty subtle, easy to miss a delete and leak, easy to have an early return and fail to take back and release ownership from something that wasn't really owning in the first place, etc.> but you had to learn yet another > pointer semantics. > > The marginal gain was that pointer interactions were explicit, making > it easier for someone *not* used to C++ pointer semantics to > understand when reading code, not necessarily when writing it. The > marginal lost was getting people that already knew the STL and Boost > smart pointer semantics to get confused. >This is another pointer semantic - I'm not suggesting replacing unique_ptr or shared_ptr - I want to use them as much as possible where they represent the right semantic. I'm just dealing with a situation that doesn't map directly to either of those: conditional ownership.> > Having done that, I still rather use normal pointers and have valgrind > / sanitizers tell me when I screwed up. > > My tuppence. > > cheers, > --renato >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140925/84124a3c/attachment.html>
On Friday, September 26, 2014 01:11 AM, David Blaikie wrote:> > > On Thu, Sep 25, 2014 at 1:44 AM, Renato Golin <renato.golin at linaro.org > <mailto:renato.golin at linaro.org>> wrote: > > On 25 September 2014 06:16, David Blaikie <dblaikie at gmail.com > <mailto:dblaikie at gmail.com>> wrote: > > I can go & dredge up some examples if we want to discuss the particular > > merits & whether each of those cases would be better solved in some other > > way, but it seemed pervasive enough in the current codebase that some > > incremental improvement could be gained by replacing these cases with a more > > specific tool for the job. We might still consider this tool to be "not > > ideal". > > Having done that in another code base, I can see both merits and > problems. > > Our smart pointer behaved more or less like it's described above > (explicit acquire/release) > > > I'm not quite sure I follow you (or that you're following me). The goal > isn't about being explicit about acquire/release (indeed I wouldn't mind > more implicit acquisition from an always-owning pointer (unique_ptr) to > a sometimes-owning pointer (whatever we end up calling it), though > release is always going to be explicit I suspect).Given the example, the smart_ptr can't deal with ownership at all, it merely destroys the pointee in the face of exceptions or forgetfulness (which, in the example, can't really be tolerated).> and we've seen around 20% performance > improvement over shared_ptr due to savings on acquire/release > semantics. > > > The existing use cases I'm interested in aren't using shared_ptr and > wouldn't be ideally migrated to it (in some cases the existing ownership > might be on the stack (so it can't be shared)null_deleter?> or several layers up the > call stack through which raw pointers have been passed (eg: build > something, pass it through a few APIs, underlying thing 'wants' to take > ownership, but it will be constructed/destroyed before this API returns > - so we hack around it either by having a "OwnsThing" flag in the > underlying thing, or having a "takeThing" we hope we call before the > underlying thing dies and destroys the thing)).Obviously combing the ownership flag and the pointer into some kind of "smart type" would be preferable, since they must be dealt with together.> We're dealing with types that have raw pointer + bool indicating > ownership members or worse, types which take ownership but we lie to > about giving them ownership (so some API takes a non-owning T* (or T&), > passes it as owning to some other thing, then is sure to take ownership > back from that thing and call 'release()" on the unique_ptr to ensure > ownership was never really taken).Confusing. null_deleter, again?> We had three derivative types of pointers > (Shared/Owned/Linked) which would just differ on the default behaviour > of construction / destruction, but all of them could still explicitly > call getClear / getLink / etc. > > The downside was that almost every interaction with smart pointers had > to be carefully planned and there was a lot of room for errors, > > > My hope is that having a single construct for conditional ownership will > be less confusing than the existing ad-hoc solutions in many places. > It's possible that the right answer is to remove the conditional > ownership in these APIs entirely, but I'm not sure that's > possible/practical/desired (it might be - which is why I've been > hesitant to write this abstraction myself just yet - sort of letting it > stew in my head a bit to see what feels right).If the code really does look like the example, hesitation is probably wise.> especially from people that didn't know the context. In the end, the > amount of work that had to be put to make it work was similar than > when dealing with normal pointers, > > > I rather hope that's not the case - these conditionally owning raw > pointers are pretty subtle, easy to miss a delete and leak, easy to have > an early return and fail to take back and release ownership from > something that wasn't really owning in the first place, etc. > > but you had to learn yet another > pointer semantics. > > The marginal gain was that pointer interactions were explicit, making > it easier for someone *not* used to C++ pointer semantics to > understand when reading code, not necessarily when writing it. The > marginal lost was getting people that already knew the STL and Boost > smart pointer semantics to get confused. > > > This is another pointer semantic - I'm not suggesting replacing > unique_ptr or shared_ptr - I want to use them as much as possible where > they represent the right semantic. I'm just dealing with a situation > that doesn't map directly to either of those: conditional ownership.I think I may have mentioned null_deleter.> Having done that, I still rather use normal pointers and have valgrind > / sanitizers tell me when I screwed up. > > My tuppence.consider a leaky example: { T* p = new T; if (condition1) { f(p); // takes ownership of p } p->SomeMethod(); if (condition2) { return nullptr; // Leak! } g(p); // don't take ownership of p return p; } Let's see what we can do: { auto p = llvm::make_unique<T>(); if (condition1) { f(p); // takes ownership of p } p->SomeMethod(); if (condition2) { return nullptr; // no leak; } g(p.get()); // don't take ownership of p return p; // not p.get()! } f(llvm::unique_ptr<T>& p) { if(!condition2) // I guess p = llvm::make_unique<T>(p, null_deleter()); [1] } Clearly the code is a mess as f(llvm::unique_ptr<T>&) needs to know condition2, but smart_ptr as described isn't helping anything. [1] std::unique_ptr doesn't have a type erased deleter, but perhaps a unique_ptr with a type erased deleter is actually the right tool for the job. Ben
On Thu, Sep 25, 2014 at 11:06 AM, Ben Pope <benpope81 at gmail.com> wrote:> On Friday, September 26, 2014 01:11 AM, David Blaikie wrote: > >> >> >> On Thu, Sep 25, 2014 at 1:44 AM, Renato Golin <renato.golin at linaro.org >> <mailto:renato.golin at linaro.org>> wrote: >> >> On 25 September 2014 06:16, David Blaikie <dblaikie at gmail.com >> <mailto:dblaikie at gmail.com>> wrote: >> > I can go & dredge up some examples if we want to discuss the >> particular >> > merits & whether each of those cases would be better solved in some >> other >> > way, but it seemed pervasive enough in the current codebase that >> some >> > incremental improvement could be gained by replacing these cases >> with a more >> > specific tool for the job. We might still consider this tool to be >> "not >> > ideal". >> >> Having done that in another code base, I can see both merits and >> problems. >> >> Our smart pointer behaved more or less like it's described above >> (explicit acquire/release) >> >> >> I'm not quite sure I follow you (or that you're following me). The goal >> isn't about being explicit about acquire/release (indeed I wouldn't mind >> more implicit acquisition from an always-owning pointer (unique_ptr) to >> a sometimes-owning pointer (whatever we end up calling it), though >> release is always going to be explicit I suspect). >> > > Given the example, the smart_ptr can't deal with ownership at all, it > merely destroys the pointee in the face of exceptions or forgetfulness > (which, in the example, can't really be tolerated). > > and we've seen around 20% performance >> improvement over shared_ptr due to savings on acquire/release >> semantics. >> >> >> The existing use cases I'm interested in aren't using shared_ptr and >> wouldn't be ideally migrated to it (in some cases the existing ownership >> might be on the stack (so it can't be shared) >> > > null_deleter? > > or several layers up the >> call stack through which raw pointers have been passed (eg: build >> something, pass it through a few APIs, underlying thing 'wants' to take >> ownership, but it will be constructed/destroyed before this API returns >> - so we hack around it either by having a "OwnsThing" flag in the >> underlying thing, or having a "takeThing" we hope we call before the >> underlying thing dies and destroys the thing)). >> > > Obviously combing the ownership flag and the pointer into some kind of > "smart type" would be preferable, since they must be dealt with together. > > We're dealing with types that have raw pointer + bool indicating >> ownership members or worse, types which take ownership but we lie to >> about giving them ownership (so some API takes a non-owning T* (or T&), >> passes it as owning to some other thing, then is sure to take ownership >> back from that thing and call 'release()" on the unique_ptr to ensure >> ownership was never really taken). >> > > Confusing. null_deleter, again? > > We had three derivative types of pointers >> (Shared/Owned/Linked) which would just differ on the default behaviour >> of construction / destruction, but all of them could still explicitly >> call getClear / getLink / etc. >> >> The downside was that almost every interaction with smart pointers had >> to be carefully planned and there was a lot of room for errors, >> >> >> My hope is that having a single construct for conditional ownership will >> be less confusing than the existing ad-hoc solutions in many places. >> It's possible that the right answer is to remove the conditional >> ownership in these APIs entirely, but I'm not sure that's >> possible/practical/desired (it might be - which is why I've been >> hesitant to write this abstraction myself just yet - sort of letting it >> stew in my head a bit to see what feels right). >> > > If the code really does look like the example, hesitation is probably wise. > > especially from people that didn't know the context. In the end, the >> amount of work that had to be put to make it work was similar than >> when dealing with normal pointers, >> >> >> I rather hope that's not the case - these conditionally owning raw >> pointers are pretty subtle, easy to miss a delete and leak, easy to have >> an early return and fail to take back and release ownership from >> something that wasn't really owning in the first place, etc. >> >> but you had to learn yet another >> pointer semantics. >> >> The marginal gain was that pointer interactions were explicit, making >> it easier for someone *not* used to C++ pointer semantics to >> understand when reading code, not necessarily when writing it. The >> marginal lost was getting people that already knew the STL and Boost >> smart pointer semantics to get confused. >> >> >> This is another pointer semantic - I'm not suggesting replacing >> unique_ptr or shared_ptr - I want to use them as much as possible where >> they represent the right semantic. I'm just dealing with a situation >> that doesn't map directly to either of those: conditional ownership. >> > > I think I may have mentioned null_deleter. > > Having done that, I still rather use normal pointers and have valgrind >> / sanitizers tell me when I screwed up. >> >> My tuppence. >> > > consider a leaky example: > { > T* p = new T; > if (condition1) { > f(p); // takes ownership of p > } > p->SomeMethod(); > > if (condition2) { > return nullptr; // Leak! > } > > g(p); // don't take ownership of p > return p; > } >Yeah, imho this is a somewhat uninteresting example - the issue is just that ownership is passed elsewhere, then after that point a non-owning pointer is required. This is statically known and can be coded statically quite simply: unique_ptr ownT = ...; /* stuff */ T *t = ownT.get(); sink(std::move(ownT)); return t; I don't think this particular use case really merits designing a new (fairly powerful) abstraction. But it's not necessarily easy to provide a nice example of the issue that's sufficiently motivating. The use case I have in mind (of which I've seen several in LLVM and Clang) looks something more like this: struct foo { unique_ptr<T> u; foo(unique_ptr<T> u) : u(std::move(u)) {} int stuff(); unique_ptr<T> take() { return std::move(u); } }; int f1(T& t) { // I want to do "stuff" to 't' but I don't own it... // lying through my teeth foo f(std::unique_ptr<T>(&t)); // I really hope 'stuff' doesn't throw, because then we'll end up with a double delete int i = f.stuff(); f.take().release(); // not really leaking because I lied in the first place return i; } That's one case - check the revision numbers mentioned in the first message on this thread for another example that doesn't involve a member, just some rather convoluted ownership semantics (some codepaths there's a default object to point at, other codepaths there's a newly allocated object) it looks something like this: T *t = nullptr; bool Owning = false; if (x) { t = new T; Owning = true; } else t = &default; func(*t); if (Owning) delete t; This comes up not infrequently. The code in func doesn't care if it owns, it just wants to do something. Now obviously in the code I just wrote it could be easily refactored without much duplication: if (x) func(*make_unique<T>()); else func(default); But it's not always that simple. Maybe it can/should be made that simple and we conclude that we don't want/need conditional ownership, but that's what this thread is hopefully here to help decide.> > Let's see what we can do: > > { > auto p = llvm::make_unique<T>(); > if (condition1) { > f(p); // takes ownership of p > } > p->SomeMethod(); > > if (condition2) { > return nullptr; // no leak; > } > > g(p.get()); // don't take ownership of p > return p; // not p.get()! > } > > f(llvm::unique_ptr<T>& p) > { > if(!condition2) // I guess > p = llvm::make_unique<T>(p, null_deleter()); [1] > } > > Clearly the code is a mess as f(llvm::unique_ptr<T>&) needs to know > condition2, but smart_ptr as described isn't helping anything. > > [1] std::unique_ptr doesn't have a type erased deleter, but perhaps a > unique_ptr with a type erased deleter is actually the right tool for the > job.Yes, the lack of type erasure is the issue - and then the lack of compatibility with existing unique_ptrs (I'd obviously like to be able to make a conditional ownership unique_ptr from an unconditional ownership unique_ptr&&, for example, and to be able to take unconditional ownership if it is owned, etc)> > > Ben > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140925/371538fe/attachment.html>