Bug 145293

Created attachment 20532 [details]
Patch to greatly speed up scanning for new images

Created attachment 20544 [details]
Patch to greatly speed up scanning for new images

Updated per review comments.

Just a reminder that this patch is still in the queue -- it makes a tremendous difference to startup speed.

OK, some comments:

a) it isn't common in KPA's sources to use void for specifying "no arguments"
b) "extensions" in RAWImageDecoder::_skipThisFile is a duplicate of stuff from RAWImageDecoder::_mightDecode. I think it should be merged somewhere... (as already suggested by Shawn Wilden on the ML)
c) using an array of QString doesn't look C++-ish to me. I see it's already done in KPA's sources, but I wonder why? Is it for performance reasons? Why don't use QStringList?
d) I'd use Set<QString> instead of QDict<void>
e) could you please sign the comment in FastDir with your name?
f) KPA uses private variable names prefixed by underscore, you use suffix (name_)
g) I'll try to make some Gentoo security dev look at the readdir calls

20:07 <+p-y> but since here you're using MAXNAMELEN, I think it's ok

So g) is solved.

a) I see.  I don't agree with this practice but I'll make the change to be consistent.

b) I agree, I'm working on factoring out the common bits.  It can't be entirely merged, but there's a lot that can be factored out.

c) I'm going to fix this up in the process of doing (b) in this place.  I'm not going to fix it elsewhere.

d) I agree, I'm using QDict<void> because the caller (and its caller) already uses QDict<void> for the list of loaded files and I'd prefer not to touch code outside of this change.

e) OK

f) OK

Created attachment 21223 [details]
Patch to greatly speed up scanning for new images

This addresses comments a-f above.

In addition, I've added some additional suffixes to the list to be ignored (by
the raw decoder, at any rate) unconditionally, including HTML/XML files and
compressed files.

SVN commit 691078 by jkt:

Speed up scan for new images by using readdir_r() instead of QDir. Patch by
Robert Krawitz, I've just made three RAWImageDecoder::_fileSOMETHING() functions
private.

BUG: 145293


 M  +67 -6     DB/NewImageFinder.cpp  
 M  +120 -35   ImageManager/RawImageDecoder.cpp  
 M  +13 -1     ImageManager/RawImageDecoder.h  


--- branches/extragear/kde3/graphics/kphotoalbum/DB/NewImageFinder.cpp #691077:691078
@@ -20,7 +20,9 @@
 #include <qfileinfo.h>
 #include "Settings/SettingsData.h"
 #include "Browser/BrowserWidget.h"
-#include <qdir.h>
+#include "ImageManager/RawImageDecoder.h"
+#include <sys/types.h>
+#include <dirent.h>
 #include "Utilities/Util.h"
 #include <qprogressdialog.h>
 #include <klocale.h>
@@ -56,6 +58,59 @@
     return (!_pendingLoad.isEmpty()); // returns if new images was found.
 }
 
+// FastDir is used in place of QDir because QDir stat()s every file in
+// the directory, even if we tell it not to restrict anything.  When
+// scanning for new images, we don't want to look at files we already
+// have in our database, and we also don't want to look at files whose
+// names indicate that we don't care about them.  So what we do is
+// simply read the names from the directory and let the higher layers
+// decide what to do with them.
+//
+// On my sample database with ~20,000 images, this improves the time
+// to rescan for images on a cold system from about 100 seconds to
+// about 3 seconds.
+//
+// -- Robert Krawitz, rlk@alum.mit.edu 2007-07-22
+class FastDir
+{
+public:
+    FastDir(const QString &path);
+    QStringList entryList() const;
+private:
+    const QString _path;
+};
+
+FastDir::FastDir(const QString &path)
+  : _path(path)
+{
+}
+
+QStringList FastDir::entryList() const
+{
+    QStringList answer;
+    DIR *dir;
+    dirent *file;
+    dir = opendir( QFile::encodeName(_path) );
+    if ( !dir )
+	return answer; // cannot read the directory
+
+#if defined(QT_THREAD_SUPPORT) && defined(_POSIX_THREAD_SAFE_FUNCTIONS) && !defined(Q_OS_CYGWIN)
+    union dirent_buf {
+	struct dirent mt_file;
+	char b[sizeof(struct dirent) + MAXNAMLEN + 1];
+    } *u = new union dirent_buf;
+    while ( readdir_r(dir, &(u->mt_file), &file ) == 0 && file )
+#else
+    while ( (file = readdir(dir)) )
+#endif // QT_THREAD_SUPPORT && _POSIX_THREAD_SAFE_FUNCTIONS
+	answer.append(QFile::decodeName(file->d_name));
+#if defined(QT_THREAD_SUPPORT) && defined(_POSIX_THREAD_SAFE_FUNCTIONS) && !defined(Q_OS_CYGWIN)
+    delete u;
+#endif
+    (void) closedir(dir);
+    return answer;
+}
+
 void NewImageFinder::searchForNewFiles( const QDict<void>& loadedFiles, QString directory )
 {
     if ( directory.endsWith( QString::fromLatin1("/") ) )
@@ -65,18 +120,24 @@
     if ( imageDir.endsWith( QString::fromLatin1("/") ) )
         imageDir = imageDir.mid( 0, imageDir.length()-1 );
 
-    QDir dir( directory );
-    QStringList dirList = dir.entryList( QDir::All );
+    FastDir dir( directory );
+    QStringList dirList = dir.entryList( );
+    ImageManager::RAWImageDecoder dec;
     for( QStringList::Iterator it = dirList.begin(); it != dirList.end(); ++it ) {
         QString file = directory + QString::fromLatin1("/") + *it;
-        QFileInfo fi( file );
         if ( (*it) == QString::fromLatin1(".") || (*it) == QString::fromLatin1("..") ||
              (*it) == QString::fromLatin1("ThumbNails") ||
              (*it) == QString::fromLatin1("CategoryImages") ||
-             !fi.isReadable() )
+	     loadedFiles.find( file ) ||
+	     dec._skipThisFile(loadedFiles, file) )
             continue;
 
-        if ( fi.isFile() && loadedFiles.find( file ) == 0) {
+        QFileInfo fi( file );
+
+	if ( !fi.isReadable() )
+	    continue;
+
+        if ( fi.isFile() ) {
             QString baseName = file.mid( imageDir.length()+1 );
             if ( ! DB::ImageDB::instance()->isBlocking( baseName ) ) {
                 if ( Utilities::canReadImage(file) )
--- branches/extragear/kde3/graphics/kphotoalbum/ImageManager/RawImageDecoder.cpp #691077:691078
@@ -21,6 +21,7 @@
 #include <qfile.h>
 #include <qimage.h>
 #include <qwmatrix.h>
+#include <qstringlist.h>
 #include "Settings/SettingsData.h"
 
 /* Main entry point into raw parser */
@@ -54,46 +55,130 @@
   return true;
 }
 
-bool RAWImageDecoder::_mightDecode( const QString& imageFile )
+QStringList RAWImageDecoder::_rawExtensions;
+QStringList RAWImageDecoder::_standardExtensions;
+QStringList RAWImageDecoder::_ignoredExtensions;
+
+void RAWImageDecoder::_initializeExtensionLists()
 {
-	/* Known RAW file extensions. TODO: Complete */
-	static const QString extensions[] = { QString::fromLatin1("crw"),
-										  QString::fromLatin1("cr2"),
-										  QString::fromLatin1("nef"),
-										  QString::fromLatin1("bay"),
-										  QString::fromLatin1("mos"),
-										  QString::fromLatin1("mrw"),
-										  QString::fromLatin1("orf"),
-										  QString::fromLatin1("cs1"),
-										  QString::fromLatin1("dc2"),
-										  QString::fromLatin1("kdc"),
-										  QString::fromLatin1("raf"),
-										  QString::fromLatin1("rdc"),
-										  QString::fromLatin1("x3f"),
-										  QString::fromLatin1("erf"),
-										  QString::fromLatin1("pef"),
-										  QString::null };
-	if (Settings::SettingsData::instance()->dontReadRawFilesWithOtherMatchingFile()) {
-            static const QString standardExtensions[] = {
-                QString::fromLatin1("jpg"),
-                QString::fromLatin1("JPG"),
-                QString::fromLatin1("tif"),
-                QString::fromLatin1("TIF"),
-                QString::fromLatin1("png"),
-                QString::fromLatin1("PNG"),
-                QString::null };
-            QString baseFileName = imageFile;
-            baseFileName.remove(baseFileName.length() - 3, 3);
-            for (int i = 0; !standardExtensions[i].isNull(); ++i) {
-                if (QFile::exists(baseFileName + standardExtensions[i]))
-                    return false;
-            }
+  static bool extensionListsInitialized = 0;
+  if (! extensionListsInitialized) {
+    /* Known RAW file extensions. TODO: Complete */
+    _rawExtensions << QString::fromLatin1("crw")
+		   << QString::fromLatin1("cr2")
+		   << QString::fromLatin1("nef")
+		   << QString::fromLatin1("bay")
+		   << QString::fromLatin1("mos")
+		   << QString::fromLatin1("mrw")
+		   << QString::fromLatin1("orf")
+		   << QString::fromLatin1("cs1")
+		   << QString::fromLatin1("dc2")
+		   << QString::fromLatin1("kdc")
+		   << QString::fromLatin1("raf")
+		   << QString::fromLatin1("rdc")
+		   << QString::fromLatin1("x3f")
+		   << QString::fromLatin1("erf")
+		   << QString::fromLatin1("pef");
+    _standardExtensions << QString::fromLatin1("jpg")
+			<< QString::fromLatin1("JPG")
+			<< QString::fromLatin1("jpeg")
+			<< QString::fromLatin1("JPEG")
+			<< QString::fromLatin1("tif")
+			<< QString::fromLatin1("TIF")
+			<< QString::fromLatin1("tiff")
+			<< QString::fromLatin1("TIFF")
+			<< QString::fromLatin1("png")
+			<< QString::fromLatin1("PNG");
+    _ignoredExtensions << QString::fromLatin1("thm") // Thumbnails
+		       << QString::fromLatin1("THM")
+		       << QString::fromLatin1("thumb") // thumbnail files
+						       // from dcraw
+		       << QString::fromLatin1("ctg") // Catalog files
+		       << QString::fromLatin1("gz") // Compressed files
+		       << QString::fromLatin1("Z")
+		       << QString::fromLatin1("bz2")
+		       << QString::fromLatin1("zip")
+		       << QString::fromLatin1("xml")
+		       << QString::fromLatin1("XML")
+		       << QString::fromLatin1("html")
+		       << QString::fromLatin1("HTML")
+		       << QString::fromLatin1("htm")
+		       << QString::fromLatin1("HTM");
+    extensionListsInitialized = 1;
+  }
+}
+
+bool RAWImageDecoder::_fileExistsWithExtensions( const QString& fileName,
+						const QStringList& extensionList) const
+{
+	QString baseFileName = fileName;
+	int extStart = fileName.findRev('.') + 1;
+	// We're interested in xxx.yyy, not .yyy
+	if (extStart <= 1) return false;
+	baseFileName.remove(extStart, baseFileName.length() - extStart);
+	for ( QStringList::ConstIterator it = extensionList.begin();
+	      it != extensionList.end(); ++it ) {
+	    if (QFile::exists(baseFileName + *it)) return true;
 	}
+	return false;
+}
 
-	for( int i = 0; !extensions[i].isNull(); ++i ) {
-		if( imageFile.endsWith( extensions[i], false ) ) return true;
+bool RAWImageDecoder::_fileIsKnownWithExtensions( const QDict<void>& files,
+						 const QString& fileName,
+						 const QStringList& extensionList) const
+{
+	QString baseFileName = fileName;
+	int extStart = fileName.findRev('.') + 1;
+	if (extStart <= 1) return false;
+	baseFileName.remove(extStart, baseFileName.length() - extStart);
+	for ( QStringList::ConstIterator it = extensionList.begin();
+	      it != extensionList.end(); ++it ) {
+	    if (files.find(baseFileName + *it) ) return true;
 	}
 	return false;
 }
 
+bool RAWImageDecoder::_fileEndsWithExtensions( const QString& fileName,
+					      const QStringList& extensionList) const
+{
+	for ( QStringList::ConstIterator it = extensionList.begin();
+	      it != extensionList.end(); ++it ) {
+	    if( fileName.endsWith( *it, false ) ) return true;
+	}
+	return false;
 }
+
+bool RAWImageDecoder::_mightDecode( const QString& imageFile )
+{
+	if (Settings::SettingsData::instance()->dontReadRawFilesWithOtherMatchingFile() &&
+	    _fileExistsWithExtensions(imageFile, _standardExtensions)) return false;
+	if (_fileEndsWithExtensions(imageFile, _rawExtensions)) return true;
+	return false;
+}
+
+bool RAWImageDecoder::_skipThisFile( const QDict<void>& loadedFiles, const QString& imageFile )
+{
+	// We're not interested in thumbnail and other files.
+	if (_fileEndsWithExtensions(imageFile, _ignoredExtensions)) return true;
+
+	// If we *are* interested in raw files even when other equivalent
+	// non-raw files are available, then we're interested in this file.
+	if (! (Settings::SettingsData::instance()->dontReadRawFilesWithOtherMatchingFile())) return false;
+
+	// If the file ends with something other than a known raw extension,
+	// we're interested in it.
+	if (! _fileEndsWithExtensions(imageFile, _rawExtensions)) return false;
+
+	// At this point, the file ends with a known raw extension, and we're
+	// not interested in raw files when other non-raw files are available.
+	// So search for an existing file with one of the standard
+	// extensions.
+	//
+	// This may not be the best way to do this, but it's using the
+	// same algorithm as _mightDecode above.
+	// -- Robert Krawitz rlk@alum.mit.edu 2007-07-22
+
+	return _fileIsKnownWithExtensions(loadedFiles, imageFile, _standardExtensions);
+}
+
+}
--- branches/extragear/kde3/graphics/kphotoalbum/ImageManager/RawImageDecoder.h #691077:691078
@@ -19,16 +19,28 @@
 #define RAWIMAGEDECODER_H
 
 #include "ImageDecoder.h"
+#include <qdict.h>
 
 namespace ImageManager
 {
 
 class RAWImageDecoder : public ImageDecoder {
 public:
-	RAWImageDecoder() {}
+	RAWImageDecoder() { _initializeExtensionLists(); }
 
 	virtual bool _decode(QImage *img, const QString& imageFile, QSize* fullSize, int dim=-1);
 	virtual bool _mightDecode( const QString& imageFile );
+	virtual bool _skipThisFile( const QDict<void>& loadedFiles, const QString& imageFile );
+
+private:
+	bool _fileExistsWithExtensions( const QString& fileName, const QStringList& extensionList ) const;
+	bool _fileEndsWithExtensions( const QString& fileName, const QStringList& extensionList ) const;
+	bool _fileIsKnownWithExtensions( const QDict<void>& files, const QString& fileName, const QStringList& extensionList ) const;
+
+	static QStringList _rawExtensions;
+	static QStringList _standardExtensions;
+	static QStringList _ignoredExtensions;
+	static void _initializeExtensionLists();
 };
 
 }