[SlaveBase] Use QMap instead of KConfig to store ioslave config
ClosedPublic

Authored by meven on Aug 28 2019, 10:51 AM.

Details

Summary

Inspired by https://github.com/blue-systems/plasma-5.16/issues/139#issuecomment-441704579
After some review, it seems to me using KConfig is not necessary here.
Removing its use should improve IO throughput since rebuildConfig is called in finished().
So mark the old config() function as deprecated.

TODO

  • port ioslaves in kio and kio-extras to use mapConfig() instead of config()
  • Clean up this description
Test Plan

ctest

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D23523
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17597
Build 17615: arc lint + arc unit
meven created this revision.Aug 28 2019, 10:51 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 28 2019, 10:51 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Aug 28 2019, 10:51 AM
broulik added inline comments.
src/core/slavebase.h
343

Why return a pointer, and why is this method not const?
Or is this supposed to be writable? Then I would prefer returning a reference to communicate ownership properly.

meven updated this revision to Diff 64816.Aug 28 2019, 11:01 AM

Make mapConfig return a const ref

meven marked an inline comment as done.Aug 28 2019, 11:01 AM
meven updated this revision to Diff 65006.Aug 30 2019, 3:48 PM

Lazily construct the KConfig but keep the instance as long as no config changes has occured

broulik added inline comments.Sep 1 2019, 4:33 PM
src/core/slavebase.cpp
189

delete nullptr is fine, no need to check first

282

This is initialized automatically like this

290

Don't you still want this?

420

Just check if (!d->config)

meven updated this revision to Diff 65200.Sep 2 2019, 6:54 AM
meven marked an inline comment as done.

Review feedback

meven marked 3 inline comments as done.Sep 2 2019, 6:55 AM
meven added inline comments.
src/core/slavebase.cpp
282

That's on purpose, the first use of mapConfig is in rebuildConfig and is a clear() call.

dfaure requested changes to this revision.Sep 2 2019, 12:25 PM

I admit that I never understood why this was using KConfig. I guess Waldo had a hammer and everything looked like nails ;-)

src/core/slavebase.cpp
191

missing config = nullptr so the delete config in the destructor doesn't crash.

Same with configGroup.

413

That is a weird abuse of const. If the member wasn't in a d pointer, this wouldn't compile.

If the slave is supposed to only read from the map, then it should be a value or const ref.
If the slave can write into the map, then this method shouldn't conceptually be const.

src/core/slavebase.h
342

missing @since

353

@deprecated since 5.xx

This revision now requires changes to proceed.Sep 2 2019, 12:25 PM
meven updated this revision to Diff 65374.Sep 4 2019, 4:10 PM
meven marked 5 inline comments as done.

Review feedback, add @since, review mapConfig signature, reset config and configGroup to nullptr after deletion

dfaure requested changes to this revision.Sep 12 2019, 12:40 PM
dfaure added inline comments.
src/core/slavebase.cpp
38

what is this used for?

282

So? This line is not needed, an empty map is an empty map, no need to assign an empty map to it.

src/core/slavebase.h
342

Time passed, this is for 5.63 now, sorry about that.

353
@deprecated since 5.63, use mapConfig() instead.
This revision now requires changes to proceed.Sep 12 2019, 12:40 PM
meven updated this revision to Diff 65931.Sep 12 2019, 3:49 PM
meven marked 2 inline comments as done.

Clean up and update @since

dfaure requested changes to this revision.Sep 13 2019, 11:54 AM
dfaure added inline comments.
src/core/slavebase.h
353

Can you update the docu as suggested above, to point more clearly to the replacement? This makes things easier for people doing porting. Thanks.

This revision now requires changes to proceed.Sep 13 2019, 11:54 AM
meven added inline comments.Sep 13 2019, 12:17 PM
src/core/slavebase.h
353

I have a @see mapConfig on the line below, did you see ? Or do you mean I should have all this on the same line.

Yes, I see the "see" :-)
But that's just a generic way to say "also look at these other methods", it doesn't constitute a clear statement like "use B instead of A".

meven updated this revision to Diff 65991.Sep 13 2019, 2:04 PM

Update @deprecated message to be more clear

meven marked 3 inline comments as done.Sep 13 2019, 2:04 PM
dfaure accepted this revision.Sep 14 2019, 12:16 PM

Thanks!

This revision is now accepted and ready to land.Sep 14 2019, 12:16 PM
meven edited the summary of this revision. (Show Details)Oct 2 2019, 9:24 AM
meven updated this revision to Diff 67762.Oct 12 2019, 7:27 AM

Update since, use QVariant as mapConfig value

meven updated this revision to Diff 67763.Oct 12 2019, 7:29 AM

Fix slavebase.cpp following QVariant changes

dfaure accepted this revision.Oct 15 2019, 6:23 AM

Looks fine, but it seems that the uploaded diff is only the delta compared to the previous diff, not the overall change. Anyhow, feel free to push both.

meven added a comment.Oct 15 2019, 7:13 AM

Looks fine, but it seems that the uploaded diff is only the delta compared to the previous diff, not the overall change. Anyhow, feel free to push both.

It was on purpose to have two clean commits : add a new API, use it in the second commit.

This revision was automatically updated to reflect the committed changes.
kossebau added inline comments.
src/core/slavebase.h
355

Given there are still a few usages of config() left which seem not easily replaceable, it would be better to remove the deprecation tag for the compiler, to not have false warnings on those places (see e.g. http.cpp for certain usages still).
A deprecation tag should be only set if there is a full migration path available ideally, otherwise people will run into warnings they cannot do something about.

meven added inline comments.May 13 2020, 1:29 PM
src/core/slavebase.h
355

Agreed, perhaps update the documentation slightly