Use the new uds implementation
ClosedPublic

Authored by jtamate on May 4 2018, 11:44 AM.

Details

Summary

KIO::UDSEntry, switch to a single std::vector for more performance.
This new implementation is the fastest in the autotests.
This new implementation has asserts in the insert() methods to detect an already inserted entry, in that case the insert() call must be changed into a replace() call.
Detected so far:

  • KFileItem::init(), setTime(), setLocalPath() because m_entry can have entries from the constructor.
  • KFileItem::setName, when renaming a file in dolphin.
  • ListJobPrivate::slotListEntries, running Kfind.
Test Plan

It should be used extensively at least one release cycle before pushing it.

Passes the autotests.
Don't assert in any kde program that uses kio.

Diff Detail

Repository
R241 KIO
Lint
Lint Skipped
Unit
Unit Tests Skipped
jtamate created this revision.May 4 2018, 11:44 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 4 2018, 11:44 AM
jtamate requested review of this revision.May 4 2018, 11:44 AM
dfaure requested changes to this revision.May 6 2018, 10:31 PM

Looks good.

src/core/udsentry.cpp
46

1ms is relative to a benchmark which isn't clear when reading this code in other contexts. As someone said in another review, do we still need this operator== anyway, given that you pass lambdas to find_if()?

125

remove this "public:", we're already in the public section.

But actually, now that there are methods wrapping all usage of "storage", how about making "storage" private? (moving it to the end of the class, to respect the standard order in Qt/KF5 code: public / protected / private).

Actually, the Field class definition can move with it too.

Ah, I see a few remaining uses from the outside, how about moving it all in for proper encapsulation? Some save/load methods with QDataStream, and a method that takes QDebug...

210
#ifndef KIOCORE_NO_DEPRECATED

like the only caller

212
res.reserve(storage.count());
220
res.reserve(storage.count());
548

Typo: unknown

src/core/udsentry.h
320

@since 5.47

327

@since 5.47

Should these be called just replace() like QMultiMap::replace also means "replace or insert" ?

333

I see you're using this internally, is it needed in the public API? I'd just make it internal.

This revision now requires changes to proceed.May 6 2018, 10:31 PM

First line of commit log should have changelog in mind, so more context needed. Maybe rather something like "KIO::UDSEntry: switch to a single std::vector for more performance".

jtamate updated this revision to Diff 33758.May 7 2018, 3:16 PM
jtamate marked 9 inline comments as done.
jtamate edited the summary of this revision. (Show Details)

Hopefully, all the issues fixed.

dfaure requested changes to this revision.May 7 2018, 3:31 PM
dfaure added inline comments.
src/core/udsentry.cpp
132 ↗(On Diff #33758)

strange that it's static, "a.d" could be this, if it wasn't static.

151 ↗(On Diff #33758)

same

265 ↗(On Diff #33758)

should also not be static

This revision now requires changes to proceed.May 7 2018, 3:31 PM
bruns added a subscriber: bruns.May 7 2018, 3:36 PM
bruns added inline comments.
src/core/udsentry.cpp
132 ↗(On Diff #33758)

Is there a reason to define the methods inside the class declaration? The diff would be significantly smaller if you kept the definition separate.

dfaure added a comment.May 7 2018, 6:57 PM

I requested it, for proper encapsulation

bruns added a comment.May 7 2018, 8:02 PM

I requested it, for proper encapsulation

Whats the difference between

class UDSEntryPrivate
{
  void load(...) {
    // function body
  }
};

and

class UDSEntryPrivate
{
  void load(...);
};

void UDSEntryPrivate::load(...)
{
    // function body
}

from an encapsulation perspective?

dfaure added a comment.May 7 2018, 8:21 PM

Oh OK I had misunderstood your comment.

Right, those methods don't have to be inline.

jtamate updated this revision to Diff 33809.May 8 2018, 8:19 AM

Don't use java style.
As UDSEntryPrivate is private, declare public methods save, load and debugUDSEntry, otherwise I couldn't access the private d pointer from the << and >> operators.

jtamate updated this revision to Diff 33815.May 8 2018, 11:31 AM

Make some friends functions.
A bad type in copy&paste didn't allowed me the use of the module functions save/load/debugUDSEntry,

dfaure added inline comments.May 8 2018, 12:03 PM
src/core/udsentry.cpp
403 ↗(On Diff #33758)

Hmm why can't this be the friend function directly?

I don't like the added global functions in the public header...

jtamate added inline comments.May 8 2018, 2:16 PM
src/core/udsentry.cpp
46

ok, ok, bye bye ==

403 ↗(On Diff #33758)

Hmm why can't this be the friend function directly?

Because the compiler (clang++ in this case) doesn't know which one to apply.
Unless both definitions are equivalent (in source and binary) and the later can be removed. That is something I don't known.

src/core/slavebase.cpp:728:14: error: use of overloaded operator '<<' is ambiguous (with operand types 'QDataStream' and 'const KIO::UDSEntry')
    KIO_DATA << entry;
    ~~~~~~~~ ^  ~~~~~
src/core/udsentry.h:315:40: note: candidate function
    friend QDataStream &operator<< (QDataStream &s, const KIO::UDSEntry &a);
                        ^
src/core/udsentry.h:361:29: note: candidate function
KIOCORE_EXPORT QDataStream &operator<< (QDataStream &s, const KIO::UDSEntry &a);
                            ^

I don't like the added global functions in the public header...

Me neither, but otherwise clang++will not be able to find the function.

src/core/udsentry.h:314:19: error: no function named 'save' with type 'void (QDataStream &, const KIO::UDSEntry &)' was found in the specified scope
    friend void ::save(QDataStream &, const KIO::UDSEntry &);
                  ^
jtamate updated this revision to Diff 33889.May 9 2018, 2:39 PM
jtamate edited the summary of this revision. (Show Details)

Direct friendship with the operators.

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptMay 9 2018, 2:39 PM
bruns added inline comments.May 9 2018, 3:28 PM
src/core/kfileitem.cpp
198 ↗(On Diff #33758)

m_entry.reserve(9)

198 ↗(On Diff #33758)

Wouldn't it be better to call m_entry.clear() and m_entry.insert(...) here?

For the call from the constructor, m_entry is empty, so you save the std::find_if() in the replace method calls.

For any other caller, is there any useful info in m_entry() which is still valid when calling init() again?

src/core/udsentry.cpp
195 ↗(On Diff #33758)

quint32, see ..::load(...) below.

308 ↗(On Diff #33758)

I can count 10 inserts below ...

bruns added inline comments.May 9 2018, 3:35 PM
src/core/kfileitem.cpp
198 ↗(On Diff #33758)

Ignore this one ..

dfaure added a comment.May 9 2018, 7:30 PM

Starting to look good ;-)

src/core/udsentry.h
143 ↗(On Diff #33758)

The old insert() should be documented that it asserts if the entry already exists, and that one should use replace() instead. This will minimize surprises in case anyone hits this.

319 ↗(On Diff #33758)

heh, nice syntax ;-)

jtamate updated this revision to Diff 33929.May 10 2018, 8:29 AM
jtamate marked 8 inline comments as done and 3 inline comments as done.
jtamate edited the summary of this revision. (Show Details)

Added the documentation for insert.
Removed the () from the QDataStream& operators, but must be kept for QDebug or clang++ will not compile.
Adjusted the reserved amounts.

dfaure accepted this revision.May 10 2018, 11:10 AM
This revision is now accepted and ready to land.May 10 2018, 11:10 AM
This revision was automatically updated to reflect the committed changes.

Hi! Probably after this commit sftp slave crashes when showing a directory with links. Please see my code comment. Can you also reproduce or is it on my side only?

Here is a crash log:

log_kio_sftp: readdir:  "/mnt/ext/files" , details:  "2"
ASSERT: "std::find_if(storage.cbegin(), storage.cend(), [udsField](const Field &entry) {return entry.m_index == udsField;}) == storage.cend()" in file /home/kotelnik/kde/src/frameworks/kio/src/core/udsentry.cpp, line 107
kioslave: ####### CRASH ###### protocol = kio_sftp pid = 20126 signal = 6
/home/kotelnik/kde/usr/lib64/libKF5KIOCore.so.5(+0x9d42d)[0x7fa660a1042d]
/usr/lib/libc.so.6(+0x368f0)[0x7fa6689888f0]
/usr/lib/libc.so.6(gsignal+0x10b)[0x7fa66898886b]
/usr/lib/libc.so.6(abort+0x129)[0x7fa66897340e]
/usr/lib/libQt5Core.so.5(+0x8033c)[0x7fa6696c433c]
/usr/lib/libQt5Core.so.5(_Z11qt_assert_xPKcS0_S0_i+0x0)[0x7fa6696c3768]
/home/kotelnik/kde/usr/lib64/libKF5KIOCore.so.5(+0x1033bb)[0x7fa660a763bb]
/home/kotelnik/kde/usr/lib64/libKF5KIOCore.so.5(_ZN3KIO8UDSEntry6insertEjx+0x33)[0x7fa660a78403]
/home/kotelnik/kde/usr/lib64/plugins/kf5/kio/sftp.so(+0x12371)[0x7fa657ee4371]
/home/kotelnik/kde/usr/lib64/libKF5KIOCore.so.5(_ZN3KIO9SlaveBase8dispatchEiRK10QByteArray+0x86f)[0x7fa660a12961]
/home/kotelnik/kde/usr/lib64/libKF5KIOCore.so.5(_ZN3KIO9SlaveBase12dispatchLoopEv+0x2a0)[0x7fa660a0e2aa]
/home/kotelnik/kde/usr/lib64/plugins/kf5/kio/sftp.so(kdemain+0x264)[0x7fa657ed88ca]
src/core/udsentry.cpp
182 ↗(On Diff #33758)

sftp slave crashes here when entering directory with links

Hi! Probably after this commit sftp slave crashes when showing a directory with links. Please see my code comment. Can you also reproduce or is it on my side only?

I'm sorry, I can't reproduce it with dolphin. (symbolic links and hard links and sftp://127.0.0.1).
Could you get a backtrace with more information, at least who is calling udsentry.cpp, line 107? Probably you can get it running the program inside gdb or under valgrind. Or you should compile the program and dependencies with -DCMAKE_BUILD_TYPE=RelWithDebInfo

src/core/kfileitem.cpp
198 ↗(On Diff #33758)

In fact, I've tried creating a method to delete in one pass some selected values and then insert again, but it was 10 milliseconds slower than calling replace directly.

src/core/udsentry.h
319 ↗(On Diff #33758)

This syntax must be kept in the case of the QDebug << operator, otherwise clang++ doesn't compile.

bruns added inline comments.Jun 7 2018, 11:56 AM
src/core/udsentry.cpp
182 ↗(On Diff #33758)

Any chance you can attach a debugger (or load the core file with gdb) and provide the values of 'udsField' and 'value'?

Thanks for guidance regarding getting additional debug info. I'll try to get it soon :).

No crash in my case, but this commit makes links with sftp:/ inaccessible.

I wonder if this is related to timeline:/ no longer working properly in the file dialog. (didn't investigate yet).

I've managed to get more info about the crash also by following this guide:
https://community.kde.org/Guidelines_and_HOWTOs/Debugging/Debugging_IOSlaves#Attaching_gdb_to_an_io-slave

Here is backtrace from gdb:

Here is debug log of sftp slave - I've added a debug line to each insert() method in UDSEntryPrivate:

Performed test was done against a remote ssh server (Solaris), therefore IP and paths are changed by me in the debug log. But I get the same results when testing against my own localhost ssh server. I can do other tests on demand and on localhost :).

I have Arch Linux with latest updates, Qt 5.11.0 installed. The rest of KDE is running in kdesrc-build environment, which was updated probably 2 days ago.

bruns added a comment.Jun 10 2018, 3:07 PM

udsField=33554445, value=16384
33554445 = 0x200000d = UDS_NUMBER = 0x02000000 | UDS_FILE_TYPE = 13
https://api.kde.org/frameworks/kio/html/classKIO_1_1UDSEntry.html

UDS_NAME = 6: "linked-folder"
UDS_LINK_DEST = 14: "/mnt/add/folders/linked-folder"
UDS_FILE_TYPE = 13: 32768 -> 16384 (0100000 -> 040000)
File mode changes from 'regular file' to 'directory'

bruns added a comment.Jun 10 2018, 3:24 PM

UDS_FILE_TYPE = 13: 32768 -> 16384 (0100000 -> 040000)
File mode changes from 'regular file' to 'directory'

https://github.com/KDE/kio-extras/blob/master/sftp/kio_sftp.cpp#L1845
entry.insert(KIO::UDSEntry::UDS_FILE_TYPE, S_IFREG);

The UDS_FILE_TYPE is also set in L1873 ff to the actual file type the symlink points to.

I'm sorry, I was missing a -dev package and therefore I was not compiling kio sftp.

A fix for this problem is in D13475