- User Since
- Apr 16 2015, 7:53 PM (144 w, 2 d)
Thu, Jan 18
this was comitted already, closing it again
I'm OK with this, but please split this up the next time. You could still have posted it as one review request, but splitting it up is always a good idea. It's a general coding mantra that you should start following, as it makes your life much easier in the long term.
Wed, Jan 17
I've adapted and pushed this now. Many thanks Ivan! Much appreciated.
Aha, -DTAKE_ADDR fails when I use ld.bfd instead of ld.gold, I assume you do that too? I still don't see a difference with using -Wl,-z,now though...
I fail to reproduce the issue with the provided test case on ArchLinux. When I compile the three variants, I always properly detect the free calls.
Thanks, I'll take over then. What email address should I use to attribute (parts of) the patch to?
Tue, Jan 16
Yeah, I agree. Let's fix it. But I won't write mega patches like you seem to prefer. I try to keep things as minimal as possible. And I also can't give you an ETA or guarantee on when I'll fix the rest. This patch solves one big issues, and the added benchmark lies the foundation for future work. Let's start from here and get going.
ping? @kfunk maybe?
@rjvbb why did you request changes to proceed with this patch? The fact that there are more issues in KDirWatch does not mean we should hold up this approach.
use QHash::insert instead of operator
Mon, Jan 15
cleanup now that I figured out what happens, thanks @sitter
[13:35] <milian> how the hell can kdesu ever work? https://phabricator.kde.org/D9888 WTF [13:35] <milian> does anyone know if I'm missing something? [13:35] <milian> googling for these macros doesn't show me anything obvious either [13:53] <sitter> milian: dead code I'd say. note the `if` following your change checks if the file 'false' is exectuable (which it isn't as that'd be ./false which likely never exists) and then falls back to QSP::findExec(cmd). where cmd is macro'd to sudo or su depending on the cmake switch KDESU_USE_SUDO_DEFAULT [13:54] <milian> but it doesn't work for me without this patch [13:54] <milian> so it cannot be dead code :D [13:55] <milian> i.e. how can this work for anyone right now? [13:58] <sitter> milian: http://paste.debian.net/1005279/ is how I read the original [13:59] <milian> I'll see why this does not happen on my system [14:02] <sitter> milian: try this for good measure http://paste.debian.net/1005280/ [14:02] <milian> yep [14:03] <sitter> mind you, it could be that __PATH_SU/SUDO is actually defined somewhere in a system level include which would then make the QT_ACCESS pass and break things [14:03] <milian> I can't find it in my /usr/include at least [14:06] <milian> sitter: QT_ACCESS("false", X_OK) == 0 on my system [14:07] <milian> and no, there's no false in the current PWD [14:07] <milian> it's in /usr/bin though [14:07] <milian> I'd also be OK with removing that whole code and simplifying it [14:08] <milian> like you proposed
- do not ever profile a debug build, the results are completely bogus
- do not use callgrind, use perf
format results on phab
fixup commit message
150KB is not a lot of memory
thanks dfaure ;-)
Thu, Jan 11
cleaner, yes. but also much slower. contrary to the other code-paths, the QTimer::singleShot taking a functor is not optimized (yet?) for timeout == 0...
So just remove the assert - instead return early when indexes.isEmpty(). That way we don't need to mess with the signal blocker and the code behaves as intended. And you remove code instead of adding more.
make compile against older Qt
requires 5.10, so I can't commit this as-is...
@rjvbb this patch just shows that KDirWatch has tons of performance issues that need to be fixed here. Please do run the new benchmarks on your benchmark with your backend, profile them, optimize them.
lgtm in general, but can be cleaned up
10 calls per second sound fine to me, that shouldn't be a big performance issue at all. Are you measuring performance of a debug build or of a release build? Can you specify the exact commands you are profiling? Is the performance better when you are using KFormat here?
I don't understand how this fixes the crash. Can you extend the bug report with a better backtrace that contains more symbols? Also, the bug report looks like it's due to a queued signal emission. Which signal is that? And it looks like something withing the kdevplatform project library, how is that related to the compiler selection?
Wed, Jan 10
Imo this should be using KFormat::formatByteSize. https://api.kde.org/frameworks/kcoreaddons/html/classKFormat.html#ae7412420b70e2ca935d0ebed6770e313
Tue, Jan 9
I'll push it then