Jump to content

  • Log In with Google      Sign In   
  • Create Account


Overusing smart pointers?


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
32 replies to this topic

#21 sundersoft   Members   -  Reputation: 216

Like
0Likes
Like

Posted 19 June 2012 - 01:47 PM

I've started changing alot of my code now.

But I have a case where I use a unique_ptr as its a factory function that creates the object.. I then move this pointer into a owning container.. but I also have to set a currently selected object in another class.. should I rather use a shared_ptr in this case and use a weak_ptr for where I set the currently selected object? Or should I use a raw pointer for the selected object and get the raw pointer out of the unique pointer like I do below?


Raw pointer. If the object is deleted and then accessed by the currently selected pointer, that would usually indicate an error in your code and both weak_ptr and raw pointers will cause a runtime error here (assuming you don't check for a null shared_ptr from the weak_ptr's lock function). It's possible that a memory access to deleted memory will not cause a runtime error but this is rare and usually yields garbage data.

Using shared_ptr generally implies that there is shared ownership of the pointed-to object, so having only one shared_ptr to an object means that there is exclusive ownership, which makes the code less clear in addition to having more overhead. It's also possible that a second shared_ptr could be created by mistake which points to the same object, which is generally not desired (removing the object from the container should delete it) and not possible with the unique_ptr implementation.

Using shared_ptr for both the currently selected pointer and the container of objects would cause the object to remain in memory when the object is removed from the container while being referenced by the currently selected pointer, but this condition is usually errorneous (the selected pointer should generally only point to objects in the container), so this will mask the error and make it hard to detect.

The object may require shared ownership without considering the existance of the currently selected pointer (for example, if the object is immutable then you might want to copy a pointer to it instead of doing a full copy, in which case shared_ptr is appropriate). In this case, weak_ptr should be preferred since the overhead of having another weak_ptr instead of a raw pointer when the shared_ptr already exists is neglegible and the weak_ptr is more likely to detect errors. However, checking if the shared_ptr returned by weak_ptr's lock function is null should usually be omitted since it is usually a non-recoverable error to have the weak_ptr return a null shared_ptr.

Sponsor:

#22 wack   Members   -  Reputation: 1225

Like
0Likes
Like

Posted 19 June 2012 - 02:17 PM

This is just my opinion, which may be a little bit on the conservative side, but I haven't seen much point in passing unique_ptrs around or keeping them in containers. This is a short example of what would be be a typical usage of unique_ptr for me:


         unique_ptr<tempfile_c> tempfile(new tempfile_c(ios::binary|ios::out));
         if (!tempfile->is_open())
            return FATAL_ERROR_TEMPFILE;
		  //insert other early returncode or code with possible exceptions here
         m_binary_tempfiles.insert(make_pair(field_id, tempfile.release()));
In the above example, the pointer is owned by the unique_ptr until it's ready to be assigned to the real owner (m_binary_tempfiles, a memer variable) with release().
Sure, I still have to clean m_binary_tempfiles in the destructor, but that's fine by me - it's all exception safe, and the data type declaration of m_binary_tempfiles doesn't make eyes bleed.

I've started changing alot of my code now.

But I have a case where I use a unique_ptr as its a factory function that creates the object.. I then move this pointer into a owning container.. but I also have to set a currently selected object in another class.. should I rather use a shared_ptr in this case and use a weak_ptr for where I set the currently selected object? Or should I use a raw pointer for the selected object and get the raw pointer out of the unique pointer like I do below?



#23 larspensjo   Members   -  Reputation: 1526

Like
0Likes
Like

Posted 20 June 2012 - 01:43 AM

But I have a case where I use a unique_ptr as its a factory function that creates the object.. I then move this pointer into a owning container.. but I also have to set a currently selected object in another class.. should I rather use a shared_ptr in this case and use a weak_ptr for where I set the currently selected object? Or should I use a raw pointer for the selected object and get the raw pointer out of the unique pointer like I do below?


Obviously, your code works, but the current use of smart pointers is an overhead with no added benefits. If it wouldn't have been for the need of secondary "selected object", then it would seem that vector should have been a vector of unique_ptr instead.

The question is, what is the wanted result of the design for this use case? We want (somewhat overlapping):
  • Flexibility that allows for change
  • Decreased chance that a change will lead to dangling pointers
  • Maintain efficiency

In this case you probably want to pass a shared_ptr to A and store it as either a shared_ptr or weak_ptr.


This would fulfil most requirements, except adding some overhead. That is, use shared_ptr for the vector and weak_ptr for the selected object. If it is not a benchmarked and proven performance critical section, I would not worry about the overhead. Start out with a "fool proof" implementation and worry about optimization later.

Adding smart pointers to source code retroactively is a little like adding "const". It will have rippling effects, spreading around, which can get out-of-hands.
Current project: Ephenation.
Sharing OpenGL experiences: http://ephenationopengl.blogspot.com/

#24 Zoomulator   Members   -  Reputation: 269

Like
1Likes
Like

Posted 20 June 2012 - 03:27 AM

Actually Stoustrup himself suggests to use smart pointers as "last resort", expecially shared_ptr .

It depends on what you mean with smart pointers. On the Going Native conference earlier this year they discussed the new smart pointers, that is both the unique_ptr and shared_ptr.

B. Stroustrup did say shared_ptr should be a last resort, but the unique_ptr should absolutely be used instead of any new/delete case. It was clearly stated that this should be the new way of handling heap objects in C++. "You should never have to call delete on your own."

Raw pointers still has its uses though, as also discussed at the conference, but they should always be perceived as non-owning pointers that you must guarantee never outlives the object it points to. This is usually the case when a child object has to point back to its parent, where the parent holds an owning pointer of the child. You could use a weak_ptr here, but it's pointless and creates unnecessary overhead. - The intention of this is however only clear if you always use the new smart pointers in the other cases, which is a small theoretical change in code style, but a very large practical change.

When ever I have a class that I put on heap, I usually put in a public typedef for a unique_ptr of that class. Simply MyClass::uptr_t, which honestly isn't that more straining to write that MyClass*. Or declare it outside the class as MyClass_uptr. The added syntax by unique_ptr<> is a poor excuse not to use them. Who can't say they should use typedef a bit more anyway?

Edited by Zoomulator, 20 June 2012 - 03:39 AM.


#25 kunos   Crossbones+   -  Reputation: 2184

Like
-2Likes
Like

Posted 20 June 2012 - 03:55 AM


Actually Stoustrup himself suggests to use smart pointers as "last resort", expecially shared_ptr .

It depends on what you mean with smart pointers. On the Going Native conference earlier this year they discussed the new smart pointers, that is both the unique_ptr and shared_ptr.

B. Stroustrup did say shared_ptr should be a last resort, but the unique_ptr should absolutely be used instead of any new/delete case. It was clearly stated that this should be the new way of handling heap objects in C++. "You should never have to call delete on your own."

Raw pointers still has its uses though, as also discussed at the conference, but they should always be perceived as non-owning pointers that you must guarantee never outlives the object it points to. This is usually the case when a child object has to point back to its parent, where the parent holds an owning pointer of the child. You could use a weak_ptr here, but it's pointless and creates unnecessary overhead. - The intention of this is however only clear if you always use the new smart pointers in the other cases, which is a small theoretical change in code style, but a very large practical change.

When ever I have a class that I put on heap, I usually put in a public typedef for a unique_ptr of that class. Simply MyClass::uptr_t, which honestly isn't that more straining to write that MyClass*. Or declare it outside the class as MyClass_uptr. The added syntax by unique_ptr<> is a poor excuse not to use them. Who can't say they should use typedef a bit more anyway?


I agree.. but, to me, they feel too much as an afterthought and totally extraneous to the language. If the committee REALLY feels like unique_ptr should take the place of new/delete why not implement it in the LANGUAGE.. just as Microsoft does with their "Node^" thing in .NET ? Node^ might then be converted to shared_ptr<Node> by the preprocessor.. and come up with something else for unique_ptr .. ResourceManager@ ? That will generate ugly stuff anyway.. const Node^ & node .. but still better than totally arbitrary typedefs.

Edited by kunos, 20 June 2012 - 03:58 AM.

Stefano Casillo
Lead Programmer
TWITTER: @KunosStefano
AssettoCorsa - netKar PRO - Kunos Simulazioni

#26 y2kiah   Members   -  Reputation: 900

Like
3Likes
Like

Posted 20 June 2012 - 08:04 AM

I agree.. but, to me, they feel too much as an afterthought and totally extraneous to the language. If the committee REALLY feels like unique_ptr should take the place of new/delete why not implement it in the LANGUAGE.. just as Microsoft does with their "Node^" thing in .NET ? Node^ might then be converted to shared_ptr<Node> by the preprocessor.. and come up with something else for unique_ptr .. ResourceManager@ ? That will generate ugly stuff anyway.. const Node^ & node .. but still better than totally arbitrary typedefs.

they are implemented in the language, and part of the Standard C++ library! How much stronger of a statement do you think they can make to you as a programmer, that smart pointers are important? If you're suggesting that these types become new primitive types in the language, no C++ doesn't work that way. Primitive types in C++ are not class types. You use std::string, std::vector, std::list, etc? Then you should not view std::unique_ptr any differently than any other standard type.
I always typedef my smart pointer types using a consistent convention. I then find it far less "ugly" than using raw pointers.

[source lang="cpp"]class Texture2D;typedef std::shared_ptr< Texture2D > Texture2DPtr;typedef std::weak_ptr< Texture2D > Texture2DWeakPtr;typedef std::unique_ptr< Texture2D > Texture2DUniquePtr;[left]// a little macro could take care of this for you, like _COM_SMARTPTR_TYPEDEF does for _com_ptr_t[/left]typedef std::vector< Texture2DPtr > Texture2DList;// etc...// you can't tell me this looks uglyclass Material {private: Texture2DList m_textures;public: void addTexture(const Texture2DPtr &t);};[/source]

Edited by y2kiah, 20 June 2012 - 08:07 AM.


#27 Zoomulator   Members   -  Reputation: 269

Like
1Likes
Like

Posted 20 June 2012 - 08:30 AM

If the committee really feels like unique_ptr should take the place of new/delete why not implement it in the language?

The main reason I see is because the std pointer templates are very generic implementations. They are made with the general case in mind, but far from every case. People will still find needs for new & delete. You may need your own memory manager and then you'll still need the "lower level" stuff. Having the template rather than a language feature also allows people to actually review the code for it, and perhaps build their own using the std version as a guide.

C++ fundamental types are all very simple structurally. A smart pointer with additional memory allocations, ref counting and destructors are miles away from a raw pointer which is only a memory address requiring no initialization or destruction, only assignment. I think this is one of the reason why it's simply a library implementation. C++ isn't a managed language and doesn't aim to be. It's the programmer that should make the choices without being too inclined by the language features.

Arbitrary typedef? Alright, say you've got an object which you've at first used a unique_ptr<MyObject> (or MyObject*, what ever). This works well for a while in your design. A while later you've come to use this plain object declaration in a lot of places, but now require to exchange it with a shared_ptr<> or ^. If you had just used a typedef MyObject_ptr it'd be a one line change instead of having to change every single line. This use for typedef goes for a lot of things, like container types. My point being, you should probably be using a typedef anyway so a preference for a symbol ^ instead of a template makes no difference.

[...] to me, they feel too much as an afterthought and totally extraneous to the language.

I would agree if it was a higher level language than C++. The code generated from the smart pointers is just as optimized as if they were a "primitive" type of the C++ language. Creating a smart pointer in a language that runs on a VM layer, then absolutely yes. Then you'd want the language to cut away the extra layer of the interpretation and just get down to business. But that's not C++.

Edited by Zoomulator, 20 June 2012 - 08:40 AM.


#28 MichaBen   Members   -  Reputation: 481

Like
0Likes
Like

Posted 20 June 2012 - 12:25 PM

they are implemented in the language, and part of the Standard C++ library! How much stronger of a statement do you think they can make to you as a programmer, that smart pointers are important?

Mutable and const_cast are part of the standard as well, doesn't mean those should be used all over the place as well. Personally I think in a well designed system, you don't really need smart pointers that much, as you won't be passing random pointers around. Thinks like file loaders, xml parsers and temporary blocks of memory for an operation are likely placed in the stack. They might internally alloacte memory, but it fully encapsulates it and frees all of it in the destructor, making it safe if an exception is thrown when you have an object of this on the stack.

#29 ChaosEngine   Crossbones+   -  Reputation: 2139

Like
5Likes
Like

Posted 20 June 2012 - 10:07 PM

they are implemented in the language, and part of the Standard C++ library! How much stronger of a statement do you think they can make to you as a programmer, that smart pointers are important?

Mutable and const_cast are part of the standard as well, doesn't mean those should be used all over the place as well. Personally I think in a well designed system, you don't really need smart pointers that much, as you won't be passing random pointers around. Thinks like file loaders, xml parsers and temporary blocks of memory for an operation are likely placed in the stack. They might internally alloacte memory, but it fully encapsulates it and frees all of it in the destructor, making it safe if an exception is thrown when you have an object of this on the stack.


Even assuming that's correct and realistic*, if you are writing a class that internally allocates memory, why wouldn't you use a unique_ptr to manage that internal allocation for you? If you can answer that with a better answer than "it's easy to write delete whatever in the destructor", then you probably know enough and have a specific enough use case to warrant such explicit memory management. If not, unique_ptr is a no brainer.

One of the major problems with C++ is that people spend more time dealing with language issues than domain issues. Things like smart pointers are an attempt to free the programmer from having to worry so much about these things and just get on with the job at hand.


* the real world is never that simple.
if you think programming is like sex, you probably haven't done much of either.-------------- - capn_midnight

#30 iMalc   Crossbones+   -  Reputation: 2259

Like
3Likes
Like

Posted 21 June 2012 - 01:22 AM

There is really no need for any debate here.
If you want to program in C++ then you have to practice using them until you understand what they do, forming an accurate mental model of what the various different types actually do.
Once you've done that, you will know when, where, why and how to use them; and until you've done that, you'll make mistakes that you can learn from.
"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

#31 Nanook   Members   -  Reputation: 470

Like
0Likes
Like

Posted 21 June 2012 - 04:54 PM

I have another issue with my resource manager that I want some comments on..

I have been using shared_ptr for this previously..

The options I have been thinking of are these:

1. Have a template<typename ResourceT> class ResourceManager; and have a function Hash register(const std::string& filename); on that class where I can register to a resource with the given filename. If the resource does not exist the register function will create the resource and store it on the stack in a unordered_map. I would also store a reference count and the user would need to call unregister when its done with the resource and it would be deleted when ref count is 0. To get the resource I would call ResourceT& GetResource(Hash resourceHash); I could then store raw pointers to the objects as they cannot be deleteded before I call the Unregister function. I like this approach as I try to follow the advice on keeping things on the stack mentioned in this thread..

2. I could have the same ResourceManager class as above, but I could store weak_ptr's in the unordered_map and when I do register it would check if theres a weak_ptr that I could get a valid shared_ptr to the object or it can create a new shared_ptr with the object, store a weak_ptr and then return the shared_ptr.. with this I dont have to worry about calling unregister and I can run a cleanup of the expired weak_ptr's whenever I want to..

Why should I choose one of these over the other, or even something else?

#32 Zoomulator   Members   -  Reputation: 269

Like
0Likes
Like

Posted 22 June 2012 - 04:47 AM

Personally, I'd use the weak pointer in map. I hate doing manual cleanup, like calling an "unregister" method. Having the first suggested setup, I'd probably wrap it into an object somehow that automatically called that method on destruction.

Another question is, would you really have to release a resource when nothing's using it? If it's not a huuuge game, you could load everything on startup and just let it sit there until shutdown. No reference counting, because you guarantee the lifetime of the resources for the entire program's lifetime. You wouldn't want something to reload a resource all the time in the middle of a game.

Edited by Zoomulator, 22 June 2012 - 04:49 AM.


#33 sundersoft   Members   -  Reputation: 216

Like
1Likes
Like

Posted 22 June 2012 - 08:26 PM

I have another issue with my resource manager that I want some comments on..

I have been using shared_ptr for this previously..

The options I have been thinking of are these:

1. Have a template<typename ResourceT> class ResourceManager; and have a function Hash register(const std::string& filename); on that class where I can register to a resource with the given filename. If the resource does not exist the register function will create the resource and store it on the stack in a unordered_map. I would also store a reference count and the user would need to call unregister when its done with the resource and it would be deleted when ref count is 0. To get the resource I would call ResourceT& GetResource(Hash resourceHash); I could then store raw pointers to the objects as they cannot be deleteded before I call the Unregister function. I like this approach as I try to follow the advice on keeping things on the stack mentioned in this thread..

2. I could have the same ResourceManager class as above, but I could store weak_ptr's in the unordered_map and when I do register it would check if theres a weak_ptr that I could get a valid shared_ptr to the object or it can create a new shared_ptr with the object, store a weak_ptr and then return the shared_ptr.. with this I dont have to worry about calling unregister and I can run a cleanup of the expired weak_ptr's whenever I want to..

Why should I choose one of these over the other, or even something else?


If you're going to have register and unregister functions in one of your classes, you should consider providing a helper RAII class which registers its argument in its constructor and unregisters it in the destructor. Anyway, if you're going to be using reference counting you might as well just use shared_ptr since the efficiency improvement of implementing it yourself is going to be negligable.

Anyway, the implementation you should use is going to depend on what kind of efficiency constraints you want and what exactly you're trying to do. For example, if you're making a game and you want to cache textures and models that were loaded from the disk, I'd recommend the following in your caching class:

-Have it return shared_ptrs on an object lookup. This allows you to not have to worry about whether the cached objects are in use or not when you flush the cache. I would also recommend storing shared_ptrs in the cache because, if you use weak_ptrs, then the object would become flushed from the cache as soon as the last shared_ptr reference to it was destroyed. This would be bad if part of your game involved loading a model (e.g. of a rocket), instancing it, and then destroying it later. Every time the model would be requested from the cache, it would have to be loaded from disk again.

-Have a "flush" function which goes through the cache and removes any object that only has one shared_ptr referring to it (i.e. the one stored by the cache). This removes all currently unused objects from the cache. It is not desireable to remove every object from the cache because this may cause two different instances of the same object to exist.

When you're performing an operation where most of the cache would be redundant (e.g. loading a new level), you would flush the cache. For loading a level, you would first delete the existing one, load and initialize the new one, and then flush the cache. This order would cause any resources that exist in both levels to not need to be loaded from disk twice.




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS