Version: 2.0 SVN (using KDE Devel) Installed from: Compiled sources OS: Linux Simplifications: no more lazy deletes, checking for main thread Speed-ups: refcounting is generally outside the store lock now; some methods became trivial and could be inlined easily.
Created attachment 19916 [details] patch to trunk
Ovy, I'm getting this linker error with your patch: CMakeFiles/amarok.dir/amarokcore/amarokdbushandler.o: In function `~AtomicString': /home/kdedev/kdesvn/extragear/multimedia/amarok/src/atomicstring.h:74: undefined reference to `AtomicString::deref(AtomicString::Data*)' /home/kdedev/kdesvn/extragear/multimedia/amarok/src/atomicstring.h:74: undefined reference to `AtomicString::deref(AtomicString::Data*)' /home/kdedev/kdesvn/extragear/multimedia/amarok/src/atomicstring.h:74: undefined reference to `AtomicString::deref(AtomicString::Data*)' /home/kdedev/kdesvn/extragear/multimedia/amarok/src/atomicstring.h:74: undefined reference to `AtomicString::deref(AtomicString::Data*)' /home/kdedev/kdesvn/extragear/multimedia/amarok/src/atomicstring.h:74: undefined reference to `AtomicString::deref(AtomicString::Data*)' CMakeFiles/amarok.dir/statusbar/statusbar.o:/home/kdedev/kdesvn/extragear/multimedia/amarok/src/atomicstring.h:74: more undefined references to `AtomicString::deref(AtomicString::Data*)' follow
I can't check it at the moment, but do we actually still need AtomicString in 2.0? iirc it's main use in 1.x is reducing the memory requirements for storing multiple identical strings in MetaBundlem, which we are going to remove. I can't check it at the moment, but do we actually still need AtomicString in 2.0? iirc it's main use in 1.x is reducing the memory requirements for storing multiple identical strings in MetaBundlem, which we are going to remove. <br>
Good point, Maximilian. I really wouldn't mind getting rid of AtomicString, if we can achieve the same memory savings with the new Meta classes.
Hi guys, I haven't followed the discussions on amarok-devel on the new meta classes, but presumably you'll need similar "unique string from disparate sources" functionality. The logic to do this with minimal locking is somewhat subtle and it took some trial and error to get it right (the unit test on smp is pretty unforgiving, btw), so perhaps you can reuse it. Regards, Ovy On 9 Mar 2007 10:59:38 -0000, Mark Kretschmann <markey@web.de> wrote: [bugs.kde.org quoted mail]
Hi Mark, As for the compilation error, I've just done a svn up and full rebuild, and it still works. I have no other files changed from svn. Perhaps you need to do a full rebuild? Or, this could happen if you have a change pending that includes atomicstring.h but doesn't link in the object. Regards, Ovy On 9 Mar 2007 10:40:13 -0000, Mark Kretschmann <markey@web.de> wrote: [bugs.kde.org quoted mail]
Hi Ovy, i'm not sure yet where we'd need AtomicString im Amarok 2.0 yet. the idea is to create one instance of our new meta classes for each artist/genre/... and then simply put a pointer to the relevant instance into the Meta::Track implementation. It would be great if you could take a look at them and fix any potential threading problems that you see (the sources are in amarok/src/collection) It is probably a good idea to apply your patch now while it still applies cleanly. We can still remove AtomicString later if we don't need it anymore.
Ok, will look. I think a similar index will be needed to find the artist/album object if one already exists, and it would be good to lock efficiently, at least in that core portion. I think a goal for 2.0 could be to make Amarok SMP-safe, without the need for the proc affinity stuff. I gave it a shot in 1.4, but QT was putting up too much resistance. It was beautifully responsive, it just didn't stay up for too long. Regards, Ovy On 9 Mar 2007 20:05:00 -0000, Maximilian Kossick <maximilian.kossick@googlemail.com> wrote: [bugs.kde.org quoted mail]
SVN commit 641052 by markey: Patch by Ovy: Simplify, speed up AtomicString using QT4 thread safe refcounting. Simplifications: no more lazy deletes, checking for main thread. Speed-ups: refcounting is generally outside the store lock now; some methods became trivial and could be inlined easily. @Ovy: moving the ~AtomicString() definition to the cpp file fixed the linker problem for me. BUG: 142721 M +4 -0 CMakeLists.txt M +0 -5 app.cpp M +39 -110 atomicstring.cpp M +13 -30 atomicstring.h --- trunk/extragear/multimedia/amarok/src/CMakeLists.txt #641051:641052 @@ -327,6 +327,10 @@ set_target_properties(amarok PROPERTIES VERSION 1.0.0 SOVERSION 1 ) install(TARGETS amarok DESTINATION ${LIB_INSTALL_DIR} ) +# uncomment these to stress-test AtomicString +#set(atomicstring_unittest_SRCS atomicstring_unittest.cpp atomicstring.cpp) +#kde4_add_executable(atomicstring_unittest ${atomicstring_unittest_SRCS}) +#target_link_libraries(atomicstring_unittest ${KDE4_KDECORE_LIBS} ${KDE4_KDE3SUPPORT_LIBS}) ########### next target ############### --- trunk/extragear/multimedia/amarok/src/app.cpp #641051:641052 @@ -181,11 +181,6 @@ new Amarok::DbusScriptHandler(); new Amarok::DbusDevicesHandler(); - // tell AtomicString that this is the GUI thread - if ( !AtomicString::isMainThread() ) - qWarning("AtomicString was initialized from a thread other than the GUI " - "thread. This could lead to memory leaks."); - #ifdef Q_WS_MAC appleEventProcessorUPP = AEEventHandlerUPP(appleEventProcessor); AEInstallEventHandler(kCoreEventClass, kAEReopenApplication, appleEventProcessorUPP, (long)this, true); --- trunk/extragear/multimedia/amarok/src/atomicstring.cpp #641051:641052 @@ -23,6 +23,8 @@ //Added by qt3to4: #include <pthread.h> +#include <qatomic.h> + #include "atomicstring.h" #if __GNUC__ >= 3 @@ -90,151 +92,78 @@ class AtomicString::Data: public QString { public: - mutable uint refcount; - Data(): refcount( 0 ) { } - Data( const QString &s ): QString( s ), refcount( 0 ) { } + QBasicAtomic refcount; + Data() { refcount.init( 0 ); } + Data( const QString &s ): QString( s ){ refcount.init( 0 ); } }; + + AtomicString::AtomicString(): m_string( 0 ) { } AtomicString::AtomicString( const AtomicString &other ) { - s_storeMutex.lock(); m_string = other.m_string; - ref( m_string ); - s_storeMutex.unlock(); + // No need to lock here: ref() is atomic, and it could not be deleted in between because + // at least one other ref exists (other's) + m_string->refcount.ref(); } AtomicString::AtomicString( const QString &string ): m_string( 0 ) { if( string.isEmpty() ) return; - - Data *s = new Data( string ); // note: s is a shallow copy + + Data *s = new Data( string ); s_storeMutex.lock(); - m_string = static_cast<Data*>( *( s_store.insert( s ).first ) ); - ref( m_string ); - uint rc = s->refcount; + m_string = static_cast<Data *>( *( s_store.insert( s ).first ) ); + // The atomic increment and test here is important to protect from the unlocked deref() + if( m_string->refcount.ref() == 1 && m_string != s ) { + // Well, it *is* about to be deleted! (see deref() ). In this case it's a bad idea + // to use it, we'll have a hard time synchronizing with the deleter. So we mark it + // removed (ref=-1) and replace it with our copy. This is safe because nobody but + // us and the deleter has a pointer to *m_string, and we're about to forget ours + m_string->refcount = -1; + s_store.erase( m_string ); + m_string = s; + s_store.insert( m_string ); + m_string->refcount.ref(); + } s_storeMutex.unlock(); - if ( !rc ) delete( s ); // already present + + if ( m_string != s ) delete( s ); } AtomicString::~AtomicString() { - s_storeMutex.lock(); deref( m_string ); - s_storeMutex.unlock(); } -QString AtomicString::string() const -{ - if ( !m_string ) return QString(); - // References to the stored string are only allowed to circulate in the main thread - return *m_string; -} - -bool AtomicString::isEmpty() const -{ - return !m_string; -} - -const QString *AtomicString::ptr() const -{ - if( !m_string ) { - Data *s = new Data( QString() ); // note: s is a shallow copy - s_storeMutex.lock(); - m_string = static_cast<Data*>( *( s_store.insert( s ).first ) ); - ref( m_string ); - uint rc = s->refcount; - s_storeMutex.unlock(); - if ( !rc ) delete( s ); // already present - } - - return m_string; -} - -uint AtomicString::refcount() const -{ - if ( m_string ) { - s_storeMutex.lock(); - uint rc = m_string->refcount; - s_storeMutex.unlock(); - return rc; - } - return 0; -} - AtomicString &AtomicString::operator=( const AtomicString &other ) { if( m_string == other.m_string ) return *this; - s_storeMutex.lock(); deref( m_string ); m_string = other.m_string; - ref( m_string ); - s_storeMutex.unlock(); + // no need for store lock: other is holding a ref + if( m_string ) m_string->refcount.ref(); return *this; } -bool AtomicString::operator==( const AtomicString &other ) const -{ - return m_string == other.m_string; -} - -bool AtomicString::operator!=( const AtomicString &other ) const -{ - return m_string != other.m_string; -} - -// needs to be called holding the lock +// call without holding the lock. if needed, it will acquire the lock itself inline void AtomicString::deref( Data *s ) { - checkLazyDeletes(); // a good time to do this - if( !s ) - return; - if( !( --s->refcount ) ) - { - s_store.erase( s ); - // only the main thread is allowed to delete stored strings - if ( isMainThread() ) - delete s; - else - s_lazyDeletes.append(s); + if ( !s ) return; + Data *old_s = s; + if ( s->refcount.deref() == 0 ) { + s_storeMutex.lock(); + // check again, in case an AtomicString(const QString &) was just happening + if( s->refcount == 0 ) s_store.erase( s ); + s_storeMutex.unlock(); + delete s; } } -// needs to be called holding the lock -inline void AtomicString::ref( Data *s ) const -{ - //checkLazyDeletes(); // a good time to do this - if( s ) - s->refcount++; -} - -// It is not necessary to hold the store mutex here. -bool AtomicString::isMainThread() -{ - // For isMainThread(), we could use QThread::currentThread(), except the - // docs say it's unreliable. And in general QThreads don't like to be called from - // app destructors. Good old pthreads will serve us well. As for Windows, these - // two calls surely have equivalents; better yet we'll have QT4 and thread safe - // QStrings by then. - // Note that the the static local init is thread safe. - static pthread_t main_thread = pthread_self(); - return pthread_equal(pthread_self(), main_thread); -} - -// call holding the store mutex -inline void AtomicString::checkLazyDeletes() -{ - // only the main thread is allowed to delete - if ( isMainThread() ) - { - s_lazyDeletes.setAutoDelete(true); - s_lazyDeletes.clear(); - } -} - AtomicString::set_type AtomicString::s_store; -Q3PtrList<QString> AtomicString::s_lazyDeletes; QMutex AtomicString::s_storeMutex; +QString AtomicString::s_emptyString; --- trunk/extragear/multimedia/amarok/src/atomicstring.h #641051:641052 @@ -27,16 +27,6 @@ * can hash 5 million 256 byte strings in 1.34s on a 1.62GHz Athlon XP.) For * other use, the overhead compared to a plain QString should be minimal. * - * Added note: due to QString's thread unsafe refcounting, special precautions have to be - * taken to avoid memory corruption, while still maintaining some level of efficiency. - * Also, deletions from other threads are delayed until that first thread - * calls AtomicString again. Thus, we would appear to leak memory if many AtomicStrings - * are deleted in a different thread than the main thread, and the main thread would - * never call AtomicString again. But this is unlikely since the GUI thread is the one - * manipulating AtomicStrings mostly. You can call the static method - * AtomicString::isMainString first thing in the app to make sure the GUI thread is - * identified correctly. This workaround can be removed with QT4. - * * @author Gábor Lehel <illissius@gmail.com> */ @@ -93,20 +83,23 @@ * This operation takes constant time. * @return whether this string and \p string are equivalent */ - bool operator==( const AtomicString &other ) const; + bool operator==( const AtomicString &other ) const { return m_string == other.m_string; } /** * This operation takes constant time. * @return whether this string and \p string are not equivalent */ - bool operator!=( const AtomicString &other ) const; + bool operator!=( const AtomicString &other ) const { return m_string != other.m_string; } /** - * Returns a reference to this string, avoiding copies if possible. + * Returns a reference to this string * * @return the string. */ - QString string() const; + const QString &string() const + { + return m_string ? *reinterpret_cast<QString*>(m_string) : s_emptyString; + } /** * Implicitly casts to a QString. @@ -119,7 +112,7 @@ * @return whether the string is empty * @see isNull */ - bool isEmpty() const; + bool isEmpty() const { return !m_string; } /** * For convenience. Equivalent to isEmpty(). @@ -134,16 +127,15 @@ * different ones. This can be useful for certain kinds of hacks, but * shouldn't normally be used. * - * Note: DO NOT COPY this pointer with QString() or QString=. It is not - * thread safe to do it (QString internal refcount) * @return the internal pointer to the string */ - const QString *ptr() const; + const QString *ptr() const { + return m_string ? reinterpret_cast<QString*>(m_string) : &s_emptyString; + } /** * For convenience, so you can do atomicstring->QStringfunction(), - * instead of atomicstring.string().QStringfunction(). The same warning - * applies as for the above ptr() function. + * instead of atomicstring.string().QStringfunction(). */ inline const QString *operator->() const { return ptr(); } @@ -153,14 +145,7 @@ */ uint refcount() const; - /** - * If called first thing in the app, this makes sure that AtomicString optimizes - * string usage for the main thread. - * @return true if this thread is considered the "main thread". - */ - static bool isMainThread(); - private: #if __GNUC__ >= 3 struct SuperFastHash; @@ -178,7 +163,6 @@ class Data; friend class Data; - void ref( Data* ) const; void deref( Data* ); static void checkLazyDeletes(); @@ -187,9 +171,8 @@ // static data static set_type s_store; // main string store - static Q3PtrList<QString> s_lazyDeletes; // strings scheduled for deletion - // by main thread static QMutex s_storeMutex; // protects the static data above + static QString s_emptyString; }; #endif
Ovy, I've just found a problem with your patch. With the patch applied, when I right-click the playlist header and try to add or remove a column, I'm getting the following crash: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread -1249270096 (LWP 22901)] AtomicString (this=0xbfdaad8c, other=@0xb74c281c) at /home/kdedev/qt-copy/include/QtCore/../../src/corelib/arch/qatomic_i386.h:70 70 : "memory"); (gdb) bt #0 AtomicString (this=0xbfdaad8c, other=@0xb74c281c) at /home/kdedev/qt-copy/include/QtCore/../../src/corelib/arch/qatomic_i386.h:70 #1 0xb72b29a9 in MetaBundle::prettyText (this=0xb74c280c, column=-1076187764) at /home/kdedev/kdesvn/extragear/multimedia/amarok/src/metabundle.h:495 #2 0xb740806a in TrackToolTip::setTrack (this=0xb74c27e0, tags=@0xb74c280c, force=true) at /home/kdedev/kdesvn/extragear/multimedia/amarok/src/tracktooltip.cpp:198 #3 0xb740a021 in TrackToolTip::slotUpdate (this=0xb74c27e0, url=@0xbfdab108) at /home/kdedev/kdesvn/extragear/multimedia/amarok/src/tracktooltip.cpp:316 #4 0xb740a168 in TrackToolTip::qt_metacall (this=0xb74c27e0, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0xbfdab178) at /home/kdedev/kdesvn/build/extragear/multimedia/amarok/src/tracktooltip.moc:77 #5 0xb7e86421 in QMetaObject::activate (sender=0x81661a0, from_signal_index=133, to_signal_index=133, argv=0x0) at kernel/qobject.cpp:2937 #6 0xb7e867f4 in QMetaObject::activate (sender=0x81661a0, m=0xb74ac4c4, local_signal_index=3, argv=0x0) at kernel/qobject.cpp:2983 #7 0xb72da177 in Playlist::columnsChanged (this=0x81661a0) at /home/kdedev/kdesvn/build/extragear/multimedia/amarok/src/playlist.moc:331 #8 0xb72dba9f in Playlist::columnResizeEvent (this=0x81661a0, col=7, oldw=0, neww=91) at /home/kdedev/kdesvn/extragear/multimedia/amarok/src/playlist.cpp:2718 #9 0xb72fa308 in Playlist::qt_metacall (this=0x81661a0, _c=QMetaObject::InvokeMetaMethod, _id=191, _a=0xbfdabbe4) at /home/kdedev/kdesvn/build/extragear/multimedia/amarok/src/playlist.moc:280 #10 0xb7e86421 in QMetaObject::activate (sender=0x816a248, from_signal_index=30, to_signal_index=30, argv=0xbfdabbe4) at kernel/qobject.cpp:2937 #11 0xb7e867f4 in QMetaObject::activate (sender=0x816a248, m=0xb60b410c, local_signal_index=3, argv=0xbfdabbe4) at kernel/qobject.cpp:2983 #12 0xb608a3c6 in Q3Header::sizeChange (this=0x816a248, _t1=7, _t2=0, _t3=91) at .moc/debug-shared/moc_q3header.cpp:169 #13 0xb5f21b45 in Q3ListView::adjustColumn (this=0x81661a0, col=7) at itemviews/q3listview.cpp:7927 #14 0xb72dac16 in Playlist::adjustColumn (this=0x81661a0, n=0) at /home/kdedev/kdesvn/extragear/multimedia/amarok/src/playlist.cpp:3546
Can you try changing atomicstring.cpp:110, inside the copy constructor, from: m_string->refcount.ref(); to if( m_string ) m_string->refcount.ref(); I'm guessing that's causing the issue, although I can't reproduce it now. Silly bug, and my test didn't cover nulls. Ovy On 10 Mar 2007 09:50:33 -0000, Mark Kretschmann <markey@web.de> wrote: [bugs.kde.org quoted mail]
SVN commit 641162 by markey: Re-apply Ovy's AtomicString patch, with crash fix. BUG: 142721 M +4 -0 CMakeLists.txt M +0 -5 app.cpp M +39 -110 atomicstring.cpp M +13 -30 atomicstring.h --- trunk/extragear/multimedia/amarok/src/CMakeLists.txt #641161:641162 @@ -327,6 +327,10 @@ set_target_properties(amarok PROPERTIES VERSION 1.0.0 SOVERSION 1 ) install(TARGETS amarok DESTINATION ${LIB_INSTALL_DIR} ) +# uncomment these to stress-test AtomicString +#set(atomicstring_unittest_SRCS atomicstring_unittest.cpp atomicstring.cpp) +#kde4_add_executable(atomicstring_unittest ${atomicstring_unittest_SRCS}) +#target_link_libraries(atomicstring_unittest ${KDE4_KDECORE_LIBS} ${KDE4_KDE3SUPPORT_LIBS}) ########### next target ############### --- trunk/extragear/multimedia/amarok/src/app.cpp #641161:641162 @@ -181,11 +181,6 @@ new Amarok::DbusScriptHandler(); new Amarok::DbusDevicesHandler(); - // tell AtomicString that this is the GUI thread - if ( !AtomicString::isMainThread() ) - qWarning("AtomicString was initialized from a thread other than the GUI " - "thread. This could lead to memory leaks."); - #ifdef Q_WS_MAC appleEventProcessorUPP = AEEventHandlerUPP(appleEventProcessor); AEInstallEventHandler(kCoreEventClass, kAEReopenApplication, appleEventProcessorUPP, (long)this, true); --- trunk/extragear/multimedia/amarok/src/atomicstring.cpp #641161:641162 @@ -23,6 +23,8 @@ //Added by qt3to4: #include <pthread.h> +#include <qatomic.h> + #include "atomicstring.h" #if __GNUC__ >= 3 @@ -90,151 +92,78 @@ class AtomicString::Data: public QString { public: - mutable uint refcount; - Data(): refcount( 0 ) { } - Data( const QString &s ): QString( s ), refcount( 0 ) { } + QBasicAtomic refcount; + Data() { refcount.init( 0 ); } + Data( const QString &s ): QString( s ){ refcount.init( 0 ); } }; + + AtomicString::AtomicString(): m_string( 0 ) { } AtomicString::AtomicString( const AtomicString &other ) { - s_storeMutex.lock(); m_string = other.m_string; - ref( m_string ); - s_storeMutex.unlock(); + // No need to lock here: ref() is atomic, and it could not be deleted in between because + // at least one other ref exists (other's) + if( m_string ) m_string->refcount.ref(); } AtomicString::AtomicString( const QString &string ): m_string( 0 ) { if( string.isEmpty() ) return; - - Data *s = new Data( string ); // note: s is a shallow copy + + Data *s = new Data( string ); s_storeMutex.lock(); - m_string = static_cast<Data*>( *( s_store.insert( s ).first ) ); - ref( m_string ); - uint rc = s->refcount; + m_string = static_cast<Data *>( *( s_store.insert( s ).first ) ); + // The atomic increment and test here is important to protect from the unlocked deref() + if( m_string->refcount.ref() == 1 && m_string != s ) { + // Well, it *is* about to be deleted! (see deref() ). In this case it's a bad idea + // to use it, we'll have a hard time synchronizing with the deleter. So we mark it + // removed (ref=-1) and replace it with our copy. This is safe because nobody but + // us and the deleter has a pointer to *m_string, and we're about to forget ours + m_string->refcount = -1; + s_store.erase( m_string ); + m_string = s; + s_store.insert( m_string ); + m_string->refcount.ref(); + } s_storeMutex.unlock(); - if ( !rc ) delete( s ); // already present + + if ( m_string != s ) delete( s ); } AtomicString::~AtomicString() { - s_storeMutex.lock(); deref( m_string ); - s_storeMutex.unlock(); } -QString AtomicString::string() const -{ - if ( !m_string ) return QString(); - // References to the stored string are only allowed to circulate in the main thread - return *m_string; -} - -bool AtomicString::isEmpty() const -{ - return !m_string; -} - -const QString *AtomicString::ptr() const -{ - if( !m_string ) { - Data *s = new Data( QString() ); // note: s is a shallow copy - s_storeMutex.lock(); - m_string = static_cast<Data*>( *( s_store.insert( s ).first ) ); - ref( m_string ); - uint rc = s->refcount; - s_storeMutex.unlock(); - if ( !rc ) delete( s ); // already present - } - - return m_string; -} - -uint AtomicString::refcount() const -{ - if ( m_string ) { - s_storeMutex.lock(); - uint rc = m_string->refcount; - s_storeMutex.unlock(); - return rc; - } - return 0; -} - AtomicString &AtomicString::operator=( const AtomicString &other ) { if( m_string == other.m_string ) return *this; - s_storeMutex.lock(); deref( m_string ); m_string = other.m_string; - ref( m_string ); - s_storeMutex.unlock(); + // no need for store lock: other is holding a ref + if( m_string ) m_string->refcount.ref(); return *this; } -bool AtomicString::operator==( const AtomicString &other ) const -{ - return m_string == other.m_string; -} - -bool AtomicString::operator!=( const AtomicString &other ) const -{ - return m_string != other.m_string; -} - -// needs to be called holding the lock +// call without holding the lock. if needed, it will acquire the lock itself inline void AtomicString::deref( Data *s ) { - checkLazyDeletes(); // a good time to do this - if( !s ) - return; - if( !( --s->refcount ) ) - { - s_store.erase( s ); - // only the main thread is allowed to delete stored strings - if ( isMainThread() ) - delete s; - else - s_lazyDeletes.append(s); + if ( !s ) return; + Data *old_s = s; + if ( s->refcount.deref() == 0 ) { + s_storeMutex.lock(); + // check again, in case an AtomicString(const QString &) was just happening + if( s->refcount == 0 ) s_store.erase( s ); + s_storeMutex.unlock(); + delete s; } } -// needs to be called holding the lock -inline void AtomicString::ref( Data *s ) const -{ - //checkLazyDeletes(); // a good time to do this - if( s ) - s->refcount++; -} - -// It is not necessary to hold the store mutex here. -bool AtomicString::isMainThread() -{ - // For isMainThread(), we could use QThread::currentThread(), except the - // docs say it's unreliable. And in general QThreads don't like to be called from - // app destructors. Good old pthreads will serve us well. As for Windows, these - // two calls surely have equivalents; better yet we'll have QT4 and thread safe - // QStrings by then. - // Note that the the static local init is thread safe. - static pthread_t main_thread = pthread_self(); - return pthread_equal(pthread_self(), main_thread); -} - -// call holding the store mutex -inline void AtomicString::checkLazyDeletes() -{ - // only the main thread is allowed to delete - if ( isMainThread() ) - { - s_lazyDeletes.setAutoDelete(true); - s_lazyDeletes.clear(); - } -} - AtomicString::set_type AtomicString::s_store; -Q3PtrList<QString> AtomicString::s_lazyDeletes; QMutex AtomicString::s_storeMutex; +QString AtomicString::s_emptyString; --- trunk/extragear/multimedia/amarok/src/atomicstring.h #641161:641162 @@ -27,16 +27,6 @@ * can hash 5 million 256 byte strings in 1.34s on a 1.62GHz Athlon XP.) For * other use, the overhead compared to a plain QString should be minimal. * - * Added note: due to QString's thread unsafe refcounting, special precautions have to be - * taken to avoid memory corruption, while still maintaining some level of efficiency. - * Also, deletions from other threads are delayed until that first thread - * calls AtomicString again. Thus, we would appear to leak memory if many AtomicStrings - * are deleted in a different thread than the main thread, and the main thread would - * never call AtomicString again. But this is unlikely since the GUI thread is the one - * manipulating AtomicStrings mostly. You can call the static method - * AtomicString::isMainString first thing in the app to make sure the GUI thread is - * identified correctly. This workaround can be removed with QT4. - * * @author Gábor Lehel <illissius@gmail.com> */ @@ -93,20 +83,23 @@ * This operation takes constant time. * @return whether this string and \p string are equivalent */ - bool operator==( const AtomicString &other ) const; + bool operator==( const AtomicString &other ) const { return m_string == other.m_string; } /** * This operation takes constant time. * @return whether this string and \p string are not equivalent */ - bool operator!=( const AtomicString &other ) const; + bool operator!=( const AtomicString &other ) const { return m_string != other.m_string; } /** - * Returns a reference to this string, avoiding copies if possible. + * Returns a reference to this string * * @return the string. */ - QString string() const; + const QString &string() const + { + return m_string ? *reinterpret_cast<QString*>(m_string) : s_emptyString; + } /** * Implicitly casts to a QString. @@ -119,7 +112,7 @@ * @return whether the string is empty * @see isNull */ - bool isEmpty() const; + bool isEmpty() const { return !m_string; } /** * For convenience. Equivalent to isEmpty(). @@ -134,16 +127,15 @@ * different ones. This can be useful for certain kinds of hacks, but * shouldn't normally be used. * - * Note: DO NOT COPY this pointer with QString() or QString=. It is not - * thread safe to do it (QString internal refcount) * @return the internal pointer to the string */ - const QString *ptr() const; + const QString *ptr() const { + return m_string ? reinterpret_cast<QString*>(m_string) : &s_emptyString; + } /** * For convenience, so you can do atomicstring->QStringfunction(), - * instead of atomicstring.string().QStringfunction(). The same warning - * applies as for the above ptr() function. + * instead of atomicstring.string().QStringfunction(). */ inline const QString *operator->() const { return ptr(); } @@ -153,14 +145,7 @@ */ uint refcount() const; - /** - * If called first thing in the app, this makes sure that AtomicString optimizes - * string usage for the main thread. - * @return true if this thread is considered the "main thread". - */ - static bool isMainThread(); - private: #if __GNUC__ >= 3 struct SuperFastHash; @@ -178,7 +163,6 @@ class Data; friend class Data; - void ref( Data* ) const; void deref( Data* ); static void checkLazyDeletes(); @@ -187,9 +171,8 @@ // static data static set_type s_store; // main string store - static Q3PtrList<QString> s_lazyDeletes; // strings scheduled for deletion - // by main thread static QMutex s_storeMutex; // protects the static data above + static QString s_emptyString; }; #endif