Tobias Leupold
2018-05-18 04:58:33 UTC
Nice work :-) Don't forget to create a branch and push your changes from time
to time!
to time!
1) It misuses timers. Timers can only be started and stopped in the
same thread. Even putting them all in the same function is not
good enough, if that function can be called from different threads;
with the multi-threaded nature of thumbnail building, calling
ThumbnailCache::save() directly means it can be called from
multiple threads.
The practical impact is that the 1-second timer for thumbnail
saving accomplished nothing, because the timer was seldom getting
started (or stopped).
The solution is to never call save() directly; always call it
indirectly through emitting a signal.
2) The thumbnail cache itself is saved very inefficiently; it's
written in full to a temporary file, which is then copied (yes,
copied, not renamed) to the real file. Even with a big database,
such as my own (270,303 images) that's probably not killing me with
I/O, but it's a lot of work to be doing every 100 images.
I'm changing it to do an incremental save where possible. That
technically violates the intent of the file format, which stores
the most recently written file and offset as well as the image
count, but those can be reconstituted on cache reload easily
enough.
Also, in order to do that, I had to add an additional list of
unsaved files (auto-save list). And there's no good reason for the
data structure to be a QMap; a QHash is faster, and ordering
doesn't matter in the cache.
3) The save() routine wasn't protected by a mutex; with multiple
threads processing thumbnails, I'm not sure how we didn't get a lot
of data corruption in the index cache.
I'm working on all of these problems.
same thread. Even putting them all in the same function is not
good enough, if that function can be called from different threads;
with the multi-threaded nature of thumbnail building, calling
ThumbnailCache::save() directly means it can be called from
multiple threads.
The practical impact is that the 1-second timer for thumbnail
saving accomplished nothing, because the timer was seldom getting
started (or stopped).
The solution is to never call save() directly; always call it
indirectly through emitting a signal.
2) The thumbnail cache itself is saved very inefficiently; it's
written in full to a temporary file, which is then copied (yes,
copied, not renamed) to the real file. Even with a big database,
such as my own (270,303 images) that's probably not killing me with
I/O, but it's a lot of work to be doing every 100 images.
I'm changing it to do an incremental save where possible. That
technically violates the intent of the file format, which stores
the most recently written file and offset as well as the image
count, but those can be reconstituted on cache reload easily
enough.
Also, in order to do that, I had to add an additional list of
unsaved files (auto-save list). And there's no good reason for the
data structure to be a QMap; a QHash is faster, and ordering
doesn't matter in the cache.
3) The save() routine wasn't protected by a mutex; with multiple
threads processing thumbnails, I'm not sure how we didn't get a lot
of data corruption in the index cache.
I'm working on all of these problems.