date:20131022

Re: Review Request 112848: Adding unit tests for kbuttongroup and fixing identation

2013-10-22 Thread Commit Hook


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112848/#review42149
---


This review has been submitted with commit 
b7944b5cae734f08d69062980b9312fe43229ce4 by David Faure on behalf of Bruno 
Farias to branch master.

- Commit Hook


On Oct. 10, 2013, 10:36 p.m., Bruno Farias wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112848/
> ---
> 
> (Updated Oct. 10, 2013, 10:36 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> - Adding new test for kbuttongroup
> - Fixing kbuttongroup.cpp identation
> 
> 
> Diffs
> -
> 
>   kdeui/tests/kbuttongrouptest.h cd9130a9b0840160bdce5a1841735e2050d10a7d 
>   kdeui/tests/kbuttongrouptest.cpp d5f9f88b839d4be88cac81c1e7d022dd17ea 
>   kdeui/widgets/kbuttongroup.h de813ea95dca1247698537a966abe474fa9650cf 
>   kdeui/widgets/kbuttongroup.cpp 4057cc023107cd76cc34590f4b1dd91e01717d3d 
> 
> Diff: http://git.reviewboard.kde.org/r/112848/diff/
> 
> 
> Testing
> ---
> 
> All tests of KButtonGroupTest were passed
> 
> 
> Thanks,
> 
> Bruno Farias
> 
>

Re: Review Request 112848: Adding unit tests for kbuttongroup and fixing identation

2013-10-22 Thread Bruno Farias


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112848/
---

(Updated Oct. 22, 2013, 7:52 a.m.)


Status
--

This change has been marked as submitted.


Review request for kdelibs.


Repository: kdelibs


Description
---

- Adding new test for kbuttongroup
- Fixing kbuttongroup.cpp identation


Diffs
-

  kdeui/tests/kbuttongrouptest.h cd9130a9b0840160bdce5a1841735e2050d10a7d 
  kdeui/tests/kbuttongrouptest.cpp d5f9f88b839d4be88cac81c1e7d022dd17ea 
  kdeui/widgets/kbuttongroup.h de813ea95dca1247698537a966abe474fa9650cf 
  kdeui/widgets/kbuttongroup.cpp 4057cc023107cd76cc34590f4b1dd91e01717d3d 

Diff: http://git.reviewboard.kde.org/r/112848/diff/


Testing
---

All tests of KButtonGroupTest were passed


Thanks,

Bruno Farias

Re: Review Request 112982: copyToFile support for kio_smb

2013-10-22 Thread David Faure

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112982/#review42150
---

This would have been easier to review if the coding style changes had been a 
different commit (not uploaded to reviewboard), to get only the actual changes 
here...

kioslave/smb/kio_smb_dir.cpp

this TODO can be removed now, right?

kioslave/smb/kio_smb_dir.cpp

The space before '(' is strange (although this file is pretty inconsistent 
in the first place...)

kioslave/smb/kio_smb_dir.cpp

0|S_IWUSR is funny :)

kioslave/smb/kio_smb_dir.cpp

You could move this before the if(), and use it like if (finfo.exists()), 
it would be faster than the separate call to QFile::exists, since you'd be 
reusing the QFileInfo, which can cache the stat() result.

kioslave/smb/kio_smb_dir.cpp

I'm pretty sure this cast is wrong. QFile::Permissions doesn't map to 
mode_t, at least not with a simple cast.

Either do like kio_ftp does (KDE::open() instead of QFile), or add a call 
to chmod() at the end of the operation, or use a function to convert from 
mode_t to QFile::Permissions (maybe we want to have that in KFileItem, in fact).

I prefer solution 2 or 3 above solution 1, so that QFile can be used as 
much as possible.

kioslave/smb/kio_smb_dir.cpp

There's also the case where n (as returned) is smaller than n (as passed to 
write). So better use a different variable, and error if ret < n. This can 
happen when the partition is full.

- David Faure

On Oct. 5, 2013, 3:07 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112982/
> ---
> 
> (Updated Oct. 5, 2013, 3:07 p.m.)
> 
> 
> Review request for KDE Runtime.
> 
> 
> Bugs: 176271 and 291835
> http://bugs.kde.org/show_bug.cgi?id=176271
> http://bugs.kde.org/show_bug.cgi?id=291835
> 
> 
> Repository: kde-runtime
> 
> 
> Description
> ---
> 
> The attach patch adds support for the following to kio_smb:
> 
> - copyToFile optimization so downloading files from window shares is faster.
> - partial download resumption as part of the copyToFile implementation.
> - preservation of modified file timstamp. Again as part of the copyToFile 
> implementation.
> 
> Note that in this patch the latter two features only apply to "smb" -> "file" 
> downloads. The second part of this patch will that will follow soon will add 
> support for the other half, the "copyFromFile" optimization.
> 
> 
> Diffs
> -
> 
>   kioslave/smb/kio_smb.h 55efb44 
>   kioslave/smb/kio_smb_dir.cpp 5573266 
>   kioslave/smb/smb.protocol 654bcfb 
> 
> Diff: http://git.reviewboard.kde.org/r/112982/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

2013-10-22 Thread Frank Reininghaus



> On Oct. 21, 2013, 4:26 p.m., Jan Kundrát wrote:
> > Have you tried a naive implementation with a 
> > std::vector>? You say that a typical use case has 
> > eight entries; that's a very small number where a well-tuned vector could 
> > easily beat the O(1) of QHash or the O(log n) of QMap by its O(n) 
> > semantics. Linear scan might very well turn out to be faster due to its 
> > better cache locality and the associated memory streaming benefits. The STL 
> > class has a lower overhead than an implicitly shared QVector (and you do 
> > not need implicit sharing of the actual entries, do you?).
> > 
> > Anyway, the point is, if the number is small enough, the big-O notation 
> > does not necessarily matter.
> 
> Milian Wolff wrote:
> This is very true. Regarding std::vector not sharing, this is OK since 
> UDSEntryPrivate _is_ sharing. Also, QString in Field is also shared (and with 
> this patch actually makes use of this). I'd be interested in the performance 
> of a (std::)vector/pair approach.
> 
> Frank Reininghaus wrote:
> I fully agree that big-O is not always the most important thing. However, 
> kioslaves can in principle add many more than 8 fields to a UDSEntry, and I 
> had thought that there is a reason why it has always used a QHash.
> 
> About the "std::vector>" idea: sounds interesting. I 
> would assume that this approach uses more memory than my current patch though 
> (if Value == Field). Note that my patch only adds one pointer to the UDSEntry 
> to keep track of the uint keys (assuming that the QHash is shared with many 
> UDSEntries), i.e., 8 bytes on a 64 bit system.
> 
> In a std::pair, you would add a uint to every field of the 
> entry, and the compiler might actually add some padding to preserve the 
> alignment of the members of the "Field", such that the uint effectively needs 
> 8 bytes for every entry.
> 
> Another approach would be to store a QVector/std::vector 
> and a separate vector containing the uint keys. A linear scan of the keys 
> would be much faster if they are all stored next to each other, right? In a 
> std::vector>, the different keys would be many bytes 
> apart.
> 
> Jan Kundrát wrote:
> Regarding the "reason why it has always used a QHash" -- this might be 
> true, or perhaps a programmer used the first thing which came across their 
> mind. I do this all the time.
> 
> You are right on the analysis of the memory consumption -- storing the 
> keys in the vector (or in the QHash) does have an overhead. The advantage is 
> that it will save you at least one pointer indirection compared to an 
> implicitly shared QMap/QHash/... of keys. That might not be a good choice in 
> this context.
> 
> I do not have any numbers comparing performance of a linear scan over a 
> vector on one hand and a scan of a vector. Your idea might or 
> might not be correct; the memory prefetch in CPUs is known to be an 
> impressive piece of silicon. It's entirely possible that the speed 
> improvement of a single vector would get neutralized by a missing 
> prefetch of the target vector.
> 
> How does your current patch work when you replace a QVector with a 
> std::vector?
> 
> Milian Wolff wrote:
> I just thought some more about it and have a potentially even more 
> performant approach, albeit more complicated code-wise. Food for thought 
> though:
> 
> Just use a std::vector, but add the uint (uds field) to Field. 
> Then encapsulate m_str and m_long in a union and add a bool m_isStr or 
> similar. I.e.:
> 
> struct Field {
>   // ctors ...
>   union { // should be sizeof 8
> QString m_str;
> long long m_long;
>   };
>   uint m_uds; // sizeof 4 but would incur a 4byte padding
>   bool m_isStr; // partially fill padding
> };
> 
> If I'm not mistaken, this has the same size as the current Field struct. 
> Then you'd fill this in a std::vector which gets sorted after it is filled by 
> m_uds. In the lookup functions you can then use binary searches (though at an 
> estimated size of 8, a simple linear search might perform better).
> 
> /me would be interested in the performance of this approach. Note that 
> you should still keep the QString cache to use implicit sharing there. The 
> benefit here is that you have _no_ overhead as far as I can see.
> 
> Cheers

Unfortunately, you cannot put a QString (or any other type with non-trivial 
constructor/destructor) into a union. It's easy to see why: when destructing 
such an object, how can the compiler know if the 8-byte object that is stored 
in the union is the d-pointer of a QString or a long long?


- Frank


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113355/#review42115
---


On Oct.

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

2013-10-22 Thread Frank Reininghaus



> On Oct. 21, 2013, 7:36 p.m., Mark Gaiser wrote:
> > Oh wow! When reading through the comments i was quite surprised to see a 
> > question for me :)
> > A very nice discussion btw!
> > 
> > As for the benchmarking. It's actually already available in a testcase if 
> > you want that: 
> > http://quickgit.kde.org/?p=kdelibs.git&a=blob&h=b5dcdb8f07a5e5dea72e80bcf8237995420cca69&hb=a959675fb3ca4d936cd6f8aeff437a1a444b7c38&f=staging%2Fkio%2Fautotests%2Flistdirtest.cpp
> > 
> > The testcase that i usually run is: 
> > https://gitorious.org/kdirchainrebuild/master/source/1594b3e43e7c75ff5dd3d80d9d06fcfb13de0bbc:
> >  but it isn't self contained and you probably don't have much use for it as 
> > a result of that.
> > 
> > So here is the stuff that is actually useful for you.
> > 
> > BIG FAT WARNING: not tested and written out the top of my head. I've been 
> > playing with KIO too much :)
> > ---
> > #include 
> > #include 
> > 
> > KIO::ListJob* job = 
> > KIO::listDir(QUrl("file:///path/to/your/massive/folder"), 
> > KIO::HideProgressInfo);
> > job->setUiDelegate(0);
> > job->addMetaData("details", "2"); // Default is 2 which means all details. 
> > 0 means just a few essential fields (KIO::UDSEntry::UDS_NAME, 
> > KIO::UDSEntry::UDS_FILE_TYPE and KIO::UDSEntry::UDS_LINK_DEST if it is a 
> > symbolic link. Not provided otherwise.
> > 
> > KIO::UDSEntryList totalEntries;
> > 
> > connect(job, &KIO::ListJob::entries, [&](KIO::Job *, const 
> > KIO::UDSEntryList &entries){
> >   totalEntries += entries;
> >   qDebug() << "Entries received:" << entries.count();
> > });
> > 
> > connect(job, &KIO::ListJob::result, [&](KJob *){
> >   qDebug() << "Total entries:" << totalEntries.count();
> > });
> > ---
> > 
> > You probably need to adjust it slightly, but it should work very easily 
> > with KDE Frameworks 5. You might need to change the signal/slot to the old 
> > syntax though, most certainly for KDE 4.x. It's not much but it will show 
> > you the memory usage of those entries. Here storing them is obviously 
> > useless, but in reality - in dolphin - this is done as well only 
> > encapsulated in KFileItem objects.
> > 
> > Good luck!
> > If you need more, just ask.

Thanks! I'll see if I can make a test case that works with both kdelibs master 
and frameworks out of this. The simplicity of your test case makes me look 
forward to the day when we can use C++11 for everything ;-)


- Frank


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113355/#review42128
---


On Oct. 21, 2013, 6:23 p.m., Frank Reininghaus wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113355/
> ---
> 
> (Updated Oct. 21, 2013, 6:23 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> This patch is based on some discussions on the kfm-devel mailing list, see 
> http://lists.kde.org/?t=13793578483&r=1&w=2
> 
> Mark found out that KIO's UDSEntry class is one of the major consumers of 
> memory in applications which use KIO to list directories with a large number 
> of files, and I found a way use implicit sharing to drastically reduce the 
> amount of memory it needs. Many thanks to Milian for his great blog post 
> http://milianw.de/blog/katekdevelop-sprint-vienna-2012-take-1, without which 
> I would probably not have had such ideas.
> 
> 
> 1. The problem
> 
> The UDSEntry keeps all sorts of information about files which can be stored 
> in a string (name, user, group, etc.) or in a long long (file size, 
> modification time, etc.). All these data can be accessed with a uint key, and 
> UDSEntry returns the corresponding QString or long long in O(1) time. 
> Internally, it stores the data in a QHash, where Field is a 
> struct that has a QString and a long long member.
> 
> The problem is that QHash needs quite a lot of memory to provide the O(1) 
> access, see http://doc.qt.digia.com/qq/qq19-containers.html for details, and 
> that the minimum capacity of a QHash seems to be 17, even though the number 
> of entries in a UDSEntry is often 8 in the rather typical standard kio_file 
> use case.
> 
> 
> 2. Proposed solution
> 
> (a) We can store the "Fields" in a QVector, which needs only very 
> little overhead compared to the memory that the actual "Fields" need. When 
> loading a UDSEntry from a QDataStream, we just append all "Fields" to this 
> QVector one by one. Moreover, we need a QHash, which maps each key 
> to the index of its Field in the QVector. This restructuring alone does not 
> reduce the memory usage, of cour

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

2013-10-22 Thread Frank Reininghaus



> On Oct. 21, 2013, 8:20 p.m., Milian Wolff wrote:
> > kio/kio/udsentry.cpp, line 240
> > 
> >
> > you still check that for every "uds" - does it make sense for any 
> > fields besides the ones I listed in my previous comment?

There are fields (like, e.g., the file name, which is different for every 
entry) for which it does not make sense. I was not sure if adding some more 
code to check if the field is "sharable" or not is worth it though (it adds 
some overhead for every "uds", but only saves the QString comparison for some). 
It might be worth checking that, but I can't say if/when I'll manage to do it - 
I'm not sure when I can continue working on this, and the number of suggested 
approaches to rewrite UDSEntryPrivate is already quite large ;-)


> On Oct. 21, 2013, 8:20 p.m., Milian Wolff wrote:
> > kio/kio/udsentry.cpp, line 257
> > 
> >
> > btw instead of clear, reserve I'd replace this with a resize and use 
> > operator[] in the loop below to "fill" the vector. Afaik QVector's clear 
> > actually clears (contrary to std::vector). Thus "my" approach might safe a 
> > call to realloc.

Thanks for the suggestion! Makes sense, but the clearing will in most cases 
only be done for the first entry of a listing (assuming that the "uds" are the 
same for all subsequent ones), and it might therefore not make a big difference.


- Frank


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113355/#review42133
---


On Oct. 21, 2013, 6:23 p.m., Frank Reininghaus wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113355/
> ---
> 
> (Updated Oct. 21, 2013, 6:23 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> This patch is based on some discussions on the kfm-devel mailing list, see 
> http://lists.kde.org/?t=13793578483&r=1&w=2
> 
> Mark found out that KIO's UDSEntry class is one of the major consumers of 
> memory in applications which use KIO to list directories with a large number 
> of files, and I found a way use implicit sharing to drastically reduce the 
> amount of memory it needs. Many thanks to Milian for his great blog post 
> http://milianw.de/blog/katekdevelop-sprint-vienna-2012-take-1, without which 
> I would probably not have had such ideas.
> 
> 
> 1. The problem
> 
> The UDSEntry keeps all sorts of information about files which can be stored 
> in a string (name, user, group, etc.) or in a long long (file size, 
> modification time, etc.). All these data can be accessed with a uint key, and 
> UDSEntry returns the corresponding QString or long long in O(1) time. 
> Internally, it stores the data in a QHash, where Field is a 
> struct that has a QString and a long long member.
> 
> The problem is that QHash needs quite a lot of memory to provide the O(1) 
> access, see http://doc.qt.digia.com/qq/qq19-containers.html for details, and 
> that the minimum capacity of a QHash seems to be 17, even though the number 
> of entries in a UDSEntry is often 8 in the rather typical standard kio_file 
> use case.
> 
> 
> 2. Proposed solution
> 
> (a) We can store the "Fields" in a QVector, which needs only very 
> little overhead compared to the memory that the actual "Fields" need. When 
> loading a UDSEntry from a QDataStream, we just append all "Fields" to this 
> QVector one by one. Moreover, we need a QHash, which maps each key 
> to the index of its Field in the QVector. This restructuring alone does not 
> reduce the memory usage, of course.
> 
> (b) The key observation is that the QDataStream, which KIO::ListJob reads the 
> UDSEntries from, typically provides the different "Fields" in exactly the 
> same order. This means that the QHash is usually exactly the same 
> for every UDSEntry, and we can make use of implicit sharing to store only one 
> copy of this QHash. I've modified
> 
> void UDSEntryPrivate::load(QDataStream &s, UDSEntry &a)
> 
> such that it remembers the most recent QHash and just adds an 
> implicitly shared copy of it to "a" if the order of the Fields has not 
> changed.
> 
> (c) Moreover, some of the QString Fields in the UDSEntries in one directory 
> are often the same, like, e.g., the user and the group. The "load" function 
> also remembers the most recently read values for each Field in a static 
> QVector and just puts an implicitly shared copy into the UDSEntry if 
> possible.
> 
> 
> 3. Possible disadvantages
> 
> (a) When using the "remove" member, the new version of UDSEntry does not 
> remove the Field from the QV

Re: Dependency to unreleased versions

2013-10-22 Thread Raymond Wooninck

On Monday 21 October 2013 19:10:49 Gilles Caulier wrote:
> Problem already reported here :
> 
> https://bugs.kde.org/show_bug.cgi?id=326368
> 
> It's completly different issue. Sound like client libraw code (as
> libkdcraw) need to be linked to libgomp when libraw OpenMP support is
> enabled. As libraw has already resolved OpenMP symbols at packaging
> time, why libkdcraw need again to resolve these ?
> 
> OpenMP support is internal to libraw. Client code as libkdcraw do not
> use OpenMP API. I suspect something wrong in libraw packaging. Under
> Mageia, i cannot reproduce the problem...
> 
Gilles, 

Ben Cooksley asked me to validate this for openSUSE as that build.kde.org is 
running openSUSE 12.3.  libkdcraw always worked fine as that it had an up-to-
date internal libraw tree.

Now with the removal of the internal libraw tree, libkdcraw falls back on the 
libraw that is provided by the distribution. And this causes the above 
indicated problem. 

openSUSE 12.3 is using libraw 0.14.7 for libkdcraw for which the build fails 
with the missing symbols.  if I compile libkdcraw against the latest openSUSE 
Factory (using libraw 0.15.4) then libkdcraw is compiling fine without any 
issues. I validated both libraw packages and the only difference is the libraw 
version. 

So I assume that libkdcraw needs libraw > 0.14.7 

Regards

Raymond
openSUSE KDE community maintainer

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

2013-10-22 Thread Milian Wolff



> On Oct. 21, 2013, 4:26 p.m., Jan Kundrát wrote:
> > Have you tried a naive implementation with a 
> > std::vector>? You say that a typical use case has 
> > eight entries; that's a very small number where a well-tuned vector could 
> > easily beat the O(1) of QHash or the O(log n) of QMap by its O(n) 
> > semantics. Linear scan might very well turn out to be faster due to its 
> > better cache locality and the associated memory streaming benefits. The STL 
> > class has a lower overhead than an implicitly shared QVector (and you do 
> > not need implicit sharing of the actual entries, do you?).
> > 
> > Anyway, the point is, if the number is small enough, the big-O notation 
> > does not necessarily matter.
> 
> Milian Wolff wrote:
> This is very true. Regarding std::vector not sharing, this is OK since 
> UDSEntryPrivate _is_ sharing. Also, QString in Field is also shared (and with 
> this patch actually makes use of this). I'd be interested in the performance 
> of a (std::)vector/pair approach.
> 
> Frank Reininghaus wrote:
> I fully agree that big-O is not always the most important thing. However, 
> kioslaves can in principle add many more than 8 fields to a UDSEntry, and I 
> had thought that there is a reason why it has always used a QHash.
> 
> About the "std::vector>" idea: sounds interesting. I 
> would assume that this approach uses more memory than my current patch though 
> (if Value == Field). Note that my patch only adds one pointer to the UDSEntry 
> to keep track of the uint keys (assuming that the QHash is shared with many 
> UDSEntries), i.e., 8 bytes on a 64 bit system.
> 
> In a std::pair, you would add a uint to every field of the 
> entry, and the compiler might actually add some padding to preserve the 
> alignment of the members of the "Field", such that the uint effectively needs 
> 8 bytes for every entry.
> 
> Another approach would be to store a QVector/std::vector 
> and a separate vector containing the uint keys. A linear scan of the keys 
> would be much faster if they are all stored next to each other, right? In a 
> std::vector>, the different keys would be many bytes 
> apart.
> 
> Jan Kundrát wrote:
> Regarding the "reason why it has always used a QHash" -- this might be 
> true, or perhaps a programmer used the first thing which came across their 
> mind. I do this all the time.
> 
> You are right on the analysis of the memory consumption -- storing the 
> keys in the vector (or in the QHash) does have an overhead. The advantage is 
> that it will save you at least one pointer indirection compared to an 
> implicitly shared QMap/QHash/... of keys. That might not be a good choice in 
> this context.
> 
> I do not have any numbers comparing performance of a linear scan over a 
> vector on one hand and a scan of a vector. Your idea might or 
> might not be correct; the memory prefetch in CPUs is known to be an 
> impressive piece of silicon. It's entirely possible that the speed 
> improvement of a single vector would get neutralized by a missing 
> prefetch of the target vector.
> 
> How does your current patch work when you replace a QVector with a 
> std::vector?
> 
> Milian Wolff wrote:
> I just thought some more about it and have a potentially even more 
> performant approach, albeit more complicated code-wise. Food for thought 
> though:
> 
> Just use a std::vector, but add the uint (uds field) to Field. 
> Then encapsulate m_str and m_long in a union and add a bool m_isStr or 
> similar. I.e.:
> 
> struct Field {
>   // ctors ...
>   union { // should be sizeof 8
> QString m_str;
> long long m_long;
>   };
>   uint m_uds; // sizeof 4 but would incur a 4byte padding
>   bool m_isStr; // partially fill padding
> };
> 
> If I'm not mistaken, this has the same size as the current Field struct. 
> Then you'd fill this in a std::vector which gets sorted after it is filled by 
> m_uds. In the lookup functions you can then use binary searches (though at an 
> estimated size of 8, a simple linear search might perform better).
> 
> /me would be interested in the performance of this approach. Note that 
> you should still keep the QString cache to use implicit sharing there. The 
> benefit here is that you have _no_ overhead as far as I can see.
> 
> Cheers
> 
> Frank Reininghaus wrote:
> Unfortunately, you cannot put a QString (or any other type with 
> non-trivial constructor/destructor) into a union. It's easy to see why: when 
> destructing such an object, how can the compiler know if the 8-byte object 
> that is stored in the union is the d-pointer of a QString or a long long?

Doh! Of course you are right. Too bad ;-)

Though I do wonder what would happen if one did a

~Field() {
  if (m_isStr) {
m_str.~QString();
  }
}

Ugly as hell though, so definitely not worth it.


- Milian

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

2013-10-22 Thread Milian Wolff



> On Oct. 21, 2013, 8:20 p.m., Milian Wolff wrote:
> > kio/kio/udsentry.cpp, line 240
> > 
> >
> > you still check that for every "uds" - does it make sense for any 
> > fields besides the ones I listed in my previous comment?
> 
> Frank Reininghaus wrote:
> There are fields (like, e.g., the file name, which is different for every 
> entry) for which it does not make sense. I was not sure if adding some more 
> code to check if the field is "sharable" or not is worth it though (it adds 
> some overhead for every "uds", but only saves the QString comparison for 
> some). It might be worth checking that, but I can't say if/when I'll manage 
> to do it - I'm not sure when I can continue working on this, and the number 
> of suggested approaches to rewrite UDSEntryPrivate is already quite large ;-)

Hehe yeah :) Most of my musings here can be seen as food for thought.


- Milian


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113355/#review42133
---


On Oct. 21, 2013, 6:23 p.m., Frank Reininghaus wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113355/
> ---
> 
> (Updated Oct. 21, 2013, 6:23 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> This patch is based on some discussions on the kfm-devel mailing list, see 
> http://lists.kde.org/?t=13793578483&r=1&w=2
> 
> Mark found out that KIO's UDSEntry class is one of the major consumers of 
> memory in applications which use KIO to list directories with a large number 
> of files, and I found a way use implicit sharing to drastically reduce the 
> amount of memory it needs. Many thanks to Milian for his great blog post 
> http://milianw.de/blog/katekdevelop-sprint-vienna-2012-take-1, without which 
> I would probably not have had such ideas.
> 
> 
> 1. The problem
> 
> The UDSEntry keeps all sorts of information about files which can be stored 
> in a string (name, user, group, etc.) or in a long long (file size, 
> modification time, etc.). All these data can be accessed with a uint key, and 
> UDSEntry returns the corresponding QString or long long in O(1) time. 
> Internally, it stores the data in a QHash, where Field is a 
> struct that has a QString and a long long member.
> 
> The problem is that QHash needs quite a lot of memory to provide the O(1) 
> access, see http://doc.qt.digia.com/qq/qq19-containers.html for details, and 
> that the minimum capacity of a QHash seems to be 17, even though the number 
> of entries in a UDSEntry is often 8 in the rather typical standard kio_file 
> use case.
> 
> 
> 2. Proposed solution
> 
> (a) We can store the "Fields" in a QVector, which needs only very 
> little overhead compared to the memory that the actual "Fields" need. When 
> loading a UDSEntry from a QDataStream, we just append all "Fields" to this 
> QVector one by one. Moreover, we need a QHash, which maps each key 
> to the index of its Field in the QVector. This restructuring alone does not 
> reduce the memory usage, of course.
> 
> (b) The key observation is that the QDataStream, which KIO::ListJob reads the 
> UDSEntries from, typically provides the different "Fields" in exactly the 
> same order. This means that the QHash is usually exactly the same 
> for every UDSEntry, and we can make use of implicit sharing to store only one 
> copy of this QHash. I've modified
> 
> void UDSEntryPrivate::load(QDataStream &s, UDSEntry &a)
> 
> such that it remembers the most recent QHash and just adds an 
> implicitly shared copy of it to "a" if the order of the Fields has not 
> changed.
> 
> (c) Moreover, some of the QString Fields in the UDSEntries in one directory 
> are often the same, like, e.g., the user and the group. The "load" function 
> also remembers the most recently read values for each Field in a static 
> QVector and just puts an implicitly shared copy into the UDSEntry if 
> possible.
> 
> 
> 3. Possible disadvantages
> 
> (a) When using the "remove" member, the new version of UDSEntry does not 
> remove the Field from the QVector. This means that removing and adding 
> a "Field" repeatedly would let the memory usage grow indefinitely. According 
> to David (http://lists.kde.org/?l=kfm-devel&m=138052519927973&w=2), this 
> doesn't matter though because no known user of UDSEntry uses its remove() 
> member. Maybe we should remove remove (pun stolen grom David) in the 
> frameworks branch then?
> 
> (b) In principle, the old version of UDSEntryPrivate::load(QDataStream&, 
> UDSEntry&) was reentrant. This is not the case for my changed version. 
> Reentrancy could be res

Re: FindKDE4Internal.cmake & clang & c++11 & -fdelayed-template-parsing

2013-10-22 Thread Mark Kretschmann

On Sat, Oct 12, 2013 at 6:46 PM, Thiago Macieira  wrote:
> On sábado, 12 de outubro de 2013 12:42:50, Milian Wolff wrote:
>> Hey Raphael!
>>
>> Thank you for working on clang support in FindKDE4Internal.cmake.
>>
>> Since recently though I have build issues with clang due to the -fdelayed-
>> template-parsing flag passed in FindKDE4Internal.cmake. A simple example
>> such as this:
>>
>> #include 
>> int main() { std::vector v; return 0; }
>>
>> produces with: clang++ -std=c++11 -fdelayed-template-parsing test.cpp
>
> Remove it forever. It's not necessary in proper C++ code.
>
> It's only necessary to emulate buggy MSVC behaviour, which we should only use
> for Clang on Windows and then only if we run into trouble compiling code
> coming from Microsoft.

Yes, please remove the flag. Amarok has recently started using C++11,
and we ran into the same issue.

The culprit is btw this: http://llvm.org/bugs/show_bug.cgi?id=15651

-- 
Mark Kretschmann
Amarok Developer
Fellow of the Free Software Foundation Europe
http://amarok.kde.org - http://fsfe.org

Re: FindKDE4Internal.cmake & clang & c++11 & -fdelayed-template-parsing

2013-10-22 Thread Raphael Kubo da Costa

Milian Wolff  writes:

> Hey Raphael!
>
> Thank you for working on clang support in FindKDE4Internal.cmake.
>
> Since recently though I have build issues with clang due to the -fdelayed-
> template-parsing flag passed in FindKDE4Internal.cmake. A simple example such
> as this:
>
> #include 
> int main() { std::vector v; return 0; }
>
> produces with: clang++ -std=c++11 -fdelayed-template-parsing test.cpp
[...]
> Apparently, delayed template parsing is not yet mature. So - can we disable it
> in FindKDE4Internal.cmake again? Or do you know a workaround for that?

Hi,

Sorry for not answering earlier.

As explained in the comment in FindKDE4Internal.cmake, simply not
passing -fdelayed-template-parsing causes a lot of problems due to us
building most repositories with -fno-exceptions and including template
code that does not get instantiated and throws exceptions: for example,
kdepimlibs' akonadi/itempayloadinternals_p.h throws some exceptions in
code that should not be reached, and while that code is built with
exceptions enabled, kdepim may end up indirectly including that header
without ever knowing it needs to enable exceptions in the first place
(which is what happened when I was testing clang support). The same
thing happened with kdelibs and OpenEXR too.

Contrary to GCC, MSVC and ICC, clang complains and fails to build if
that happens (and is probably doing the right thing).

One option is to fix all places where this might arise (it's manual and
error-prone work), or just remove -fno-exceptions from the default
compiler flags for clang in FindKDE4Internal.cmake. I tend ot favor the
latter, but it will increase the size of the generated binaries (thiago
estimated them to increase by ~5-7%).

Re: Dependency to unreleased versions

2013-10-22 Thread Rex Dieter

Alexander Neundorf wrote:

> On Sunday 20 October 2013, Torgny Nyblom wrote:
>> Hi,
>> 
>> What is the policy of depending on unreleased libraries? And is that
>> written down somewhere?
...
> IMO it's a pain to depend on unreleased versions of anything.

As this seems to be semi-regular occurrence, I concur this pain can and 
should be avoided by policy.  

-- Rex

KF5 Update Meeting Minutes 2013-w43

2013-10-22 Thread Kevin Ottens

Hello everyone,

This is the minutes of the Week 43 KF5 meeting. As usual it has been held on 
#kde-devel at 4pm Paris time.

Were present: afiestas, agateau, dfaure, dMaggot, jpwhiting, mck182, 
PovAddict, sebas, teo, vHanda and myself.

Announcement:
 * All new code should use qCDebug and friends instead of qDebug:
   http://community.kde.org/index.php?title=Frameworks/Porting_To_qCDebug

Topics discussed:
 * afiestas is finishing the kded/ksycoca move;
 * he's also replacing Solid::PowerManagement with newer API;

 * agateau brought back the notification support of KMessageBox;
 * he's fixing the standalone build of tier1 frameworks;
 * he's also cleaning up kcompletion, ki18n, kdoctools, knotifications and 
dbusmenu-qt;

 * dfaure is killing kio one stab at a time (which means among other things 
moving KFileDialog to kde4support);
 * only KImageIO and kdbusservicestarter are left;

 * dMaggot found only already done tasks in khtml;
 * he's planning to help agateau for ter2 standalone build fixes;

 * jpwhiting is removing the KImageIO dependency of knewstuff;

 * mck182 is working on the workspace (ksplash in particular);

 * PovAddict is working on getting things to build on windows (very important 
work! help needed!);
 * Effort tracking there: http://community.kde.org/Frameworks/Windows

 * sebas renamed KF5::plasma to KF5::Plasma
 * he's also working on build fixes;
 * he's also tackling the network and systray applets;

 * teo is done with KEncodingFileDialog;
 * he's still working on ksmserver;

 * vHanda found out kross and kjsembed as unmaintained;
 * he's also getting nepomuk ready for frameworks;

 * ervin emptying KDE4Attic, almost there.

If you got questions, feel free to ask.

Regards.
-- 
Kévin Ottens, http://ervin.ipsquad.net

Sponsored by KDAB to work on KDE Frameworks
KDAB - proud supporter of KDE, http://www.kdab.com



signature.asc
Description: This is a digitally signed message part.

Re: Review Request 113260: Port KTimeZoned to Qt5/KF5

2013-10-22 Thread Martin Klapetek


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113260/
---

(Updated Oct. 22, 2013, 4:49 p.m.)


Review request for KDE Runtime, KDE Frameworks, Plasma, and John Layt.


Changes
---

* Watch the whole timezone database dir
* Rename signal to timeZoneDatabaseUpdated()
  * Windows support is to be added later by John (I have no way to test)
* The Solaris support remains removed. Yes, Solaris still has its users, but 
they are using KDE4 and will keep so for some time. IF PlasmaWorkspaces2 will 
get Solaris support (so that it all builds and runs), I hereby commit to add 
proper Solaris support to ktimezoned at that time, otherwise there's no point 
in keeping old (now untested) code around to bitrot and add maintenance burden, 
especially since there is 0 effort to have other, more important components 
work on Solaris (including Frameworks).


Repository: kde-runtime


Description
---

Originally I wanted to port KTimeZoned 1:1 to Qt5/KF5, but then I found out 
that all the stuff KTZD was doing was added in QTimeZone, that includes reading 
correct system files/env variables to obtain the timezone and returning the 
proper system zone (KTZD did all this by itself). It also doesn't need to parse 
the timezone files itself or watch for their changes as QTimeZone objects are 
not stored.

So now it's just a thin module watching /etc/timezone (used by Debian-based 
distros) and /etc/localtime (used by eg. Fedora or Suse, but also by Debian in 
conjunction with /etc/timezone) for changes and if it detects a change, it 
checks if the new timezone is really different and if it is, it sends out a 
DBus signal "timeZoneChange". I changed it from "configChanged" as I think 
"timeZoneChanged" makes way more sense.

I didn't touch the Windows part as I have no way to test, would be nice if 
someone could help with that.

EDIT: I removed the other two DBus signals which were not used and I'm unsure 
KTZD is the correct place for that now anyway. The only usage in 
KSystemTimeZone can be replaced by own KDirWatch instance.


Diffs (updated)
-

  CMakeLists.txt a5ec93d 
  ktimezoned/CMakeLists.txt bafc85e 
  ktimezoned/ktimezoned.h ff21807 
  ktimezoned/ktimezoned.cpp f380c09 
  ktimezoned/ktimezoned_win.h 26e21cc 
  ktimezoned/ktimezoned_win.cpp cadfe3a 
  ktimezoned/ktimezonedbase.h ca00aca 
  ktimezoned/org.kde.KTimeZoned.xml daaa0b7 

Diff: http://git.reviewboard.kde.org/r/113260/diff/


Testing
---

Tested by changing the timezone in different ways, change was detected and 
signalled out.


Thanks,

Martin Klapetek

Re: Review Request 112991: Fix compilation rules of KDE-Workspace under OSX/Macports

2013-10-22 Thread Gilles Caulier


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112991/#review42169
---


Martin,

KDE 4.11 or master (Qt5) _must_ compile under OSX. My current patch work fine 
against master and can be applied to git repository.

I currently working to adjust this patch with KDE 4.11 branch.

I know that most of developers work under Linux and X11 based system. But this 
must not be a portability limitation, especially when people provide a patch on 
other platform. After all, Windows is also supported by KDE-Workspace, why not 
OSX. My patch simply reproduce compilation rules defined by Windows to OSX. 

Gilles Caulier

- Gilles Caulier


On Sept. 29, 2013, 5:15 p.m., Gilles Caulier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112991/
> ---
> 
> (Updated Sept. 29, 2013, 5:15 p.m.)
> 
> 
> Review request for kde-workspace.
> 
> 
> Bugs: https://trac.macports.org/ticket/33780
> http://bugs.kde.org/show_bug.cgi?id=https://trac.macports.org/ticket/33780
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> This patch fix broken compilation under OSX / macports about kde-workspace.
> 
> Patch do not touch implementation. Only compilation rules are changed in 
> cmake script to follow the way way than Windows rules, where no X11 lib are 
> available.
> 
> By this way, Oxygen is compiled and installed to macport and digiKam has a 
> suitable GUI under OSX.
> 
> See my Macports bug report for details : 
> https://trac.macports.org/ticket/33780 
> 
> Gilles Caulier
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt c37ab8b 
>   kcontrol/CMakeLists.txt a25aaa0 
>   libs/CMakeLists.txt 9d71a03 
> 
> Diff: http://git.reviewboard.kde.org/r/112991/diff/
> 
> 
> Testing
> ---
> 
> I tested this patch under my macbook pro, using a fresh install of Macports 
> (KDE 4.11.1 / Qt 4.8.5)
> 
> As kde-workspace macports package is broken, i checkout code from KDE 
> git/master repository and fixed compilation rules as well. 
> 
> 
> Thanks,
> 
> Gilles Caulier
> 
>

Re: Review Request 113366: kio_http: small optimization in response code parsing

2013-10-22 Thread Andrea Iacovitti


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113366/
---

(Updated Oct. 22, 2013, 8:06 p.m.)


Status
--

This change has been marked as submitted.


Review request for kdelibs and Dawit Alemayehu.


Repository: kdelibs


Description
---

Execute the long "if...else if..." only if the previous condition "if 
(m_request.responseCode != 200 && m_request.responseCode != 304)" is verified.
If it fails (that is responseCode == 200 or responseCode == 304) none of the 
nine conditions in the subsequent if..else if.. can be true.


Diffs
-

  kioslave/http/http.cpp 81182eb 

Diff: http://git.reviewboard.kde.org/r/113366/diff/


Testing
---


Thanks,

Andrea Iacovitti

Re: Review Request 113366: kio_http: small optimization in response code parsing

2013-10-22 Thread Commit Hook


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113366/#review42185
---


This review has been submitted with commit 
75a4d5deaa9e0d263d37514292cab36e9e895c2e by Andrea Iacovitti to branch KDE/4.11.

- Commit Hook


On Oct. 21, 2013, 3:23 p.m., Andrea Iacovitti wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113366/
> ---
> 
> (Updated Oct. 21, 2013, 3:23 p.m.)
> 
> 
> Review request for kdelibs and Dawit Alemayehu.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Execute the long "if...else if..." only if the previous condition "if 
> (m_request.responseCode != 200 && m_request.responseCode != 304)" is verified.
> If it fails (that is responseCode == 200 or responseCode == 304) none of the 
> nine conditions in the subsequent if..else if.. can be true.
> 
> 
> Diffs
> -
> 
>   kioslave/http/http.cpp 81182eb 
> 
> Diff: http://git.reviewboard.kde.org/r/113366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrea Iacovitti
> 
>

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

2013-10-22 Thread Alexander Richardson



> On Oct. 21, 2013, 6:26 p.m., Jan Kundrát wrote:
> > Have you tried a naive implementation with a 
> > std::vector>? You say that a typical use case has 
> > eight entries; that's a very small number where a well-tuned vector could 
> > easily beat the O(1) of QHash or the O(log n) of QMap by its O(n) 
> > semantics. Linear scan might very well turn out to be faster due to its 
> > better cache locality and the associated memory streaming benefits. The STL 
> > class has a lower overhead than an implicitly shared QVector (and you do 
> > not need implicit sharing of the actual entries, do you?).
> > 
> > Anyway, the point is, if the number is small enough, the big-O notation 
> > does not necessarily matter.
> 
> Milian Wolff wrote:
> This is very true. Regarding std::vector not sharing, this is OK since 
> UDSEntryPrivate _is_ sharing. Also, QString in Field is also shared (and with 
> this patch actually makes use of this). I'd be interested in the performance 
> of a (std::)vector/pair approach.
> 
> Frank Reininghaus wrote:
> I fully agree that big-O is not always the most important thing. However, 
> kioslaves can in principle add many more than 8 fields to a UDSEntry, and I 
> had thought that there is a reason why it has always used a QHash.
> 
> About the "std::vector>" idea: sounds interesting. I 
> would assume that this approach uses more memory than my current patch though 
> (if Value == Field). Note that my patch only adds one pointer to the UDSEntry 
> to keep track of the uint keys (assuming that the QHash is shared with many 
> UDSEntries), i.e., 8 bytes on a 64 bit system.
> 
> In a std::pair, you would add a uint to every field of the 
> entry, and the compiler might actually add some padding to preserve the 
> alignment of the members of the "Field", such that the uint effectively needs 
> 8 bytes for every entry.
> 
> Another approach would be to store a QVector/std::vector 
> and a separate vector containing the uint keys. A linear scan of the keys 
> would be much faster if they are all stored next to each other, right? In a 
> std::vector>, the different keys would be many bytes 
> apart.
> 
> Jan Kundrát wrote:
> Regarding the "reason why it has always used a QHash" -- this might be 
> true, or perhaps a programmer used the first thing which came across their 
> mind. I do this all the time.
> 
> You are right on the analysis of the memory consumption -- storing the 
> keys in the vector (or in the QHash) does have an overhead. The advantage is 
> that it will save you at least one pointer indirection compared to an 
> implicitly shared QMap/QHash/... of keys. That might not be a good choice in 
> this context.
> 
> I do not have any numbers comparing performance of a linear scan over a 
> vector on one hand and a scan of a vector. Your idea might or 
> might not be correct; the memory prefetch in CPUs is known to be an 
> impressive piece of silicon. It's entirely possible that the speed 
> improvement of a single vector would get neutralized by a missing 
> prefetch of the target vector.
> 
> How does your current patch work when you replace a QVector with a 
> std::vector?
> 
> Milian Wolff wrote:
> I just thought some more about it and have a potentially even more 
> performant approach, albeit more complicated code-wise. Food for thought 
> though:
> 
> Just use a std::vector, but add the uint (uds field) to Field. 
> Then encapsulate m_str and m_long in a union and add a bool m_isStr or 
> similar. I.e.:
> 
> struct Field {
>   // ctors ...
>   union { // should be sizeof 8
> QString m_str;
> long long m_long;
>   };
>   uint m_uds; // sizeof 4 but would incur a 4byte padding
>   bool m_isStr; // partially fill padding
> };
> 
> If I'm not mistaken, this has the same size as the current Field struct. 
> Then you'd fill this in a std::vector which gets sorted after it is filled by 
> m_uds. In the lookup functions you can then use binary searches (though at an 
> estimated size of 8, a simple linear search might perform better).
> 
> /me would be interested in the performance of this approach. Note that 
> you should still keep the QString cache to use implicit sharing there. The 
> benefit here is that you have _no_ overhead as far as I can see.
> 
> Cheers
> 
> Frank Reininghaus wrote:
> Unfortunately, you cannot put a QString (or any other type with 
> non-trivial constructor/destructor) into a union. It's easy to see why: when 
> destructing such an object, how can the compiler know if the 8-byte object 
> that is stored in the union is the d-pointer of a QString or a long long?
> 
> Milian Wolff wrote:
> Doh! Of course you are right. Too bad ;-)
> 
> Though I do wonder what would happen if one did a
> 
> ~Field() {
>   if (m_isStr) {
> m_str.~QString();
>

Re: Review Request 112848: Adding unit tests for kbuttongroup and fixing identation

Re: Review Request 112848: Adding unit tests for kbuttongroup and fixing identation

Re: Review Request 112982: copyToFile support for kio_smb

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

Re: Dependency to unreleased versions

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

Re: FindKDE4Internal.cmake & clang & c++11 & -fdelayed-template-parsing

Re: FindKDE4Internal.cmake & clang & c++11 & -fdelayed-template-parsing

Re: Dependency to unreleased versions

KF5 Update Meeting Minutes 2013-w43

Re: Review Request 113260: Port KTimeZoned to Qt5/KF5

Re: Review Request 112991: Fix compilation rules of KDE-Workspace under OSX/Macports

Re: Review Request 113366: kio_http: small optimization in response code parsing

Re: Review Request 113366: kio_http: small optimization in response code parsing

Re: Review Request 113355: Reduce UDSEntry memory usage with implicit sharing

18 matches

Site Navigation

Mail list logo

Footer information