Accelerate thumnailgenerator when loading JpegContent
ClosedPublic

Authored by tommo on May 17 2019, 5:42 PM.

Details

Summary

JpegContent::load() is extensively called by the ThumbnailGenerator. Its major purpose is to retrieve EXIF data from a file, if available also an embedded thumbnail. This change memory maps the file, rather than reading it all in using QFile::readAll(). This should also explain the heavy load pointed out in https://bugs.kde.org/show_bug.cgi?id=331435 .

BUG: 331435

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
tommo requested review of this revision.May 17 2019, 5:42 PM
tommo created this revision.
cfeck added a subscriber: cfeck.May 18 2019, 9:30 AM

Please handle possible nullptr result of QFile::map().

cfeck added a comment.May 18 2019, 9:32 AM

Also, please use MapPrivate.

tommo updated this revision to Diff 58274.May 19 2019, 3:49 AM

Use private mapping and handle nullptr.

ngraham requested changes to this revision.May 20 2019, 7:25 PM
ngraham added a subscriber: ngraham.

Thanks very much! However this patch makes the jpegcontenttest crash. Looks like it needs some adjustment to account for your changes.

$ grep -A 3 QFATAL ../../build/gwenview/Testing/Temporary/LastTest.log 
QFATAL : JpegContentTest::testTransform() Received signal 7
         Function time: 2ms Total time: 28ms
FAIL!  : JpegContentTest::testTransform() Received a fatal error.
   Loc: [Unknown file(0)]

You can run the tests by entering your build dir and running ctest.

This revision now requires changes to proceed.May 20 2019, 7:25 PM
tommo updated this revision to Diff 58422.May 21 2019, 4:33 PM

Unit test fixed. Unfortunately I can't avoid QFile::readAll() entirely. When the same file is saved as the one already open through mem-mapping, we are in trouble when writing to that file: "It is unspecified whether modifications made to the file made after the mapping is created will be visible through the mapped memory." Apparently this might be the case and causes strange behavior when working with the input data later on. And I can't simply unmap the input data, as it may be needed for transformation. Thus postpone QFile::readAll() to JpegContent::save(). At least it speeds up the thumbnail generator.

ngraham accepted this revision.May 21 2019, 5:34 PM

Thanks very much!

This revision is now accepted and ready to land.May 21 2019, 5:34 PM
This revision was automatically updated to reflect the committed changes.
cfeck added a comment.May 21 2019, 5:38 PM

There are still code styling issues, but too late...

My bad, sorry. I should have done a closer code review. Fixed with 3a5cd96bfc92741e6fb6285b516934ffb76ae56f.