View Issue Details

IDProjectCategoryView StatusLast Update
0015720ScribusScrapbookpublic2019-07-30 21:27
Reporterwilliam Assigned Tojghali  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Platformx86_64OSFedora LinuxOS Version30
Product Version1.5.4.svn 
Target VersionFixed in Version1.5.5.svn 
Summary0015720: Small improvement to scrapbook initialization
DescriptionWhen Scribus starts up, it checks for every vector and raster file extension in all of the scrapbook directories. For each directory, it checks once just to count, then a second time to read the files. `strace -e openat scribus` showed 496 opens of each scrapbook directory. Each open includes reading the directory with a filter for a single extension. This patch cuts the number of directory opens in half by remembering which extensions had matches, and only tries searching for those.
It might be even better to read the entire directory once with no filter and then make a list of files with interesting extensions, but that might increase memory requirements if a scrapbook directory had a huge number of files.
The patch also replaces a few instances of `QFileInfo fi(fileName); if (fi.exists()) ...` with `bool exists = QFile::exists(fileName); if (exists) ...` to use the static method instead of creating and populating a QFileInfo.
Steps To ReproduceRun scribus under strace
strace -e openat scribus > scribus.log 2>&1
sort < scribus.log | uniq -c | sort -r | head
TagsNo tags attached.
PatchYes

Activities

william

2019-06-19 23:57

updater  

scribus-scrapbookinit-20190620-004027.patch (3,751 bytes)
--- scribus/ui/scrapbookpalette.cpp-	2018-11-29 17:35:31.885662145 +0000
+++ scribus/ui/scrapbookpalette.cpp	2019-06-19 23:27:12.139433110 +0100
@@ -494,6 +494,8 @@
 	int fileCount = 0;
 	int readCount = 0;
 	QDir thumbs(name);
+	QSet<QString> vectorFound;
+	QSet<QString> rasterFound;
 	if (thumbs.exists())
 	{
 		if ((canWrite) && (PrefsManager::instance()->appPrefs.scrapbookPrefs.writePreviews))
@@ -510,6 +512,8 @@
 		QString ext = "*." + vectorFiles[v];
 		QDir d4(name, ext, QDir::Name, QDir::Files | QDir::Readable | QDir::NoSymLinks);
 		fileCount += d4.count();
+		if (d4.count() > 0)
+			vectorFound.insert(vectorFiles[v]);
 	}
 	QString formatD(FormatsManager::instance()->extensionListForFormat(FormatsManager::RASTORIMAGES, 1));
 	QStringList rasterFiles = formatD.split("|");
@@ -518,6 +522,8 @@
 		QString ext = "*." + rasterFiles[v];
 		QDir d5(name, ext, QDir::Name, QDir::Files | QDir::Readable | QDir::NoSymLinks);
 		fileCount += d5.count();
+		if (d5.count() > 0)
+			rasterFound.insert(rasterFiles[v]);
 	}
 	QProgressDialog *pgDia = nullptr;
 	QStringList previewFiles;
@@ -556,13 +562,13 @@
 			if (!loadRawText(QDir::cleanPath(QDir::toNativeSeparators(name + "/" + d[dc])), cf))
 				continue;
 			QFileInfo fi(QDir::cleanPath(QDir::toNativeSeparators(name + "/" + d[dc])));
-			QFileInfo fi2(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/.ScribusThumbs/"+fi.baseName()+".png")));
-			QFileInfo fi3(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/"+fi.baseName()+".png")));
-			if (fi2.exists())
+			bool pngExists = QFile::exists(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/.ScribusThumbs/"+fi.baseName()+".png")));
+			if (pngExists)
 				pm.load(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/.ScribusThumbs/"+fi.baseName()+".png")));
 			else
 			{
-				if (fi3.exists())
+				pngExists = QFile::exists(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/"+fi.baseName()+".png")));
+				if (pngExists)
 					pm.load(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/"+fi.baseName()+".png")));
 				else
 				{
@@ -584,6 +590,8 @@
 	}
 	for (int v = 0; v < vectorFiles.count(); v++)
 	{
+		if (!vectorFound.contains(vectorFiles[v]))
+			continue;
 		QString ext = "*." + vectorFiles[v];
 		QDir d4(name, ext, QDir::Name, QDir::Files | QDir::Readable | QDir::NoSymLinks);
 		if ((!d4.exists()) || (d4.count() <= 0))
@@ -597,8 +605,8 @@
 			}
 			QPixmap pm;
 			QFileInfo fi(QDir::cleanPath(QDir::toNativeSeparators(name + "/" + d4[dc])));
-			QFileInfo fi2(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/.ScribusThumbs/"+fi.fileName()+".png")));
-			if (fi2.exists())
+			bool pngExists = QFile::exists(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/.ScribusThumbs/"+fi.fileName()+".png")));
+			if (pngExists)
 				pm.load(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/.ScribusThumbs/"+fi.fileName()+".png")));
 			else
 			{
@@ -623,6 +631,8 @@
 	}
 	for (int v = 0; v < rasterFiles.count(); v++)
 	{
+		if (!rasterFound.contains(rasterFiles[v]))
+			continue;
 		QString ext = "*." + rasterFiles[v];
 		QDir d5(name, ext, QDir::Name, QDir::Files | QDir::Readable | QDir::NoSymLinks);
 		if ((!d5.exists()) || (d5.count() <= 0))
@@ -638,8 +648,8 @@
 				continue;
 			QPixmap pm;
 			QFileInfo fi(QDir::cleanPath(QDir::toNativeSeparators(name + "/" + d5[dc])));
-			QFileInfo fi2(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/.ScribusThumbs/"+fi.fileName()+".png")));
-			if (fi2.exists())
+			bool pngExists = QFile::exists(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/.ScribusThumbs/"+fi.fileName()+".png")));
+			if (pngExists)
 				pm.load(QDir::cleanPath(QDir::toNativeSeparators(fi.path()+"/.ScribusThumbs/"+fi.fileName()+".png")));
 			else
 			{

ale

2019-06-20 06:02

manager   ~0046333

what are "FormatsManager::RASTORIMAGES" ? : - )

ale

2019-06-20 06:09

manager   ~0046334

the scrapbook directories should only contain files that are readable by the scrapbook! (plus eventually some readme files...)

personally, i think that if it's much faster to first read all the files and then filtering them, and if the the memory difference is simply due to the lower or higher number of files read (and not about having a huge number of QFileInfo in memory...), then it should be fine to do that way...
... and the ones who have "garbage" in their scrapbook directories should clean them up (or write patches for scribus to add support for the files that are treated as garbage : - )

william

2019-06-20 16:16

updater   ~0046337

>what are "FormatsManager::RASTORIMAGES" ? : - )

It looks like a bitset of types that Scribus handles without plugins.

util_formats.h:47: RASTORIMAGES = 2|4|8|32|128|256|512|65536|524288|1048576|2097152|8388608, // All pure rastor image types

>i think that if it's much faster to first read all the files and then filtering them

I think so also, but I was worried that making the list could increase memory requirements if someone set a scrapbook directory with a huge number of files.

What I did with the QSet is twice as fast as the original code and won't do any damage in a worst case.

If you want, I can redo it to make a single scan of all files and then scan through the list instead of repeatedly calling QDir with a filter for each extension.
I would do it if it the change would be accepted.

jghali

2019-06-20 20:25

administrator   ~0046339

Patch committed! Thanks!

Issue History

Date Modified Username Field Change
2019-06-19 23:57 william New Issue
2019-06-19 23:57 william File Added: scribus-scrapbookinit-20190620-004027.patch
2019-06-20 06:02 ale Note Added: 0046333
2019-06-20 06:09 ale Note Added: 0046334
2019-06-20 16:16 william Note Added: 0046337
2019-06-20 20:25 jghali Assigned To => jghali
2019-06-20 20:25 jghali Status new => resolved
2019-06-20 20:25 jghali Resolution open => fixed
2019-06-20 20:25 jghali Fixed in Version => 1.5.5.svn
2019-06-20 20:25 jghali Note Added: 0046339
2019-06-20 20:25 jghali Summary [patch] Small improvement to scrapbook initialization => Small improvement to scrapbook initialization
2019-06-20 20:25 jghali Description Updated View Revisions
2019-06-20 20:25 jghali Steps to Reproduce Updated View Revisions
2019-07-30 21:27 cbradney Status resolved => closed