Details

Reviewers

Group Reviewers

Commits

R241:e80c31163170: Use the new uds implementation

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

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".

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.

I requested it, for proper encapsulation

In D12696#259292, @dfaure wrote:

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?

Oh OK I had misunderstood your comment.

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

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.

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 &); ^

src/core/udsentry.cpp

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 &);
                  ^

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 ..

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 ;-)

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

Closed by commit R241:e80c31163170: Use the new uds implementation (authored by jtamate). · Explain WhyMay 10 2018, 5:26 PM

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

In D12696#275105, @martinkostolny wrote:

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:

udsentry_bt.txt2 KBDownload

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

udsentry_debug.log4 KBDownload

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.

In D12696#276660, @martinkostolny wrote:

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'

In D12696#276696, @bruns wrote:

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

			Path	Packages
M			src/core/kfileitem.cpp (2 lines)
M			src/core/listjob.cpp (4 lines)
M			src/core/udsentry.h (39 lines)
M			src/core/udsentry.cpp (456 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	33622			May 4 2018, 11:36 AM	★	★
Diff 2	33758			May 7 2018, 3:07 PM	★	★
Diff 3	33809			May 8 2018, 7:54 AM	★	★
Diff 4	33815			May 8 2018, 11:28 AM	★	★
Diff 5	33889			May 9 2018, 2:27 PM	★	★
Diff 6	33929			May 10 2018, 8:26 AM	★	★
Diff 7	33957	2a57054	R241:e80c31163170125e79ce2735a1e3cb029cedfbc9	May 10 2018, 5:25 PM	★	★

Use the new uds implementation
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 33809

src/core/kfileitem.cpp

src/core/listjob.cpp

src/core/udsentry.h

src/core/udsentry.cpp

Use the new uds implementationClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 33809

src/core/kfileitem.cpp

src/core/listjob.cpp

src/core/udsentry.h

src/core/udsentry.cpp

Use the new uds implementation
ClosedPublic
Actions

Revision Contents
Changeset List