Details
- Reviewers
dfaure - Group Reviewers
Frameworks - Commits
- R241:b878cb30d36a: Use more UDSEntry::reserve() calls to avoid reallocs on multiple inserts
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.
+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.
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
@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.