D28528: UDSEntry: add constructor variant with std::initializer_list
kossebau abandoned this revision. kossebau added a comment. 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. INLINE COMMENTS > dfaure wrote in udsentry.h:132 > Why not simply QString*? How would you see `QString*` be used here? > dfaure wrote in udsentrybenchmark.cpp:141 > std::move(entry) if you want to skip the copying? 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? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28528 To: kossebau, #frameworks, dfaure, apol Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28528: UDSEntry: add constructor variant with std::initializer_list
dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > udsentry.h:132 > +const long long l; > +char s[sizeof(QString)]; > +} m_u; Why not simply QString*? > udsentrybenchmark.cpp:141 > +}; > +m_smallEntries.append(entry); > +} std::move(entry) if you want to skip the copying? > udsentrybenchmark.cpp:145 > + > +Q_ASSERT(m_smallEntries.count() == numberOfSmallUDSEntries); > +} QCOMPARE() REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28528 To: kossebau, #frameworks, dfaure, apol Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28528: UDSEntry: add constructor variant with std::initializer_list
kossebau updated this revision to Diff 79210. kossebau added a comment. Update to latest master REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28528?vs=79182&id=79210 BRANCH udsentryinitlistconstructor REVISION DETAIL https://phabricator.kde.org/D28528 AFFECTED FILES autotests/udsentrytest.cpp autotests/udsentrytest.h src/core/slavebase.cpp src/core/udsentry.cpp src/core/udsentry.h src/ioslaves/ftp/ftp.cpp src/ioslaves/http/http.cpp src/widgets/renamedialog.cpp tests/udsentrybenchmark.cpp To: kossebau, #frameworks, dfaure, apol Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28528: UDSEntry: add constructor variant with std::initializer_list
apol added a comment. +1 Makes sense, the code looks a bit fiddly but should allow cleaner code. INLINE COMMENTS > kossebau wrote in 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 :) Yeah you're right. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28528 To: kossebau, #frameworks, dfaure, apol Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28528: UDSEntry: add constructor variant with std::initializer_list
kossebau added inline comments. INLINE COMMENTS > apol wrote in 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? 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 :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28528 To: kossebau, #frameworks, dfaure, apol Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28528: UDSEntry: add constructor variant with std::initializer_list
apol added inline comments.
INLINE COMMENTS
> udsentry.h:127
> +// holds either the numeric or the string value
> +// done to avoid unneeded QString constructor & destructor in case of
> numeric value
> +union U {
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?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D28528
To: kossebau, #frameworks, dfaure, apol
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28528: UDSEntry: add constructor variant with std::initializer_list
kossebau created this revision. kossebau added reviewers: Frameworks, dfaure, apol. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. kossebau requested review of this revision. REVISION 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. REPOSITORY R241 KIO BRANCH udsentryinitlistconstructor REVISION DETAIL https://phabricator.kde.org/D28528 AFFECTED FILES autotests/udsentrytest.cpp autotests/udsentrytest.h src/core/slavebase.cpp src/core/udsentry.cpp src/core/udsentry.h src/ioslaves/ftp/ftp.cpp src/ioslaves/http/http.cpp src/widgets/renamedialog.cpp tests/udsentrybenchmark.cpp To: kossebau, #frameworks, dfaure, apol Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
