Hello, I have a new programmer on my team who has been brought on as an optimization engineer. She has a lot more experience than I do but I fear some of that experience may have left impressions that have stuck with her for too long. On the first check in labeled spot optimizations there were many things changed that disturbed me. One of them was changing a class from essentially. #include <boost/scoped_array.hpp> class Context { public: Context( unsigned int width, unsigned int height ) : m_buffer( new unsigned int[width*height] ) { } unsigned int* GetBuffer() { return m_buffer.get(); } private: boost::scoped_array<unsigned int> m_buffer; }; to class Context { public: Context( unsigned int width, unsigned int height ) : m_buffer( new unsigned int[width*height] ) { } ~Context() { delete m_buffer; } inline unsigned int* GetBuffer() { return m_buffer; } private: unsigned int* m_buffer; }; First of all there is the defect of delete being called instead of delete[]. Other than that I was under the impression that there should be no difference between the two other than the former being more descriptive (it instantly tells me this is an array and the memory is owned by the instance of this class). Is this really an optimization? Thanks, Michael Marcin
-----Original Message----- From: boost-users-bounces@lists.boost.org [mailto:boost-users-bounces@lists.boost.org] On Behalf Of Michael Marcin Sent: Wednesday, April 25, 2007 11:33 AM To: boost-users@lists.boost.org Subject: [Boost-users] [scoped_array] "optimization"
Hello,
I have a new programmer on my team who has been brought on as an optimization engineer. She has a lot more experience than I do but I fear some of that experience may have left impressions that have stuck with her for too long. On the first check in labeled spot optimizations there were many things changed that disturbed me. One of them was changing a class from essentially.
Besides the things you mention (the code being plain wrong!), I can't imagine there is any optimization going on here. The only optimizations worth doing are ones you can measure. Anything else is just guessing. That being said, you should ask why she felt that was an optimization. It could be at one point delete[] did something really stupid for primitive types for some compiler but delete still deallocated the memory. Who knows. The dark corners of our experiences we'd rather forget still make us who we are :-) Make sure she knows the difference between delete and delete[]. Anyway, none of this should be true for a modern compiler. Make sure she knows that too! Sohail
Michael Marcin wrote:
I have a new programmer on my team who has been brought on as an optimization engineer. She has a lot more experience than I do but I fear some of that experience may have left impressions that have stuck with her for too long. On the first check in labeled spot optimizations there were many things changed that disturbed me. One of them was changing a class from essentially.
#include <boost/scoped_array.hpp> class Context { public: Context( unsigned int width, unsigned int height ) : m_buffer( new unsigned int[width*height] ) { }
unsigned int* GetBuffer() { return m_buffer.get(); } private: boost::scoped_array<unsigned int> m_buffer; };
to
class Context { public: Context( unsigned int width, unsigned int height ) : m_buffer( new unsigned int[width*height] ) { } ~Context() { delete m_buffer; }
inline unsigned int* GetBuffer() { return m_buffer; } private: unsigned int* m_buffer; };
First of all there is the defect of delete being called instead of delete[]. Other than that I was under the impression that there should be no difference between the two other than the former being more descriptive (it instantly tells me this is an array and the memory is owned by the instance of this class).
Is this really an optimization?
Aside from the mistaken form of delete, your engineer is missing the whole point of scoped_array<>. For instance, what happens when a program copies a Context (which can happen easily in various expressions). The copy constructor will copy the pointer, and then you will have two Context objects that will both delete the same array. Someone had a good reason for using scoped_array. Don't throw it away due to someone's not understanding the reason(s). -- Dick Hadsell 914-259-6320 Fax: 914-259-6499 Reply-to: hadsell@blueskystudios.com Blue Sky Studios http://www.blueskystudios.com 44 South Broadway, White Plains, NY 10601
Hi Michael, Straight out of the docs http://www.boost.org/libs/smart_ptr/scoped_array.htm
Because scoped_array is so simple, in its usual implementation every operation is as fast as a built-in array pointer and it has no more space overhead that a built-in array pointer.
IMHO saying that the change to an array pointer is an "optimization" at all is dubious. -Dave On Wed, 2007-04-25 at 13:33 -0500, Michael Marcin wrote:
Hello,
I have a new programmer on my team who has been brought on as an optimization engineer. She has a lot more experience than I do but I fear some of that experience may have left impressions that have stuck with her for too long. On the first check in labeled spot optimizations there were many things changed that disturbed me. One of them was changing a class from essentially.
#include <boost/scoped_array.hpp> class Context { public: Context( unsigned int width, unsigned int height ) : m_buffer( new unsigned int[width*height] ) { }
unsigned int* GetBuffer() { return m_buffer.get(); } private: boost::scoped_array<unsigned int> m_buffer; };
to
class Context { public: Context( unsigned int width, unsigned int height ) : m_buffer( new unsigned int[width*height] ) { } ~Context() { delete m_buffer; }
inline unsigned int* GetBuffer() { return m_buffer; } private: unsigned int* m_buffer; };
First of all there is the defect of delete being called instead of delete[]. Other than that I was under the impression that there should be no difference between the two other than the former being more descriptive (it instantly tells me this is an array and the memory is owned by the instance of this class).
Is this really an optimization?
Thanks,
Michael Marcin
_______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
David Daeschler wrote:
Straight out of the docs http://www.boost.org/libs/smart_ptr/scoped_array.htm
Because scoped_array is so simple, in its usual implementation every operation is as fast as a built-in array pointer and it has no more space overhead that a built-in array pointer.
IMHO saying that the change to an array pointer is an "optimization" at all is dubious.
That's pretty much what I figured. Also I believe the added inline keyword is redundant as a function defined inside of a class declaration is the same as defining inline after the closing semi-colon of the class correct? I originally wrote that using a scoped_array to document the lifetime, ownership, and type of data pointed to. And so I didn't have to worry about things like forgetting to call delete[] instead of delete. I figured that any compiler worth using should inline this out to the same code as the "optimized" version but I have no proof of that I was hoping someone would have done simple tests comparing generated code that I could reference when I discuss these "improvements" with the team later this week. The copy constructor argument Richard Hadsell mentioned I hadn't even considered and it is a good argument in its own right. What are the likely results of calling operator delete instead of operator delete[] on an array of builtins allocated with operator new[]? Is it undefined behavior? Thanks, Michael Marcin
Michael Marcin wrote:
That's pretty much what I figured. Also I believe the added inline keyword is redundant as a function defined inside of a class declaration is the same as defining inline after the closing semi-colon of the class correct?
Your interpretation is correct, the inline specifier is redundant here.
The copy constructor argument Richard Hadsell mentioned I hadn't even considered and it is a good argument in its own right.
I would like to strengthen Richard's remarks: The newly proposed class violates the "rule of three", because it define's the d'tor, but forget's to implement copy c'tor and copy-assignment op (which is, as Richard already said, a bad idea here). Astonishingly for an expert...
What are the likely results of calling operator delete instead of operator delete[] on an array of builtins allocated with operator new[]? Is it undefined behavior?
It's UB, see 5.3.5/2: "[..]In the first alternative (delete object), the value of the operand of delete shall be a pointer to a non-array object or a pointer to a sub-object (1.8) representing a base class of such an object (clause 10). If not, the behavior is undefined. In the second alternative (delete array), the value of the operand of delete shall be the pointer value which resulted from a previous array new-expression. If not, the behavior is undefined.[..]" Greetings from Bremen, Daniel Krügler
Michael Marcin wrote:
That's pretty much what I figured. Also I believe the added inline keyword is redundant as a function defined inside of a class declaration is the same as defining inline after the closing semi-colon of the class correct?
Your interpretation is correct, the inline specifier is redundant here.
The copy constructor argument Richard Hadsell mentioned I hadn't even considered and it is a good argument in its own right.
I would like to strengthen Richard's remarks: The newly proposed class violates the "rule of three", because it define's the d'tor, but forget's to implement copy c'tor and copy-assignment op (which is, as Richard already said, a bad idea here). Astonishingly for an expert...
This really should be emphasized. Having a pointer to dynamically allocated buffer without custom copy constructor/copy assignment?Sooner or later somebody will try to copy it ... The "optimized" code is plainly wrong for the reasons other people already pointed out. I can't belive somebody starts optimizing code without first learning the basics of the language semantics and the libraries used. Leon
On Thu, April 26, 2007 10:09, Leon Mlakar wrote:
Michael Marcin wrote:
That's pretty much what I figured. Also I believe the added inline keyword is redundant as a function defined inside of a class declaration is the same as defining inline after the closing semi-colon of the class correct?
Your interpretation is correct, the inline specifier is redundant here.
The copy constructor argument Richard Hadsell mentioned I hadn't even considered and it is a good argument in its own right.
I would like to strengthen Richard's remarks: The newly proposed class violates the "rule of three", because it define's the d'tor, but forget's to implement copy c'tor and copy-assignment op (which is, as Richard already said, a bad idea here). Astonishingly for an expert...
This really should be emphasized. Having a pointer to dynamically allocated buffer without custom copy constructor/copy assignment?Sooner or later somebody will try to copy it ...
Please read the docs! There is clearly written that this class is derived from noncopyable (boost utility class!!!). The source does not contain the derivation but define private declarations of copy ctor and assignment operator. So there is no way to copy this class. Here some source from scoped_array.hpp take a look at the marked lines: // scoped_array extends scoped_ptr to arrays. Deletion of the array pointed to // is guaranteed, either on destruction of the scoped_array or via an explicit // reset(). Use shared_array or std::vector if your needs are more complex. template<class T> class scoped_array // noncopyable { private: T * ptr; scoped_array(scoped_array const &); // <<<===== private not implemented copy ctor scoped_array & operator=(scoped_array const &); // <<<===== private not implemented assignement operator [...] With Kind Regards, Ovanes Markarian
The copy constructor argument Richard Hadsell mentioned I hadn't even considered and it is a good argument in its own right.
I would like to strengthen Richard's remarks: The newly proposed class violates the "rule of three", because it define's the d'tor, but forget's to implement copy c'tor and copy-assignment op (which is, as Richard already said, a bad idea here).
Astonishingly for an
expert...
This really should be emphasized. Having a pointer to dynamically allocated buffer without custom copy constructor/copy assignment?Sooner or later somebody will try to copy it ...
Please read the docs! There is clearly written that this class is derived from noncopyable (boost utility class!!!).
The source does not contain the derivation but define private declarations of copy ctor and assignment operator. So there is no way to copy this class.
Not the original Context class, no. I was refering to the so called optimized version. I see no private copy ctor and assignment operator there. Leon
On Wed, 25 Apr 2007, Michael Marcin wrote: [...]
I originally wrote that using a scoped_array to document the lifetime, ownership, and type of data pointed to. And so I didn't have to worry about things like forgetting to call delete[] instead of delete. I figured that any compiler worth using should inline this out to the same code as the "optimized" version but I have no proof of that I was hoping someone would have done simple tests comparing generated code that I could reference when I discuss these "improvements" with the team later this week. [...]
I did a simple test (code below). By changing delete to delete[] and compiling as (using gcc-4.0.3) g++ -o test_n test.cpp -O3 and g++ -o test_b test.cpp -O3 and then disassembling the two binaries (objdump -d ...) and comparing the generated code, I could see that the generated code for the 'test' function was identical in both case. The code is of course a little bit different between delete and delete[]. #include <iostream> #ifdef USE_BOOST #include <boost/scoped_array.hpp> class Context { public: Context( unsigned int width, unsigned int height ) : m_buffer( new unsigned int[width*height] ) { } unsigned int* GetBuffer() { return m_buffer.get(); } private: boost::scoped_array<unsigned int> m_buffer; }; #else class Context { public: Context( unsigned int width, unsigned int height ) : m_buffer( new unsigned int[width*height] ) { } ~Context() { delete[] m_buffer; } inline unsigned int* GetBuffer() { return m_buffer; } private: unsigned int* m_buffer; }; #endif void test() { Context ctx( 20 , 20 ) ; ::std::cout << ctx.GetBuffer() ; } int main() { test() ; return 0 ; } -- François Duranleau LIGUM, Université de Montréal
On 4/25/07, Michael Marcin <mmarcin@method-solutions.com> wrote:
Hello,
I have a new programmer on my team who has been brought on as an optimization engineer. She has a lot more experience than I do but I fear some of that experience may have left impressions that have stuck with her for too long. On the first check in labeled spot optimizations there were many things changed that disturbed me.
Stop right there. Doesn't matter what the changes are, whether they are boost or whatever, etc. Just ask to see the before and after measurements. The optimizer's first *and last* tool should be a profiler. Find the bottlenecks and fix them, nothing else. Tony
participants (2)
-
Daniel Krügler -
David Daeschler -
François Duranleau -
Gottlob Frege -
Leon Mlakar -
Michael Marcin -
Ovanes Markarian -
Richard Hadsell -
Sohail Somani