[smart_ptr] optimize current enable_shared_from_this with 100% compatibility

The attached patch optimizes the current implementation of enable_shared_from_this, it saves one plain pointer. That might not be very much, but it's a start. It is 100% compatible for the users, no features are dropped. Further improvements might be possible, but I think the attached patch is a good base. Peter, do you see any chance for it? If not, please tell me why and (if possible) what needs to be improved. Regards, Daniel

Daniel Frey:
Your patch (still) contains a number of changes, some of which are unrelated to its purpose (to save one pointer in esft). To break it down: 1. Changes esft.hpp to use the shared_count constructor to eliminate the circular initialization instead of sp_accept_owner overload( sp_deleter_wrapper* ). Stylistic change, not necessary, avoids testing a possible use of the sp_accept_owner mechanism. 2. Changes shared_count(weak_count) to not throw. Arbitrary change, not necessary, makes shared_count:weak_count inconsistent with shared_ptr:weak_ptr. 3. Adds a move constructor to shared_count. OK, not essential, but desirable. 4. Adds if( tmp != pi_ ) optimizations. OK, but a separate issue. 5. Adds shared_ptr(shared_count, p). OK, essential. The && overload is not guarded, BTW. 6. Adds _internal_shared_count. OK, essential. Should probably be called get_shared_count. You may commit #4 right away; as for the rest, I'd really like to see the patch without #1 and #2.

On Tue, 2008-04-22 at 01:54 +0300, Peter Dimov wrote:
Your patch (still) contains a number of changes, some of which are unrelated to its purpose (to save one pointer in esft). To break it down:
OK, so let's separate them. For now, I'll skip 1 and 2.
4. Adds if( tmp != pi_ ) optimizations. OK, but a separate issue.
Committed to svn.
5. Adds shared_ptr(shared_count, p). OK, essential. The && overload is not guarded, BTW.
I assume by "guarded" you mean "protected by #ifdef BOOST_HAS_RVALUE_REFS"? It actually is guarded then, although it might not be visible from looking at the patch itself, since I added it to the section with the other move semantic ctors.
6. Adds _internal_shared_count. OK, essential. Should probably be called get_shared_count.
Do you suggest to make it a part of the public interface? This would allow another, very explicit approach to have an aliasing ctor: shared_ptr<T> t = ...; U* pu = ...; shared_ptr<U> u( t.get_shared_count(), pu ); // aliasing Which /could/ replace the existing aliasing ctor - except that this is again a breaking change. But OK, that's independent of renaming it to get_shared_count.
You may commit #4 right away; as for the rest, I'd really like to see the patch without #1 and #2.
OK, I'll prepare it after work. Regards, Daniel

On Tue, 2008-04-22 at 01:54 +0300, Peter Dimov wrote:
as for the rest, I'd really like to see the patch without #1 and #2.
Please see the attached patch. OK to commit? Note that with this patch, the alternative aliasing is already possible: shared_ptr<X> x = ...; Y* y = ...; shared_ptr<Y>( x.get_shared_count(), y ); // aliasing Since the current aliasing ctor is not as explicit and it is not yet standardizes: What do you think about removing it? One might document get_shared_count as returning a const reference to an unspecified-internal-shared-counter-type, as well as documenting the new aliasing ctor(s) to accept this argument. That way, the interface is useful as a public interface, not just for internal optimizations. Regards, Daniel

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday 22 April 2008 15:06 pm, Peter Dimov wrote:
It seems fine to me. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFIDjuN5vihyNWuA4URAlM6AKCldnHQOZrC8/ekiDVA5tl5LTHp7ACg3xro +z5p5j0E7RaUHNfEmVFEmVc= =FtYR -----END PGP SIGNATURE-----

On Tue, 2008-04-22 at 01:54 +0300, Peter Dimov wrote:
I just noticed that by replacing _internal_shared_this with _internal_shared_count (and saving one plain pointer), the sp_accept_owner overload you mentioned is no longer called - there is simply no shared_ptr ctor involved, otherwise the optimization would not have been possible. Is it OK to remove the overload now? Regards, Daniel

On Monday 21 April 2008 18:18, Daniel Frey wrote:
I believe it would be possible to replace _internal_shared_this and the shared_ptr inside the deleter wrapper with a single shared_count. -- Frank

On Mon, 2008-04-21 at 20:14 -0400, Frank Mori Hess wrote:
That would be the next logical step, but it is not as easy as replacing the weak_ptr. Which is why I try to get as much of the other patches in before attacking this idea, it should make it easier once the patches to discuss become smaller. Regards, Daniel

On Tue, 2008-04-22 at 08:29 +0200, Daniel Frey wrote:
Now that the basic patch is committed, here's the next patch to replace the _internal_shared_this with _internal_shared_count to save another plain pointer. OK to commit? Regards, Daniel

On Tuesday 22 April 2008 17:18, Daniel Frey wrote:
It's okay by me. Although, I believe you could drop _internal_shared_count entirely and instead use the shared_ptr (or it could be a shared_count) in the deleter wrapper for the same purpose. And, if shared_from_this is not called prior to a shared_ptr taking ownership then no deleter wrapper will even be created. To be more specific, instead of storing _internal_shared_count in the enable_shared_from_this class, you store it in its own deleter, intentionally creating a circular reference that keeps it alive. When a shared_ptr takes ownership, the real owner is put into the deleter wrapper, breaking the circular reference. It would also require a slight modification to the deleter_wrapper class, something like a swap_deleter method, or you could make set_deleter return the old deleter. -- Frank

On Tue, 2008-04-22 at 19:48 -0400, Frank Mori Hess wrote:
Done.
I'll need some time to think about that. In the meantime, I noticed another improvement, see the attached patch. Looking at the comments just above init_internal_shared_once(), this seems to open up even more optimization possibilities. Since I think you wrote it and I don't know the internals as well as you, maybe you could elaborate on the comment and what could be improved given that the pointer inside of the deleter is not used anyway... Regards, Daniel

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Wednesday 23 April 2008 07:14 am, Daniel Frey wrote:
The comment is obsolete, now that the pointer value is calculated in the shared_from_this() calls. Yes, the deleter_wrapper only needs a shared_count. And what I was getting at before, _internal_shared_count could be made a local variable in init_internal_shared_once(), if you add a line where you stuff it inside its own deleter_wrapper. Anywhere else it is used, it can be obtained from _internal_weak_count. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFIDy9V5vihyNWuA4URAuhBAJ0bCdrchMkzigM3c+vssgbtPQDLOwCghWhv jwYg/eSswNaYltXeBFX47js= =Y2UL -----END PGP SIGNATURE-----

On Wed, 2008-04-23 at 08:45 -0400, Frank Mori Hess wrote:
The comment suggest that the lazy initialization is just a work-around for the more desirable initialization during the ctor. Is this no longer true?
I think we should pick the low-hanging fruits first. If it's OK for you I'll apply the attached patch. Next thing I'd like to address is getting rid of the _owner flag, I think it should be possible. We are then left with an overhead of the vtable and two plain pointers (inside of shared_count and weak_count). For the vtable, we could remove it by switching shared_from_this from a member function to a free function that takes T* as a parameter, but that's of course a breaking change... Regards, Daniel

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Wednesday 23 April 2008 14:33 pm, Daniel Frey wrote:
If you initialize in the constructor, you have to always create a deleter_wrapper. Currently, one is only created if you need it. I'd guess you would find the current behavior preferrable.
It's fine.
Didn't you propose something before like implementing the current esft interface as a derived class from a vtable-free base class that uses the free function approach? The last thing I saw from Peter about that was it needed to maintain compatibility. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFID4uI5vihyNWuA4URAmToAKCUAc6X+P6FCXhPMYiDEQFgjDGE3gCcDYaS keDGK/mqzefRjIvzOqwwbNU= =qpxu -----END PGP SIGNATURE-----

On Wed, 2008-04-23 at 15:18 -0400, Frank Mori Hess wrote:
OK, thanks for clarifying.
It's fine.
OK, committed.
Peter raised a valid concern about compatibility with TR1 and C++0x. I pointed out that C++0x is not yet standardized, so this is our chance to improve the interface. std::tr1::esft<T> could be derived from std::esft (note, no template). Likewise for boost::tr1::esft<T> and boost::esft. Of course the TR1 implementation needs an additional T* (in my optimized 1.35.0 esft version) or a vtable (in your trunk version), so only the non-TR1-version would benefit. As it has already been pointed out that the implementation should not drive the interface, I'd like to point out that the interface should not *force* some overhead without a good reason (IMHO). I attached an (untested) wrapper for tr1, just in case someone is interested in seeing some code... Regards, Daniel

On Wednesday 23 April 2008 16:44, Daniel Frey wrote:
It isn't my decision, but FWIW I prefer the template-less/free function esft interface too. It's slightly more elegant to have the template type deduced by a free shared_from_this call, than to make the user specify it explicitly as the template parameter to the esft class. -- Frank
participants (4)
-
Daniel Frey
-
Frank Mori Hess
-
Frank Mori Hess
-
Peter Dimov