D12945: kcoredirlister lstItems benchmark
This revision was automatically updated to reflect the committed changes. Closed by commit R241:908bfca9fe72: kcoredirlister lstItems benchmark (authored by jtamate). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D12945?vs=38134=38612#toc REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12945?vs=38134=38612 REVISION DETAIL https://phabricator.kde.org/D12945 AFFECTED FILES autotests/CMakeLists.txt autotests/kcoredirlister_benchmark.cpp To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D12945: kcoredirlister lstItems benchmark
jtamate edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12945 To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D12945: kcoredirlister lstItems benchmark
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12945 To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D12945: kcoredirlister lstItems benchmark
jtamate updated this revision to Diff 38134. jtamate marked 6 inline comments as done. jtamate edited the summary of this revision. jtamate added a comment. Hopefully done all the requested changes. Passed uncristify-kf5. Removed the classes for simulating the filtering. Added benchmarks for 10, 100, 1000 and 1 items. The items are inserted in the same random order for all the implementations. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12945?vs=35585=38134 REVISION DETAIL https://phabricator.kde.org/D12945 AFFECTED FILES autotests/kcoredirlister_benchmark.cpp To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D12945: kcoredirlister lstItems benchmark
dfaure added a comment.
It's also interesting to look at numbers for small directories (which is
still the most common case, too).
This code could use a run of uncrustify to match the coding style of the rest
of kio, but otherwise it looks good.
INLINE COMMENTS
> kcoredirlister_benchmark.cpp:372
> +//BEGIN List
> +// List Implementation (without the NonMovable part)
> +class ListImplementation:public Base
In the long run, people will really wonder what's that about, since your other
commit will remove the NonMovableList class/hack ;)
> kcoredirlister_benchmark.cpp:679
> +data.f.setMimeFilter(QStringList("text/text"));
> +QUrl directoryUrl = QUrl::fromLocalFile("");
> +// Add every item in data.lstItems
fromLocalFile("") doesn't look valid to me
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D12945
To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D12945: kcoredirlister lstItems benchmark
jtamate updated this revision to Diff 35585. jtamate edited the summary of this revision. jtamate added a comment. Changed the structure QListBinaryHash to QMap Changed from KFileItems pointers to Values (it caused memory problems). Imported the parts that handle the filters to do the benchmarks of addNewItems. Assume KFileItems has < operands. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12945?vs=34368=35585 REVISION DETAIL https://phabricator.kde.org/D12945 AFFECTED FILES autotests/CMakeLists.txt autotests/kcoredirlister_benchmark.cpp To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D12945: kcoredirlister lstItems benchmark
jtamate added a dependency: D13211: Enable comparing KFileItems by url. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12945 To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D12945: kcoredirlister lstItems benchmark
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kcoredirlister_benchmark.cpp:244
> +uint hash=qHash(url);
> +auto it = std::lower_bound(lstItems.cbegin(), lstItems.cend(), hash,
> lessThanHash);
> +if (it != lstItems.cend() && it->item->url() == url) {
As discussed in https://phabricator.kde.org/D10742, this doesn't handle the
case of hash collisions (this code assumes each URL gets a unique hash). So
this commit needs to be updated as well, possibly by just removing this class
if making it right is too much effort, for too slow code in the end?
> kcoredirlister_benchmark.cpp:329
> +
> +template void createFiles(int number)
> +{
`number` is always 50 so you could remove it as a parameter in all these
methods and just use a static const int for instance.
It's more dangerous to have it as a method parameter: if one caller passes a
different number, the benchmarks won't be comparable anymore...
> kcoredirlister_benchmark.cpp:376
> +QBENCHMARK {
> +for (int i=0; i<5000; i++) {
> +data.findByUrl(u1);
QBENCHMARK takes care of repeating as many times as necessary, so this for loop
isn't needed, is it?
> kcoredirlister_benchmark.cpp:384
> +
> QUrl::fromLocalFile(QLatin1String("/home/user/Folder1/SubFolder2/a4505.txt")));
> +
> QCOMPARE(data.findByUrl(QUrl::fromLocalFile(QLatin1String("/home/user/Folder1/SubFolder2/a4505.txt")))->url(),
> +
> QUrl::fromLocalFile(QLatin1String("/home/user/Folder1/SubFolder2/a4505.txt")));
copy/paste errors? This is looking up the same URL three times.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D12945
To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D12945: kcoredirlister lstItems benchmark
jtamate added a dependent revision: D11282: less expensive findByUrl in KCoreDirListerCache. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12945 To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D12945: kcoredirlister lstItems benchmark
jtamate created this revision. jtamate added reviewers: dfaure, Frameworks. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. jtamate requested review of this revision. REVISION SUMMARY Decide which data structure is best for kcoredirlister lstItems. Defined as NonMovableFileItemList lstItems; in kcoredirlister_p.h (484). The results in one machine: | | QList | QListBinary | QListBinaryHash | QHash | | add | 17| 35 | 20 | 18| | findByName | 937 | 969 | 1.326 | 1.626 | | findByUrl| 1.953 | 66 | 7,6 | 7,2 | | findByUrlAll | 692 | 25 | 8,2 | 8,0 | | REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12945 AFFECTED FILES autotests/CMakeLists.txt autotests/kcoredirlister_benchmark.cpp To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
