UDSEntry: add constructor variant with std::initializer_list
AbandonedPublic

Authored by kossebau on Apr 3 2020, 1:05 AM.

Details

Reviewers
dfaure
apol
Group Reviewers
Frameworks
Summary

For entries with fixed set of fields having an initializer list option comes
as nice syntactix sugar and spares the need of the explicit reserve() call,
and making sure to use the right size value there.

Due to fields having either a numeric value or a string one, this needs some
bit of C++ vodoo to avoid extra costs of having both a QString & a longlong
field per every entry, which would also mean a QString constructor &
destructor call also for each numeric field of the init list.
The UDSEntryBenchmark showed that those constructors & destructors add
notable cossts over explicit code to reserve & fill a UDSEntry, while using
a union with a QString mapped onto a char array gets numbers close.

Test Plan

New & old unit tests work, KIO usage e.g. in Dolphin with local filesystem
or FTP server works as before.

Diff Detail

Repository
R241 KIO
Branch
udsentryinitlistconstructor
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24669
Build 24687: arc lint + arc unit
kossebau created this revision.Apr 3 2020, 1:05 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 3 2020, 1:05 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Apr 3 2020, 1:05 AM
apol added inline comments.Apr 3 2020, 1:15 AM
src/core/udsentry.h
127

I'm not sure, it skips the constructor/destructor, but then when it's a string we need to do a memcopy. Are you sure this is faster than the refcounting?

kossebau added inline comments.Apr 3 2020, 1:20 AM
src/core/udsentry.h
127

Do you mean deep-copy by memcopy? There should be no such, the UDSEntryInitListEntry(uint field, const QString &value) constructor does a normal shared copy:

new(m_u.s) QString(value);

At least by what I understand and the test/experiments told me.

Still chances I missed something, being the first time I do such code, so tear apart where needed :)

apol added a comment.Apr 3 2020, 1:39 AM

+1 Makes sense, the code looks a bit fiddly but should allow cleaner code.

src/core/udsentry.h
127

Yeah you're right.

kossebau updated this revision to Diff 79210.Apr 3 2020, 1:19 PM

Update to latest master

dfaure requested changes to this revision.Apr 5 2020, 3:33 PM
dfaure added inline comments.
src/core/udsentry.h
132

Why not simply QString*?

tests/udsentrybenchmark.cpp
141

std::move(entry) if you want to skip the copying?

145

QCOMPARE()

This revision now requires changes to proceed.Apr 5 2020, 3:33 PM
kossebau abandoned this revision.Sun, May 24, 3:43 PM

As I am not sure about the risk introduced by the just-works-currently alignment in the union, I would rather discard this patch for now. Going from union to having both fields increases the runtime for what I measured, despite the reserve(), so also not eager to add that just for some syntactic sugar.
Happy to have somebody else pick this up though, just not continuing myself.

src/core/udsentry.h
132

How would you see QString* be used here?

tests/udsentrybenchmark.cpp
141

This test is a copy from UDSEntryBenchmark::createSmallEntries() just with different UDSEntry constructor so I did not question things :)
Though append() might be what the benchmark is about?