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

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

Details

Reviewers
dfaure
kossebau
Group Reviewers
Frameworks
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
Branch
arcpatch-D25010
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19010
Build 19028: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
kossebau added inline comments.Tue, Oct 29, 10:42 AM
src/core/statjob.h
51

Is there other KIO API which has similar enums, where some consistency would be good to have?

On a first look, the names seems very short & generic to me, having some other name element might avoid semantic collisions perhaps. No idea yet, not looked further.

51

Please add @since comment

93–94

Not deprecated now?

165

Does KIO::DefaultDetails make sense beyond stat in the KIO namespace? How does it match other similar flags?
Perhaps should be more bound to "stat" by the name.
Please also add documentation, incl. "@since"

193

Please add @since

199

Please wrap the deprecated API call (incl. API dox comment) with visibilty controlling #if/#endif., so here

#if KIOCORE_ENABLE_DEPRECATED_SINCE(5, 64)
224–225

Please add "@deprecated Since 5.64. Use KIO::stat(const QUrl &, KIO::StatJob::StatSide, StatJob::Details int, JobFlags)"

For all the recommended details to add when deprecating API (also with the new deprecation macros), please see
https://community.kde.org/Policies/Library_Code_Policy#Deprecation_of_API

230–241

#endif

meven updated this revision to Diff 68969.Tue, Oct 29, 12:03 PM
meven marked 8 inline comments as done.

Rename StatJob::Detail to StatJob::StatDetail, KIO::DefaultDetails to KIO::StatDefaultDetails, add deprecation doc and macro

kossebau added inline comments.Tue, Oct 29, 12:13 PM
src/core/statjob.h
82

All deprecated API should be also wrapped in visibility controls., so same #if/#endif also here.

Basic structure:

if deprecated api should be visible to compiler & Co (e.g. docs generator)
    API docs
    warning attribute if should be emitted 
    method declaration
endif
meven updated this revision to Diff 68971.Tue, Oct 29, 12:30 PM
meven marked an inline comment as done.

Wrap setDetails with #if KIOCORE_ENABLE_DEPRECATED_SINCE, use the new API throughout the rest of KIO

kossebau added inline comments.Tue, Oct 29, 12:40 PM
autotests/jobremotetest.cpp
70

As reader of this code here alone, I wonder what KIO::StatJob::Basic means. To understand what this code does, I would first have to look at the API dox, not good.
So possibly Basic should get a different name, at least contain "Detail" term perhaps. "Basic" also needs context to have semantics, I could e.g. not tell instantly what basic details are. So perhaps needs to be more expliciti here.

src/core/copyjob.cpp
366

Fear the same as said for Basic is true with Default. I would prefer explicit flags here as code reader.

src/core/statjob.h
166

I wonder if this should not be rather a member of StatJob, instead of being on generic KIO namespace level.
It feels unbalanced to have the enum being in the class, but a util flag set not.

meven updated this revision to Diff 68973.Tue, Oct 29, 12:56 PM

Fix jobs details argument passing and fix setDetails(int)

kossebau added inline comments.Tue, Oct 29, 1:00 PM
src/core/statjob.h
166

Actually, this could be part of the StatJob::StatDetail enum, no? Having combinations of lfags in the enum itself (to be used as shortcuts) is usual practice and also should not conflict with Q_DECLARE_FLAGS, IIRC (would need to check).

meven added a comment.Tue, Oct 29, 1:05 PM

Now that is a quick turn-around, +1 for doing the work :)

No time myself to look at this closely the next days, also not that much into KIO, but here some quick feedback with an API police hat on.

Your comments are already of great value, given this needs some polishing.

Would be also good to have some patches which make use of the new API, so one could better judge the usefulness.

The interest here is to have a modern API and better documented that is was and more features.
In the meantime I have ported the rest of KIO to use it.

autotests/jobremotetest.cpp
70

I guess so, "Basic" here lacks context.
But the documentation will be much faster to get than before with the 0 field.

src/core/statjob.h
166

Great suggestion !
I wanted to do that but I just did not find a way to have StatDefaultDetails part of StatJob namespace given Q_DECLARE_OPERATORS_FOR_FLAGS(StatJob::Details) must be declared outside of StatJob class.

Adding it to the enum should just work, working on it.

meven updated this revision to Diff 68975.Tue, Oct 29, 1:09 PM

Move KIO::StatDefaultDetails to KIO::StatJob::StatDefaultDetails

meven marked 3 inline comments as done.Tue, Oct 29, 1:10 PM
meven edited the test plan for this revision. (Show Details)Tue, Oct 29, 1:14 PM
meven retitled this revision from [StatJob] Use A QFlag to specify the details returned by statJob to [StatJob] Use A QFlag to specify the details returned by StatJob.
dfaure requested changes to this revision.Tue, Oct 29, 6:56 PM

If you change the actual values being sent to kioslaves, then this breaks the "wire protocol". I.e. after upgrading KF5 on a live system, kioslaves (started by kdeinit5 forking the "old" libkiocore) will receive numbers from newly started apps (which link to the "new" libkiocore), that they won't understand. Of course a simple kdeinit5 in a terminal would fix that, but most users don't know that. (One idea that has been floating around is to detect changes to libKIO.so and other dependencies and auto-restart kdeinit when these libs are changed... But, hmm, then already-running apps will send old values to newly started kioslaves, problem again).

One solution is to make the enum actually use the old values (0, 2, 3) by doing careful mapping. That's what I thought we would do, but I see you went for a more flexible approach where apps can pick the exact groups of fields they want.

So another solution would be to use a different metadata key and send the old key (with the old value, at least a close approximation of it) for compatibility purposes.

src/ioslaves/file/file.cpp
913–914

int is actually faster on most CPUs, and reserve() will convert it to an int anyway, so it might as well be an int from the beginning.

914

Did you know you can just write if (details & KIO::StatJob::Basic)?
This is more C++-like, which is probably more future-proof if one day don't use QFlags anymore for these types of things.

Or did you use testFlag() for a specific reason?

tests/listjobtest.cpp
44

It's a bit weird to use a StatJob enum in the ListJob class. But then again, it is about the stat() done by listing.... So this is OK, I guess.

This revision now requires changes to proceed.Tue, Oct 29, 6:56 PM
meven updated this revision to Diff 69021.Wed, Oct 30, 5:34 AM
meven marked 3 inline comments as done.

Avoid using short and testFlag

meven planned changes to this revision.Wed, Oct 30, 5:35 AM

If you change the actual values being sent to kioslaves, then this breaks the "wire protocol". I.e. after upgrading KF5 on a live system, kioslaves (started by kdeinit5 forking the "old" libkiocore) will receive numbers from newly started apps (which link to the "new" libkiocore), that they won't understand. Of course a simple kdeinit5 in a terminal would fix that, but most users don't know that. (One idea that has been floating around is to detect changes to libKIO.so and other dependencies and auto-restart kdeinit when these libs are changed... But, hmm, then already-running apps will send old values to newly started kioslaves, problem again).

Thank you for reminding me of this concern.

One solution is to make the enum actually use the old values (0, 2, 3) by doing careful mapping. That's what I thought we would do, but I see you went for a more flexible approach where apps can pick the exact groups of fields they want.

So another solution would be to use a different metadata key and send the old key (with the old value, at least a close approximation of it) for compatibility purposes.

That's the solution I would prefer : it would allow me to add a "TODO KF6 remove to the old metadata key", breaking the wire format should be fine then, so we end up with the best state for KF6.

Next items:

  • Add compatibility to older KIO version on the wire
  • match the flags to statx flags to avoid paying for unwanted fields down to the syscall.
src/core/statjob.h
54

I am open to suggestion here :

  1. Should it be more granular ?
  1. Are names KIO-ish ?

@kossebau :

On a first look, the names seems very short & generic to me, having some other name element might avoid semantic collisions perhaps. No idea yet, not looked further.

src/ioslaves/file/file.cpp
914

I used testFlag because i was documented, and I still have to familiar myself with C/C++.

tests/listjobtest.cpp
44

Does KIO::DefaultDetails make sense beyond stat in the KIO namespace? How does it match other similar flags?

It was @kossebau concern as well.

I am open to suggestion to improve on that, maybe this enum should be in KIO namespace ?
In the meantime, it was what was made the most sense to me.

meven updated this revision to Diff 69023.Wed, Oct 30, 6:46 AM

Use StatDetail to reduce statx mask, support old details metadata in file ioslave, adding metadata statDetails for the new values

meven edited the summary of this revision. (Show Details)Wed, Oct 30, 7:52 AM
meven planned changes to this revision.Thu, Oct 31, 7:58 AM

I don't expect this to make to KF5.64, will update the @since and all.

meven updated this revision to Diff 69181.Sat, Nov 2, 7:44 AM

Allow retro-compatibility with old details both ways : old kio or old app, update comments to ready patch for KF 5.65

dfaure added a comment.Sun, Nov 3, 8:25 PM

You could use KIOCORE_ENABLE_DEPRECATED_SINCE(5, 65) to already put in place the trigger for the compat code to disappear when KF6 comes around, like you did in file_unix.cpp.
It will also make it easier for the future developer who cleans this up.

I mean deletejob.cpp:401, directorysizejob.cpp:137, those places with a "TODO KF6".

tests/listjobtest.cpp
44

Well if it's called KIO::StatDefaultDetails (with the extra "Stat" compared to the question above), then it doesn't need to be in KIO::StatJob.

So yes, you could move the enum to the KIO namespace and define it in src/core/global.h
This will keep the two jobs a bit more separate (no need to add #include "statjob.h" in a few places)

meven updated this revision to Diff 69261.Mon, Nov 4, 12:23 PM

Move KIO::StatJob::StatDetail(s) to KIO::StatDetail(s), wrapped old details metadata code path with KIOCORE_ENABLE_DEPRECATED_SINCE

meven updated this revision to Diff 69385.Thu, Nov 7, 12:27 PM
meven marked an inline comment as done.

Add Stat prefix to enum values of KIO::StatDetail

meven added a comment.Thu, Nov 7, 12:31 PM

I feel the code is ready for review toward merging.

src/core/global.h
321

Naming should be explicit enough now.
I still wonder about granularity if I should expose some more.

meven marked 5 inline comments as done.Thu, Nov 14, 8:41 AM

friendly ping

Is https://phabricator.kde.org/D25010#inline-142132 satisfactory ?
I think names are now explicit enough.
I still wonder if the granularity level is enough.

dfaure requested changes to this revision.Wed, Nov 20, 9:22 AM

About granularity: I think it's fine. I was about to say that the use case of a recursive listing to calculate directory size only needs type and size, but it actually needs name too (for dirs), in order to recurse into subdirs.

So overall, I think Name is always useful, and should always be provided, whether or not the Basic flag is set.

Access, type and size aren't always needed, but they come at zero cost (copying a few ints), while name implies string conversions (8bit->unicode). So I don't think it makes sense to make access, type and size optional.

Hmm, with this reasoning, linkDest should be separated out. But that means going through all users of StatBasic and finding out if they look at UDS_LINK_DEST.

src/core/statjob.h
92

Typo: s/stat/Stat/

190

Not sure what this specific sentence adds to the previous one. It's a parameter, obviously that means we can choose what to pass to it... Maybe more useful to mention that this is for performance reasons, less details implies more speed.

227

Why does this say "int" after "StatDetails"?

235

Typo: direcly -> directly (happens again two lines down)

237

missing the same #if as the above methods, no?
In a "clean" build it should disappear.

src/filewidgets/kdiroperator.cpp
748

This would actually be a use case for an even more basic level, "NoDetails"...

src/ioslaves/file/file.h
123

Please move it up with the other private methods. This last section is member variables only.

Also, it could be marked as const.

src/ioslaves/file/file_unix.cpp
542

file_win.cpp needs to be ported the same way.

830

This should go to file.cpp so that it's available on Windows too, it's not Unix specific.

OK, right now it's not really implemented on Windows, but let's make it easy for the future developer who will implement it ;)

This revision now requires changes to proceed.Wed, Nov 20, 9:22 AM
meven updated this revision to Diff 70051.Wed, Nov 20, 1:16 PM
meven marked 8 inline comments as done.

Typos, rephrase a comment, move FileProtocol::getStatDetails to file.cpp, add a TODO for in file_win.cpp

dfaure accepted this revision.Wed, Nov 20, 9:50 PM

Hmm, I meant Name is always useful when *listing*. But when *stating*, we don't always need to get the name back. The NoDetails comment is about a stat that really can just succeed/fail.

We can always add NoDetails later (with a value of 0), however, so I'll stop nitpicking here :-)

This revision is now accepted and ready to land.Wed, Nov 20, 9:50 PM
kossebau added inline comments.Wed, Nov 20, 9:55 PM
src/core/directorysizejob.cpp
137–141

More correct would be KIOCORE_BUILD_DEPRECATED_SINCE(5, 65)., not ENABLE here and in the other cpp files.

Ah, EXCLUDE_DEPRECATED_BEFORE_AND_AT is not yet available with KIO (compare the TODO next to the ecm_generate_export_header call).
So as result also the BUILD macros are not available. Using instead the ENABLE macros in the sources of the library instead does not make that much sense, they will be never triggerd to evaluate to FALSE.

IMHO they should be left out simply in the sources.

Isn't it simpler to commit as is, so we can just s/ENABLE/BUILD/ in *.cpp files once we add support for EXCLUDE_DEPRECATED_BEFORE_AND_AT? Faster than having to figure out where is the implementation of each method to exclude.

meven updated this revision to Diff 70088.Thu, Nov 21, 8:42 AM

Add NoDetails to StatDetail, add StatJob::setDetails\(KIO::StatDetail detail\), small comment update

meven added a comment.Thu, Nov 21, 8:48 AM

Hmm, I meant Name is always useful when *listing*. But when *stating*, we don't always need to get the name back. The NoDetails comment is about a stat that really can just succeed/fail.

I added

enum StatDetail {
    /// No field returned, useful to check if a file exists
    NoDetails = 0x0,

Funny thing is that statx includes always the name, down to syscall we can save at most STATX_SIZE , STATX_TYPE fields in NoDetails case.
I updated kdiroperator.cpp to use it line 748 since it only checks if a file exists.
This code path is not run for local files anyway.

We can always add NoDetails later (with a value of 0), however, so I'll stop nitpicking here :-)

I would not mind nitpicking/discussing a little more, this is easier to do now and since it is no small change.
As long as we don't go off topic at least now I have a design standpoint validation.

Another thing comes to mind
If we allow not to fill KIO::UDSEntry::UDS_LINK_DEST but when KIO::ResolveSymlink is passed, we can save the second STAT by not passing AT_SYMLINK_NOFOLLOW to the first statx.
So perhaps we want to expose a detail for this use case : that is add a IncludeLinkDest to KIO::StatDetail removing this field for StatDetail::Basic.
I am not sure we have any use case currently though.

src/core/statjob.h
237

It shares the same #if block with the stat function