[PATCH] Speeding up kabc

[Simon Hausmann]

On Sun, Oct 05, 2003 at 11:10:22AM +0200, Reinhold Kainhofer wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Attached is a patch for libkabc, which turns the [] operator in the 
> VCard::lines(..) method from the non-const operator to the const operator. As 
> a consequence, the returned list - if empty - is not added to the map, which 
> is not a problem, as it is only used to generate the VCARD string or the 
> addressee object using iterators (but nowhere is the returned list modified).
> For adding lines we have the VCard::addLine method anyway.
> 
> This gives a tremendous loading speedup for all applications using libkabc. 
> E.g. I did some tests with korganizer (just starting it and then quitting). 
> Before the patch about 15% (!!) of the overall costs were spent in the 
> VCard::lines() method, after the patch that number goes down to 5%, so this 
> patch provides more than 10% overall speedup...

It appears to me that vcard.h is not part of KDE 3.1's public API,
which means a IMHO much nicer but binary incompatible fix could be
applied:

Making mLineMap a value and making the line methods constant, making
the code much simpler, giving less duplication and better
performance by using the const [] operators, as you found out.

Please review.

Simon
-------------- next part --------------
Index: vcard.cpp
===================================================================
RCS file: /home/kde/kdelibs/kabc/vcardparser/vcard.cpp,v
retrieving revision 1.6
diff -u -p -b -r1.6 vcard.cpp
--- vcard.cpp	20 May 2003 16:07:28 -0000	1.6
+++ vcard.cpp	5 Oct 2003 09:48:19 -0000
@@ -23,28 +23,16 @@
 using namespace KABC;
 
 VCard::VCard()
-  : mLineMap( 0 )
 {
 }
 
 VCard::VCard( const VCard& vcard )
-  : mLineMap( 0 )
 {
-  if ( vcard.mLineMap ) {
-    if ( !mLineMap )
-      mLineMap = new QMap<QString, QValueList<VCardLine> >;
-
-    *mLineMap = *(vcard.mLineMap);
-  } else {
-    delete mLineMap;
-    mLineMap = 0;
-  }
+  mLineMap = vcard.mLineMap;
 }
 
 VCard::~VCard()
 {
-  delete mLineMap;
-  mLineMap = 0;
 }
 
 VCard& VCard::operator=( const VCard& vcard )
@@ -52,63 +40,47 @@ VCard& VCard::operator=( const VCard& vc
   if ( &vcard == this )
     return *this;
 
-  if ( vcard.mLineMap ) {
-    if ( !mLineMap )
-      mLineMap = new QMap<QString, QValueList<VCardLine> >;
-
-    *mLineMap = *(vcard.mLineMap);
-  } else {
-    delete mLineMap;
-    mLineMap = 0;
-  }
+  mLineMap = vcard.mLineMap;
 
   return *this;
 }
 
 void VCard::clear()
 {
-  if ( mLineMap )
-    mLineMap->clear();
+  mLineMap.clear();
 }
 
 QStringList VCard::identifiers() const
 {
-  if ( !mLineMap )
-    return QStringList();
-  else
-    return mLineMap->keys();
+  return mLineMap.keys();
 }
 
 void VCard::addLine( const VCardLine& line )
 {
-  if ( !mLineMap )
-    mLineMap = new QMap<QString, QValueList<VCardLine> >;
-
-  (*mLineMap)[ line.identifier() ].append( line );
+  mLineMap[ line.identifier() ].append( line );
 }
 
-VCardLine::List VCard::lines( const QString& identifier )
+VCardLine::List VCard::lines( const QString& identifier ) const
 {
-  if ( !mLineMap )
+  LineMap::ConstIterator it = mLineMap.find( identifier );
+  if ( it == mLineMap.end() )
     return VCardLine::List();
-  else
-    return (*mLineMap)[ identifier ];
+
+  return *it;
 }
 
-VCardLine VCard::line( const QString& identifier )
+VCardLine VCard::line( const QString& identifier ) const
 {
-  if ( !mLineMap )
+  LineMap::ConstIterator it = mLineMap.find( identifier );
+  if ( it == mLineMap.end() )
     return VCardLine();
-  else
-    return (*mLineMap)[ identifier ][ 0 ];
+
+  return ( *it ).first();
 }
 
 void VCard::setVersion( Version version )
 {
-  if ( !mLineMap )
-    mLineMap = new QMap<QString, QValueList<VCardLine> >;
-  else
-    mLineMap->erase( "VERSION" );
+  mLineMap.erase( "VERSION" );
 
   VCardLine line;
   line.setIdentifier( "VERSION" );
@@ -117,15 +89,16 @@ void VCard::setVersion( Version version 
   if ( version == v3_0 )
     line.setIdentifier( "3.0" );
 
-  (*mLineMap)[ "VERSION" ].append( line );
+  mLineMap[ "VERSION" ].append( line );
 }
 
 VCard::Version VCard::version() const
 {
-  if ( !mLineMap )
+  LineMap::ConstIterator versionEntry = mLineMap.find( "VERSION" );
+  if ( versionEntry == mLineMap.end() )
     return v3_0;
 
-  VCardLine line = (*mLineMap)[ "VERSION" ][ 0 ];
+  VCardLine line = ( *versionEntry )[ 0 ];
   if ( line.value() == "2.1" )
     return v2_1;
   else
Index: vcard.h
===================================================================
RCS file: /home/kde/kdelibs/kabc/vcardparser/vcard.h,v
retrieving revision 1.7
diff -u -p -b -r1.7 vcard.h
--- vcard.h	25 Sep 2003 19:27:14 -0000	1.7
+++ vcard.h	5 Oct 2003 09:48:19 -0000
@@ -32,6 +32,7 @@ class VCard
 {
   public:
     typedef QValueList<VCard> List;
+    typedef QMap< QString, VCardLine::List > LineMap;
 
     enum Version { v2_1, v3_0 };
 
@@ -61,12 +62,12 @@ class VCard
     /**
      * Returns all lines of the vcard with a special identifier.
      */
-    VCardLine::List lines( const QString& identifier );
+    VCardLine::List lines( const QString& identifier ) const;
 
     /**
      * Returns only the first line of the vcard with a special identifier.
      */
-    VCardLine line( const QString& identifier );
+    VCardLine line( const QString& identifier ) const;
 
     /**
      * Set the version of the vCard.
@@ -79,7 +80,7 @@ class VCard
     Version version() const;
 
   private:
-    QMap< QString, VCardLine::List > *mLineMap;
+    LineMap mLineMap;
 
     class VCardPrivate;
     VCardPrivate *d;