This reads strange... What is the purpose of save() followed by load()? It sounds like reloading what we just saved, i.e. the load being a no-op. I'm guessing this isn't the case, but then it's worth a comment or renaming....

Why is this instance a global variable? It looks like it's only used in one method.

If locate() can't find the file, it will return an empty string, which will lead to

• a runtime warning from QFile about the filename being empty
• unusable debug output only showing empty strings

I recommend splitting the locate() and the QFile ctor -- and then you can remove the if !exists check, locate already checked that for you.
Instead, check for isEmpty on the string, and possibly add the relative path (/messagelist/threading/%1) to the debug output.

This warning should also display the path to the file, so people can find out which dir they messed up permissions on (e.g.)

This isn't byte-order-independent or architecture-independent (32/64 bits), unlike QDataStream which you use for the rest. Not sure it's real problem though; in the unlikely event that someone copies his home dir from a 32 bit computer to a 64 bit computer, he should hit your "unknown version" code path, hopefully ;-). But maybe using QDataStream would be safer and more consistent?

s/Failed/Unusable cache/. Failed in debug output sounds like a bug that has to be investigated, while this is a normal condition here.

The QFile destructor closes the file, so this line is unnecessary.

Should this use QSaveFile to avoid partially written out files in case of full partition? On the other hand I suppose not using QSaveFile is slightly faster so if we can handle truncated files gracefully....

Copy-paste mistake.

I added flush() after writing the CacheHeader, so if the file gets truncated, we will detect it while loading it the next time because we will reach EOF before reading CacheHeader::cacheSize records.

The in-memory cache gets dynamically updated when new emails arrive and are threaded by the model (or simply when you open a folder there's no cache for yet). When new folder is opened, Model::setStorageModel() is called and that's when we save() the in-memory cache of the old folder threading into a file, and then load() threading cache of the newly-selected folder.

I see. Can you add a "// save cache for old folder" comment, for the next dumb reader like me? :-)

Also.... what about the first time? Shouldn't save only be done if there *is* an old folder?

What if the user chose no threading, i.e. flat list?
Does the cache bring anything to the picture in that case, or just overhead?
AFAICS it's just overhead, writing out the full list of all items with a null parent into the cache. So I think the cache should be skipped if no threading is happening.

I use flat list in many folders, I'd like this opportunity while it's fresh in your head to ask if anything can be done to make that fast too :-)

Missing space before =

No qCDebug in this case? All other return paths have one...

Good point. I extended the logic a bit and completely disabled the cache for no-threading case.

I use flat list in many folders, I'd like this opportunity while it's fresh in your head to ask if anything can be done to make that fast too :-)

It basically boils down to the Model emitting beginInsertRows()/endInsertRows() for every single item. Disabling grouping should speed it up a little bit more. I'm looking into it now, I basically want to create some sort of insertion aggregator that would be able to insert items and emit beginInsertRows() in larger batches. However the code relies heavily on inserted items being immediately available in the internal tree (and the view as well, for auto-expanded threads), which makes this a tricky task

is there a newline after d here, or is phabricator on crack?

More generally this could say if (d->mThreadingCache.isEnabled() && !isReload)
This way the logic of when to enable/disable cache is in a single place (a bit below), and we never risk calling save() with the cache disabled. Alternatively you could add an early return in save if !mEnabled (which would also address my other comment).

If you go to a folder with threading, then to one without threading (-> saving old folder, and then mEnabled=false), and then you destroy the ThreadingCache, this will save again the cache for the old folder, right? I think this should test for mEnabled.