Use more UDSEntry::reserve() calls to avoid reallocs on multiple inserts
ClosedPublic

Authored by kossebau on Apr 2 2020, 5:05 PM.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Apr 2 2020, 5:05 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 2 2020, 5:05 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Apr 2 2020, 5:05 PM

Typo in commit message "reallocson".

kossebau retitled this revision from Use more UDSEntry::reserve() calls to avoid reallocson multiple inserts to Use more UDSEntry::reserve() calls to avoid reallocs on multiple inserts.Apr 2 2020, 5:13 PM
apol added a subscriber: apol.Apr 2 2020, 5:27 PM

+1 Makes sense.

Would it be possible to have a constructor such as this?

const UDSEntry destUds = {
            { UDSEntry::UDS_NAME, d->dest.fileName()} ,
            { UDSEntry::UDS_MODIFICATION_TIME, mtimeDest.toMSecsSinceEpoch() / 1000} ,
            { UDSEntry::UDS_CREATION_TIME, ctimeDest.toMSecsSinceEpoch() / 1000} ,
            { UDSEntry::UDS_SIZE, sizeDest}
        };

It would allow for a cleaner text and no need to reserve explicitly.

kossebau added a comment.EditedApr 2 2020, 5:36 PM

Yes, I had also looked into adding a constructor with std::initializer_list for more code sugar, but found no way to deal with both QString & longlong where the overhead needed would be worth this. At least by the numbers the benchmark gave me
And there are only a few cases with a fixed set of inserts, most add stuff conditionally, so the few cases seemed not worth the effort and I deleted my approach.

Edit: well, not being a C++-magician surely someone might be able to come up with a better approach which also works out iin the benchmark. The helper value class I used for ::initializer_list had both a long long & QString member for every value, so there was a QString constructor & a destructor call for every value, even if the actual value is the long long one, which surely brings cost, with QString() not being a constexpr.
If one would be able to do true QString or longlong memory-union variant, with also only respective constructor/destructor calls being made, things might be worth it.

Looking at kio-extras, seems there are quite some more cases where the number of values inserted is fixed.

Okay, I will revive my std::initializer_list code again, in a separate patch, there are enough use-cases available, so at least the sugar code is nice to have, even without measurable performance changes. Perhaps one of the reviewers then will be able to help how to do efficient code which avoids the unneeded QString constructor/destructor calls.

Until then, the current patch should at least do some µ-improvement for people browsing lots of files via ftp & http :)

D28528 now up additionally, for those cases where a init list could be used instead.

Having challenged myself, I even found some pattern for doing a union with non-trivial constructors, which is a bit ugly, but makes one feel C++ expertish ;) Also learned during that that unions can have constructors (needed/used for init of the const long long)... that language keeps on an eternal student :P

kossebau added a comment.EditedApr 3 2020, 12:44 PM

@apol So okay if I land this one for now?
The initlist approach does not cover all cases anyway, and until that one is reviewed by enough experienced developers (e.g. who can tell if alignment of data works as hoped), this here should save resources across the globe already :) (even more as std::vector does not know about Q_DECLARE_TYPEINFO(KIO::UDSEntry, Q_MOVABLE_TYPE); and on each resize does a full copy of all items.
Edit: eh, seems I messed up in my testing, std::vector actually calls the move constructor on the Field class objects in the vector when resizing, so little less expensive over the copy constructor. Still not a simple memmove.

apol added a comment.Apr 3 2020, 1:05 PM

@apol So okay if I land this one for now?
The initlist approach does not cover all cases anyway, and until that one is reviewed by enough experienced developers (e.g. who can tell if alignment of data works as hoped), this here should save resources across the globe already :) (even more as std::vector does not know about Q_DECLARE_TYPEINFO(KIO::UDSEntry, Q_MOVABLE_TYPE); and on each resize does a full copy of all items.

sure, go for it.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 3 2020, 1:09 PM
This revision was automatically updated to reflect the committed changes.