Differential D19766

Improve performance of cliplugins
ClosedPublic
Actions

Authored by rthomsen on Mar 14 2019, 7:05 PM.

Details

Reviewers

Commits

R36:01289d698855: Improve performance of cliplugins

Summary

The CliPlugins are really slow when listing and extracting archives. This is due to several QRegularExpressions being matched on each line of output.

This diff removes all of the QRegularExpression matchings and uses string comparison instead (e.g. startsWith(), contains() and ==()). This necessitated moving the checks from the CliProperties class to the individual CliPlugins, because each plugin has different requirements to which string comparison methods should be used.

Some numbers for my system (6.gen. Core i7 with SSD). Listing linux kernel source archive:
clirar: Before 167 secs, after 7 secs
cli7z: Before 216 secs, after 37 secs
clizip: Before 15 secs, after 5 secs

The difference was less when extracting due to fewer regexp being matched, but still noticable.

Test Plan

All unit tests pass.

Password prompt is still detected.
Wrong password prompt is still detected.
Corrupt archive prompt is still detected.
Disk full prompt is still detected.
File exists prompt is still detected.

Diff Detail

Repository

R36 Ark

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

rthomsen created this revision.Mar 14 2019, 7:05 PM

Restricted Application added subscribers: Ark, kde-utils-devel. · View Herald TranscriptMar 14 2019, 7:05 PM

rthomsen requested review of this revision.Mar 14 2019, 7:05 PM

rthomsen added inline comments.

kerfuffle/cliinterface.cpp
928–931	I removed this, since we shouldnt check for "file exists" messages when listing, right?

elvisangelaccio added inline comments.Mar 17 2019, 3:39 PM

kerfuffle/cliinterface.cpp
928–931	Probably yes, but please do mention this change in the commit message.
964	Coding style: brace should open at the end of previous line
plugins/clizipplugin/cliplugin.cpp
118–119	Unrelated, but I just noticed that we never define this property, am I wrong?
118–119	What about this one? We are dropping the `diskFullPatterns` property, aren't we?

Implement Elvis' suggestions.

rthomsen marked 5 inline comments as done.Mar 18 2019, 5:50 PM

rthomsen added inline comments.

kerfuffle/cliinterface.cpp
928–931	Will do.
plugins/clizipplugin/cliplugin.cpp
118–119	Yes, the property is not used. I will delete separately.

dakon added a subscriber: dakon.Mar 18 2019, 8:09 PM

dakon added inline comments.

kerfuffle/cliinterface.cpp
781	constLast(), not sure if you really save a lot if you additionally do the conversion to QLatin1String only once, but since you need it at multiple places it's likely no overhead to do so anyway.
815	qAsConst to avoid detaching
964	range based for? Don't forget to put the list in a (const) variable to avoid that the iterators point to an already destroyed temporary.

rthomsen marked 2 inline comments as done.Mar 19 2019, 7:05 PM

rthomsen added inline comments.

kerfuffle/cliinterface.cpp
781	I would rather handle this separately, since it's not really related to this diff.
815	I didn't think this was necessary when using Qt's foreach loops on Qt container classes? AFAIK foreach prevents detaching...
964	We loop over a QList. Isn't a foreach loop better suited for this container class?

dakon added inline comments.Mar 19 2019, 10:58 PM

kerfuffle/cliinterface.cpp
815	Yes, you are right. Will become necessary if you switch to range based for loop ;)
964	Qt itself tries to get rid of them as much as possible, so I think they should possibly go away.

elvisangelaccio accepted this revision.Mar 20 2019, 8:10 PM

This revision is now accepted and ready to land.Mar 20 2019, 8:10 PM

Closed by commit R36:01289d698855: Improve performance of cliplugins (authored by rthomsen). · Explain WhyMar 20 2019, 8:16 PM

This revision was automatically updated to reflect the committed changes.

rthomsen mentioned this in R36:01289d698855: Improve performance of cliplugins.

Revision Contents
Changeset List

			Path	Packages
M			kerfuffle/cliinterface.h (6 lines)
M			kerfuffle/cliinterface.cpp (80 lines)
M			kerfuffle/cliproperties.h (20 lines)
M			kerfuffle/cliproperties.cpp (60 lines)
M			plugins/cli7zplugin/cliplugin.h (6 lines)
M			plugins/cli7zplugin/cliplugin.cpp (45 lines)
M			plugins/clirarplugin/cliplugin.h (6 lines)
M			plugins/clirarplugin/cliplugin.cpp (44 lines)
M			plugins/cliunarchiverplugin/cliplugin.h (1 line)
M			plugins/cliunarchiverplugin/cliplugin.cpp (9 lines)
M			plugins/clizipplugin/cliplugin.h (6 lines)
M			plugins/clizipplugin/cliplugin.cpp (43 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	53908			Mar 14 2019, 6:49 PM	★	★
Diff 2	54247			Mar 18 2019, 5:47 PM	★	★
Diff 3	54450	e446cf1	R36:01289d69885514c58071293bbcf4ef88dd9c5ef5	Mar 20 2019, 8:13 PM	★	★