> On June 23, 2013, 6:29 p.m., Milian Wolff wrote: > > kdecore/services/kmimetype.cpp, line 376 > > <http://git.reviewboard.kde.org/r/111050/diff/2/?file=165321#file165321line376> > > > > unrelated change > > Mark Gaiser wrote: > Well, i changed the function so i took the liberty of updating the code > style for that function as well. Do you mind if i keep this in? > > Milian Wolff wrote: > In general I think this is frown upon (I do, at least). Thing is, you > then uglify your patch and hide the actual interesting logic in potentially > many of these "cleanup" changes. Separate the two so the git history will > show something like: > > patch1: improve performance > patch2: cleanup
For just 2 brackets in dead code? > On June 23, 2013, 6:29 p.m., Milian Wolff wrote: > > kdecore/services/kmimetype.cpp, line 345 > > <http://git.reviewboard.kde.org/r/111050/diff/2/?file=165321#file165321line345> > > > > try using QStringRef here to safe allocations if you care about speed. > > Mark Gaiser wrote: > QStringRef misses the mid function which i need. Or left or right, but it > misses those as well. So i don't really see how i can use QStringRef here > without adding another QString... > > Milian Wolff wrote: > You can still use midRef on the base QString, you just need to take the > offsets into account. But anyways, this is probably not worth it if you never > saw this in a profile as a hotspot. Just saying, that in general repeated > .mid() is quite often a performance bottleneck. Not worth it indeed. "Usually" files have only one "." in there name so it won't matter much. It only "might" matter if files have a lot of dots and if you have a lot of those filenames. But even then, other things will likely end up as higher hot spots. - Mark ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111050/#review34924 ----------------------------------------------------------- On June 23, 2013, 9:21 p.m., Mark Gaiser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111050/ > ----------------------------------------------------------- > > (Updated June 23, 2013, 9:21 p.m.) > > > Review request for kdelibs, David Faure and Frank Reininghaus. > > > Description > ------- > > Hi, > > I've recently seen Frank Reininghaus do his best in speeding up the rendering > in dolphin with regards to the app icons. And trying to prevent icon > flickering between "unknown" and the actual icon. > > While reading his posts on the mailing list i was beginning to wonder: "is > fast mime detection actually fast"? While it was certainly faster then "slow" > mime detection, it still didn't really seem fast to me. A small benchmark app > hat ran fast mime detection in /usr/bin took ~40ms to complete. That's for > just 2656 items. > > After quite a bit of profiling i managed to to bring the duration down from > ~40ms to ~3ms sometimes ~4ms. That's well over 10x faster. > Mime detection by extension (like "file.tar.bz") is done as follows: > > file.tar.bz > Loop - find first dot > - "tar.gz" > if that matches a mime type then it's returned if it doesn't then it proceeds > on to the next dot: > - next dot: "gz" > if that matches.. return. > Otherwise it will return the default mime type. > > I am getting an inconsistency. Using the unpatched fast mime detection on a > file like: "test.tar.gz" gets detected as "application-x-compressed-tar" > where the patched version detects it as "application-gzip". The slow and > detailed mime detection detects the same file as > "application-x-compressed-tar". What should it be? application-gzip or > application-x-compressed-tar? > > Note: This improved detection does expect folders to end with a "/". > Otherwise they will be detected as application-octet-stream (the default). > But i think this is common sense to let folders end with a "/". If any apps > that don't do that, they should fix it i suppose. > > Best thing, it's all internal and private api change. No public function is > changed. > > All feedback is welcome! If possible, i would like to put this in KDE 4.11. > > > Diffs > ----- > > kdecore/services/kmimetype.h bc35bcf > kdecore/services/kmimetype.cpp d748523 > kdecore/services/kmimetyperepository.cpp f56f48e > > Diff: http://git.reviewboard.kde.org/r/111050/diff/ > > > Testing > ------- > > Tested this using just output comparison between the old version and the new > implementation. It works just fine. > kurlmimetest output: > ********* Start testing of KUrlMimeTest ********* > Config: Using QTest library 4.8.4, Qt 4.8.4 > PASS : KUrlMimeTest::initTestCase() > PASS : KUrlMimeTest::testURLList() > PASS : KUrlMimeTest::testOneURL() > PASS : KUrlMimeTest::testFromQUrl() > PASS : KUrlMimeTest::testMostLocalUrlList() > PASS : KUrlMimeTest::cleanupTestCase() > Totals: 6 passed, 0 failed, 0 skipped > ********* Finished testing of KUrlMimeTest ********* > > > Thanks, > > Mark Gaiser > >
