D12945: kcoredirlister lstItems benchmark

[David Faure]

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 <class T> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180525/92a82795/attachment.html>