[StatJob] Use A QFlag to specify the details returned by StatJob
ClosedPublic

Authored by meven on Oct 28 2019, 3:26 PM.

Details

Summary

Use this QFlag to minimize fields retrieved by statx, yielding a little memory and performance yield.

Test Plan

ctest

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
meven marked 2 inline comments as done.Nov 28 2019, 6:05 PM
meven added inline comments.
src/core/directorysizejob.cpp
140–143

The previous code did (details of 3)

meven marked 5 inline comments as done.Nov 28 2019, 6:06 PM
dfaure added inline comments.Nov 28 2019, 6:21 PM
src/core/directorysizejob.cpp
140–143

I'm confused by what you mean here... and I think I found more problems:

Indeed the old code used 3.
In your patch, you changed this to 2, in the legacy field for old kioslaves; any reason for that?
"3" adds UDS_DEVICE_ID/UDS_INODE which are used on lines 154/157 here. Surely we want to keep using 3.

And this means, in the new field statDetails, we want to specify KIO::Inode to get those fields.

meven updated this revision to Diff 70543.Nov 29 2019, 9:17 AM

Do not forget to get UDS_DEVICE_ID UDS_INODE

dfaure requested changes to this revision.Dec 1 2019, 1:20 PM
dfaure added inline comments.
src/core/directorysizejob.cpp
142

You want |, not & ...

This revision now requires changes to proceed.Dec 1 2019, 1:20 PM
meven updated this revision to Diff 70669.Dec 1 2019, 1:38 PM
meven marked an inline comment as done.

Fix: | instead of & to build a QFlags

dfaure accepted this revision.Dec 1 2019, 2:30 PM
This revision is now accepted and ready to land.Dec 1 2019, 2:30 PM
meven updated this revision to Diff 71701.Dec 17 2019, 6:27 AM

Rebase for kio 5.66

meven updated this revision to Diff 71702.Dec 17 2019, 6:28 AM

Fix Two references to KF 5,65

meven updated this revision to Diff 71703.Dec 17 2019, 6:30 AM

Fix Two references to KF 5.65

dfaure requested changes to this revision.Dec 18 2019, 9:40 AM
dfaure added inline comments.
src/core/directorysizejob.cpp
140–143

This still uses ENABLED instead of BUILD, see initial comment by kossebau.

This revision now requires changes to proceed.Dec 18 2019, 9:40 AM
meven updated this revision to Diff 71864.Dec 20 2019, 7:23 AM
meven marked 2 inline comments as done.

Replace use of KIOCORE_ENABLE_DEPRECATED_SINCE by KIOCORE_BUILD_DEPRECATED_SINCE in cpp files

dfaure accepted this revision.Dec 20 2019, 9:09 AM
This revision is now accepted and ready to land.Dec 20 2019, 9:09 AM
meven marked an inline comment as done.Dec 20 2019, 9:49 AM
meven planned changes to this revision.Dec 23 2019, 3:41 PM

I have tried adding unit tests.
It showed that the old:
*stat(const QUrl &url, KIO::StatJob::StatSide side, short int details, JobFlags flags = DefaultFlags);
Takes over the new :
KIO::stat(const QUrl &, KIO::StatSide, KIO::StatDetails int, JobFlags)")
Since KIO::StatDetails data type is int and enums are transparently converted to integer.
I will need to find a workarount or rename/change the signature of the new KIO::stat to make them apart

meven updated this revision to Diff 75304.Feb 9 2020, 3:26 PM

Fix implementation having a statDetails function to distinguish from the original function, fix and adapt test, in file_unix when using statx reduce the field retrieved for symlinks

This revision is now accepted and ready to land.Feb 9 2020, 3:26 PM
dfaure requested changes to this revision.Feb 9 2020, 7:03 PM
dfaure added inline comments.
src/core/statjob.cpp
97

When the declaration is in #if, the definition should be too (with s/ENABLED/BUILD/).

#if KIOCORE_BUILD_DEPRECATED_SINCE(5, 68)

src/core/statjob.h
79

I guess this overload exists because of the setDetails(short int) overload?
(QFlags has an implicit constructor from Enum so usually we don't need a single-flag overload)

If so, I suppose we can get rid of it for Qt6.
Or when KIOCORE_ENABLE_DEPRECATED_SINCE(5, 68) isn't set.
So basically it could move into an #else section of the #if below?

232

This was marked as done, but I still see ", KIO::StatDetails int,"

240

Still there

This revision now requires changes to proceed.Feb 9 2020, 7:03 PM
meven updated this revision to Diff 75566.Feb 12 2020, 5:45 PM
meven marked 3 inline comments as done.

Add a test for setDetails(KIO::StatDetails), fix comment, add a KIOCORE_ENABLE_DEPRECATED_SINCE

meven updated this revision to Diff 75567.Feb 12 2020, 5:49 PM
meven marked an inline comment as done.

Make setDetails(KIO::StatDetail detail) compile only when deprecated since 5.68 is enabled

meven updated this revision to Diff 75740.Feb 15 2020, 10:57 AM

Add an assertion to JobTest::stat()^Ccomment out other tests debug output

meven updated this revision to Diff 77235.Sun, Mar 8, 8:01 PM

Get code ready for 5.69, I really HOPE that the last time I have to do this, remove a few qDebug(), add some test assertions, make test more reliable

dfaure accepted this revision.Wed, Mar 11, 2:38 AM

Huh. Sorry about the delays and the need to update version numbers once more.

My last two comments are still not fixed. To see the issues, grep for "direcly" ('t' missing) and "Details int" (comma missing? or "int" should be removed?).
Or just land as is and I'll fix them in a followup commit, it'll be faster...

This revision is now accepted and ready to land.Wed, Mar 11, 2:38 AM
meven updated this revision to Diff 77385.Wed, Mar 11, 7:43 AM

Fix typo, make StatJob::setDetails(KIO::StatDetail detail) build only when build_deprecated(5.69) is set

meven updated this revision to Diff 77386.Wed, Mar 11, 7:48 AM

Improve comment of StatJob *statDetails

dfaure accepted this revision.Wed, Mar 11, 10:47 AM
meven added a comment.Wed, Mar 11, 5:51 PM

Ok to merge ?

Yep, that's what "Accepted" means ;)

This revision was automatically updated to reflect the committed changes.

This doesn't seem to compile:

[ 26%] Building CXX object src/ioslaves/file/CMakeFiles/kio_file.dir/file.cpp.o
cd /usr/src/kio-git/src/build/src/ioslaves/file && /usr/lib/ccache/bin/c++  -DKCOMPLETION_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054100 -DKCOREADDONS_LIB -DKF_DEPRECATED_WARNINGS_SINCE=0x060000 -DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054300 -DKIOCORE_DEPRECATED_WARNINGS_SINCE=0x0 -DKIOCORE_DISABLE_DEPRECATED_BEFORE_AND_AT=0x0 -DQT_CONCURRENT_LIB -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_DEPRECATED_WARNINGS_SINCE=0x060000 -DQT_DISABLE_DEPRECATED_BEFORE=0x050d00 -DQT_NETWORK_LIB -DQT_NO_CAST_FROM_ASCII -DQT_NO_CAST_FROM_BYTEARRAY -DQT_NO_CAST_TO_ASCII -DQT_NO_DEBUG -DQT_NO_NARROWING_CONVERSIONS_IN_CONNECT -DQT_NO_SIGNALS_SLOTS_KEYWORDS -DQT_NO_URL_CAST_FROM_STRING -DQT_STRICT_ITERATORS -DQT_USE_QSTRINGBUILDER -DTRANSLATION_DOMAIN=\"kio5\" -D_GNU_SOURCE -D_LARGEFILE64_SOURCE -Dkio_file_EXPORTS -I/home/al/.cache/yay/kio-git/src/build/src/ioslaves/file -I/home/al/.cache/yay/kio-git/src/kio/src/ioslaves/file -I/home/al/.cache/yay/kio-git/src/build/src/ioslaves/file/kio_file_autogen/include -I/home/al/.cache/yay/kio-git/src/build/src/core/.. -I/home/al/.cache/yay/kio-git/src/kio/src/core/kssl -I/home/al/.cache/yay/kio-git/src/build/src/core -I/home/al/.cache/yay/kio-git/src/kio/src/core -isystem /usr/include/KF5/KCoreAddons -isystem /usr/include/KF5-isystem /usr/include/qt -isystem /usr/include/qt/QtCore -isystem /usr/lib/qt/mkspecs/linux-g++ -isystem /usr/include/KF5/KService -isystem /usr/include/KF5/KConfigCore -isystem /usr/include/qt/QtNetwork -isystem /usr/include/qt/QtConcurrent -isystem /usr/include/qt/QtDBus -isystem /usr/include/KF5/KI18n -isystem /usr/include/KF5/KAuth  -march=x86-64 -mtune=native -O2 -pipe -fno-plt -g -fvar-tracking-assignments -fdebug-prefix-map=/home/al/.cache/yay/kio-git/src=/usr/src/debug -fno-operator-names -fno-exceptions -Wall -Wextra -Wcast-align -Wchar-subscripts -Wformat-security -Wno-long-long -Wpointer-arith -Wundef -Wnon-virtual-dtor -Woverloaded-virtual -Werror=return-type -Wvla -Wdate-time -Wsuggest-override -Wlogical-op -pedantic -Wzero-as-null-pointer-constant -O3 -DNDEBUG -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -fPIC -std=gnu++11 -o CMakeFiles/kio_file.dir/file.cpp.o -c /home/al/.cache/yay/kio-git/src/kio/src/ioslaves/file/file.cpp
/usr/src/kio-git/src/kio/src/ioslaves/file/file.cpp: In member function 'bool FileProtocol::createUDSEntry(const QString&, const QByteArray&, KIO::UDSEntry&, KIO::StatDetails)':
/usr/src/kio-git/src/kio/src/ioslaves/file/file.cpp:1041:38: error: 'linkTargetBuffer' was not declared in this scope
1041 |                         targetPath = linkTargetBuffer;
|                                      ^~~~~~~~~~~~~~~~
make[2]: *** [src/ioslaves/file/CMakeFiles/kio_file.dir/build.make:76: src/ioslaves/file/CMakeFiles/kio_file.dir/file.cpp.o] Error 1

"/compile/kde5/framework/kde/kdegames/kjumpingcube/game.cpp:582:63: erreur: impossible de convertir « KIO::StatJob::DestinationSide » de « KIO::StatJob::StatSide » vers « KIO::JobFlags » {aka « QFlags<KIO::JobFlag> »}

582 |          KIO::StatJob* statJob = KIO::stat(url, KIO::StatJob::DestinationSide, 0);
    |                                                 ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
    |                                                               |
    |                                                               KIO::StatJob::StatSide

/compile/kde5/framework/kde/kdegames/kjumpingcube/game.cpp: Dans la fonction membre « void Game::loadGame() »:
/compile/kde5/framework/kde/kdegames/kjumpingcube/game.cpp:627:60: erreur: impossible de convertir « KIO::StatJob::SourceSide » de « KIO::StatJob::StatSide » vers « KIO::JobFlags » {aka « QFlags<KIO::JobFlag> »}

627 |       KIO::StatJob* statJob = KIO::stat(url, KIO::StatJob::SourceSide, 0);
    |                                              ~~~~~~~~~~~~~~^~~~~~~~~~
    |                                                            |
    |                                                            KIO::StatJob::StatSide

"

compile is broken in several package see below.

@mlaurent compilation is broken because a method got deprecated, and you set -DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x060000 so the use of that method no longer compiles.
You can hardly blame other people for deprecating methods, it's a rather natural thing to do at this point...
On the contrary, I strongly suggest to stop setting -DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x060000, it just breaks too much. It even breaks stable branches.

I fixed that for kjumpingcube in https://commits.kde.org/kjumpingcube/5f238b424da1c57c56024a7e871579075bd06c73

In order to port away from deprecated methods, I suggest a script that increases the KF_DISABLE_DEPRECATED_BEFORE_AND_AT, which you (or anyone who wants to help with that) can run and fix compilation before pushing. It's what I regularly do for KF5 itself.

Did you test to compile with flags == 5.68.0 ?
I tested it with kdepim and I need to adapt it.
-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054400 is 5.68.0 ?

kossebau added inline comments.Thu, Mar 12, 10:21 PM
src/core/statjob.h
203

This wants to be "5, 69" now, not "5, 68"

kossebau added inline comments.Thu, Mar 12, 11:02 PM
src/core/statjob.h
203

Ah, was already fixed.

Did you test to compile with flags == 5.68.0 ?
I tested it with kdepim and I need to adapt it.
-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054400 is 5.68.0 ?

You also want to adapt kdepim-runtime it seems -> failing as part of dependency build e.g. here: https://build.kde.org/view/Failing/job/Administration/job/Dependency%20Build%20Extragear%20kf5-qt5%20SUSEQt5.12/lastFailedBuild/console (I just triggered product build, but missed that the latest "Applications" dep build might not yet have pulled in the new KIO with the respective deprecation).

meven marked 2 inline comments as done.Fri, Mar 13, 7:46 AM
meven added inline comments.
kossebau added inline comments.Mon, Mar 23, 4:27 AM
src/core/global.h
320

This injects generic terms like Basic, User, Time, Acl, etc. into the KIO namespace, with no futher hint that these belong to this very enum, resulting in potential wrong usages (due to completion-based coding when being convertable to int) or in potential conflicts with other future additions.

Sadly no time to follow the review. Had this been discussed before? Ideally those flags would get more explicit names, like BasicDetail (hm, what is basic actually), UserDetail, etc.

Could not find naming recommendations for current Qt, but here some old one, scroll to the section "Naming Enum Types and Values": https://doc.qt.io/archives/qq/qq13-apis.html#theartofnaming

meven marked 2 inline comments as done.Mon, Mar 23, 8:45 AM
meven added inline comments.
src/core/global.h
320

Yes it had been discussed and what you suggest was in a previous iteration : https://phabricator.kde.org/D25010?vs=69181&id=69385&whitespace=ignore-most#change-FL2qoDJhfEB5

Somehow I removed it :
https://phabricator.kde.org/D25010?vs=70051&id=70088&whitespace=ignore-most#change-FL2qoDJhfEB5

But I can't find why I did this, I guess it was a merging/synchronizing issue and lost those changes.
Have I already mentioned how much I dislike arcanist as I work on two machines, it is quite annoying.

Unless @dfaure you find a reason why I shouldn't do it, I will add those prefix back, since KF 5.69 is not yet out of the door, it is possible.

Yes, either prefixes or the C++11 way with enum class.

Yes, either prefixes or the C++11 way with enum class.

I propose to go with prefixes again, for consistency with currrent enum usages in KF/KIO APIs.

meven added a comment.Mon, Mar 23, 5:45 PM

Yes, either prefixes or the C++11 way with enum class.

I propose to go with prefixes again, for consistency with currrent enum usages in KF/KIO APIs.

I have a patch D28223