{"action":"created","issue":{"url":"https://api.github.com/repos/idaholab/moose/issues/7062","repository_url":"https://api.github.com/repos/idaholab/moose","labels_url":"https://api.github.com/repos/idaholab/moose/issues/7062/labels{/name}","comments_url":"https://api.github.com/repos/idaholab/moose/issues/7062/comments","events_url":"https://api.github.com/repos/idaholab/moose/issues/7062/events","html_url":"https://github.com/idaholab/moose/issues/7062","id":156527375,"number":7062,"title":"Smart Pointer Usage in MOOSE (Discussion)","user":{"login":"permcody","id":3036960,"avatar_url":"https://avatars.githubusercontent.com/u/3036960?v=3","gravatar_id":"","url":"https://api.github.com/users/permcody","html_url":"https://github.com/permcody","followers_url":"https://api.github.com/users/permcody/followers","following_url":"https://api.github.com/users/permcody/following{/other_user}","gists_url":"https://api.github.com/users/permcody/gists{/gist_id}","starred_url":"https://api.github.com/users/permcody/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/permcody/subscriptions","organizations_url":"https://api.github.com/users/permcody/orgs","repos_url":"https://api.github.com/users/permcody/repos","events_url":"https://api.github.com/users/permcody/events{/privacy}","received_events_url":"https://api.github.com/users/permcody/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/idaholab/moose/labels/C:%20MOOSE","name":"C: MOOSE","color":"0052cc"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":51,"created_at":"2016-05-24T14:47:27Z","updated_at":"2016-05-25T19:53:50Z","closed_at":null,"body":"We need to chat about what kinds of smart pointers we should be using them and how we should be using them in the framework. According to the new Scott Meyer's book. He recommends `std::unique_ptr()` as the go to pointer to replace raw pointers in a lot of contexts. It's very fast since there is simply a pointer to the destructor and there's no other complications to deal. Shared pointers on the other hand actually require two words (128 bytes) to store and there's a \"control block\" for each object stored by a shared pointer. As those pointers are copied around there are multiple hits (at least one increment and decrement) to update the information in the control block which is protected with atomics which can severely impact performance especially in threaded regions.\r\n\r\nI think the fear of using unique_ptrs is that as you copy them, the owner's pointer becomes invalid and that has left a nasty taste in some people's mouths. This is fine for some things and not fine for others but it goes hand in hand with using move semantics in C++11 of which we can start taking advantage of throughout. Scott points out that Factory patterns should _always_ use unique pointers to construct objects. The Factory doesn't need a pointer to the object after it's constructed so it's really fast an efficient to build a \"unique_ptr\" object and pass it out of the factory. In contexts where those objects might need to be shared you can simply make them shared on demand. He points out that if you understand the lifetimes of your objects you can simply create raw pointers from your unique pointers for general access. We absolutely do know who owns all objects in MOOSE, and we know when they are destructed so again, it seems like a smart idea.\r\n\r\nI realize that some of this is moot because hopefully we don't need to worry about the costs of copying around factory constructed objects. That happens once per simulation. However, we've already seen people passing smart pointers by reference for no reason other than C++98 best practices of passing non-POD objects by reference. This needs to go away with C++11. It's no longer the best or only way to get the best performance (as half of Scott's book talks about).\r\n\r\nThe only huge downside to using unique ptrs right now, is that \"make_unique\" is MISSING in C++11! This however can be rectified with a simple wrapper that we can add to MOOSE. I'd like to start this discussion with @idaholab/moose-developers and see what people think? Should we be using a few more unique_ptrs instead of turning to shared_ptrs throughout the framework?"},"comment":{"url":"https://api.github.com/repos/idaholab/moose/issues/comments/221687642","html_url":"https://github.com/idaholab/moose/issues/7062#issuecomment-221687642","issue_url":"https://api.github.com/repos/idaholab/moose/issues/7062","id":221687642,"user":{"login":"friedmud","id":870705,"avatar_url":"https://avatars.githubusercontent.com/u/870705?v=3","gravatar_id":"","url":"https://api.github.com/users/friedmud","html_url":"https://github.com/friedmud","followers_url":"https://api.github.com/users/friedmud/followers","following_url":"https://api.github.com/users/friedmud/following{/other_user}","gists_url":"https://api.github.com/users/friedmud/gists{/gist_id}","starred_url":"https://api.github.com/users/friedmud/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/friedmud/subscriptions","organizations_url":"https://api.github.com/users/friedmud/orgs","repos_url":"https://api.github.com/users/friedmud/repos","events_url":"https://api.github.com/users/friedmud/events{/privacy}","received_events_url":"https://api.github.com/users/friedmud/received_events","type":"User","site_admin":false},"created_at":"2016-05-25T19:53:50Z","updated_at":"2016-05-25T19:53:50Z","body":">about about this case?\r\n\r\nThat's why I said our policy would be to _pass_ `shared_ptr` by reference and _store_ `shared_ptr` by copy.  _Storing_ a shared pointer by reference never makes sense.\r\n\r\n>That actually is stupid code. Yeah the '=' overload is less obvious but if you actually do a standard move and then wonder why the original isn't there anymore... Well...\r\n\r\nThe is use is who is \"_you_\"?  One person write one piece of code... another person writes another... and you end up in a bad situation.  It's even worse if the person writing `afunc()` instantiates an object that they didn't write that calls `std::move` internally (unknown to them).\r\n\r\nIf an api hands you a `unique_ptr` I feel like you're free to assume that you have full responsibility for that memory and can `std::move` it to your heart's content.  Also: consider trying to put the `unique_ptr` into a container like a `std::vector`... you _have_ to move it to get it in there... which will invalidate the external version of the `unique_ptr`.\r\n\r\nAll I'm pointing out is that passing `unique_ptr` by reference is a bad idea... especially in a large framework with multiple developers.  If you're going to pass `unique_ptr`... do it by `std::move` and do it on purpose to give that memory to the callee.\r\n\r\nThis really is a problem just with `unique ptr`.  Consider the same code (which is still unsafe, but less problematic) with raw pointers:\r\n\r\n```C++\r\n#include <iostream>\r\n\r\nvoid afunc(int * & ptr)\r\n{\r\n  auto a = ptr;\r\n  std::cout<<*a<<std::endl;\r\n}\r\n\r\nint main()\r\n{\r\n  auto a  = new int(1);\r\n\r\n  afunc(a);\r\n\r\n  std::cout<<*a<<std::endl;\r\n\r\n  return 0;\r\n}\r\n```\r\n\r\nIt works just fine.\r\n\r\nThe \"issue\" with `unique_ptr` is that there is NO WAY to make a \"copy\" of it without invalidating the original.  Many would say that's an \"asset\"... and I would agree with them... as long as you are using `unique_ptr` where you really want memory custody to be exchanged.\r\n\r\nIf you're only using `unique_ptr` because you're trying to optimize around the control block and reference counting of `shared_ptr` then you're making a mistake.\r\n\r\nNow - using a `unique_ptr` and passing around raw pointers is fine... if you like raw pointers and don't mind having two different ways to store things :smile:"}}
{"actor":{"display_login":"friedmud"}}