enhance performance at updating KWallet Editor Tree
Review Request #105633 - Created July 20, 2012 and discarded
I have > 340 entries in "Form Data" in the wallet. When opening the wallet editor, it takes about 9 Seconds until the tree is displayed! I investigated the problem and found out that this is due to 2 reasons: 1.) The list of entries is checked against existing entries in the tree, which is done by linear search in the given entries list and in the tree which results in a quadratic complexity. 2.) unneeded duplicate dbus calls I solved the first by using QSet for fast lookup and I reduced the second problem by avoiding a duplicate, unneeded query over dbus
|This is not your fault as it was that way before, but shouldn't this be translatable? I guess RTL languages ...||Rolf Eike Beer|
|Well, it doesn't really refresh anymore, if you use a cache :-) The whole point of this method is to ...||David Faure|
|All this was useful when updateEntries was called too often. Now that it's only called on actual changes, does it ...||David Faure|
|This is the actual bugfix -- this method was wrongly assuming the item actually got renamed. But then, if it ...||David Faure|
Review request changed
Found one other major performance problem: when filling the tree, EVERY NEW tree element emitted the itemChanged() signal, which led to calling KWalletEditor::listItemRenamed() which sent a dbus message to the kwallted to "rename" the item. This is now avoided by simply doing an old/new comparison for the name.
Revision 2 (+26 -12)
Long-standing open reviews are bad, they lead to duplicate work :(
Well, it doesn't really refresh anymore, if you use a cache :-) The whole point of this method is to get the new count from the daemon. The problem was that it was called far too often -- I just fixed that [and you had too, below... I hadn't seen this pending review before investigating the bug... sigh]
All this was useful when updateEntries was called too often. Now that it's only called on actual changes, does it matter as much?
This is the actual bugfix -- this method was wrongly assuming the item actually got renamed. But then, if it wasn't renamed, there's no reason to call setText() back either. I committed my fix for this (to 4.9 and master, see bug 279161 for ref). I think this change request can be discarded, apart maybe from the QSet which could help lower CPU usage when adding something to the wallet while kwalletmanager is running, or when renaming an entry in kwalletmanager.
I'm really sorry for not saying anything so far, I'm not familiar with this code base so I thought it'd be better for someone else to comment. Meanwhile, dfaure has pushed a fix for bug 279161. Is there any part of this review request that still applies?