[Kmymoney-devel] AccountsModel loadInstitution and loadSubaccounts recursiveness

[Cristian Oneţ]

On Sat, Sep 25, 2010 at 5:06 PM, Alvaro Soliverez <asoliverez at kde.org> wrote:
> On Sat, Sep 25, 2010 at 10:39 AM, Cristian Oneţ <onet.cristian at gmail.com> wrote:
>> On Sat, Sep 25, 2010 at 4:11 PM, Alvaro Soliverez <asoliverez at kde.org> wrote:
>>> On Sat, Sep 25, 2010 at 9:41 AM, Cristian Oneţ <onet.cristian at gmail.com> wrote:
>>>> On Tue, Sep 21, 2010 at 6:16 PM, Cristian Oneţ <onet.cristian at gmail.com> wrote:
>>>>> On Tue, Sep 21, 2010 at 6:06 PM, Alvaro Soliverez <asoliverez at kde.org> wrote:
>>>>>> On Mon, Sep 20, 2010 at 11:15 AM, Cristian Oneţ <onet.cristian at gmail.com> wrote:
>>>>>>> On Sun, Sep 19, 2010 at 4:44 PM, Alvaro Soliverez <asoliverez at kde.org> wrote:
>>>>>>>> Hello Cristian,
>>>>>>> Hi Alvaro,
>>>>>>>
>>>>>>>> I'm checking the results of the callgrind profiling.
>>>>>>>>
>>>>>>>> I've found some oddness.
>>>>>>>>
>>>>>>>> There are two methods in the AccountsModel that get called and incur
>>>>>>>> the most time, loadSubAccounts and loadInstitution.
>>>>>>>> The AccountModel::load() method calls both, but in turn, the
>>>>>>>> loadSubAccounts method calls loadInstitution too.
>>>>>>>>
>>>>>>>> I haven't looked at it in enough detail to verify if that's correct or
>>>>>>>> it can be avoided, because I was looking for something else.
>>>>>>>>
>>>>>>>> Could you look into it and check whether that's correct?
>>>>>>>
>>>>>>> Yes it is correct. The AccountModel::loadInstitution function creates
>>>>>>> the needed items in the model for the institutions view for each
>>>>>>> account (that is why it's called from load sub-accounts also). The
>>>>>>> best way to optimize the accounts model (and other parts of the
>>>>>>> application) would be to add more fine grained change signaling then
>>>>>>> the now existing dataChanged signal. So until we don't know what has
>>>>>>> changed we need to sync all the data between the model and the data
>>>>>>> storage and that takes time. I still need to take a look at this if we
>>>>>>> can gain some speed without doing the extra development of changing
>>>>>>> dataChanged but I think the time has come to do just that. Because
>>>>>>> that will be needed in the ledger also where for example everything is
>>>>>>> reloaded if the status of the transaction is changed.
>>>>>>>
>>>>>>
>>>>>> Is there any expensive calculation on the model, besides retrieving
>>>>>> the balance, which should be handled by the balance cache?
>>>>>
>>>>> Not that I know of. If rebuilding the account tree on each data
>>>>> changed is not expensive then it could be that the balance computation
>>>>> is and with the cache it can be improved.
>>>>
>>>> After running some tests it seems that synchronizing the account tree
>>>> is the most expensive operation so adding the balance cache wont bring
>>>> big speed improvements in this area but a more fine grained data
>>>> change signal would. I will start to work on a patch that implements
>>>> this.
>>>>
>>>
>>> What does this synchronizing do? Can you handle it with a signal which
>>> is only emmited when an account is added/modified/rdeleted?
>>
>> Synchronizing means to sync the data between the model and the
>> underlying storage. If the storage signals that data has changed the
>> accounts model must reload (not an actual reload but more of a
>> synchronization) to contain the new data. To fix this it would be nice
>> to add account added/modified/deleted signals to the MyMoneyFileObject
>> similar signals should be added for other types of changes (changes
>> that affect the balance or the value for instance). So this is a
>> pretty big task and I don't know if it will be worth it.
>>
>
> How about adding two sets of signals? One that is emitted whenever
> there is an account change, and another that covers transactions?
I don't know if it would but it would be a solution only for this
model and not a complete solution. The complete solution would be to
have signals for each mymoney objects that are changed but adding such
a facility is a pretty big task (I've just checked) and I don't know
if it is worth it.

> Also, I've found another issue, which should be probably solved elsewhere.
>
> The accountModel calls balance(accountId).  That, in turn, adds a
> default QDate() when calling the engine. That prevents the cache from
> being used, even if possible.
The cache is not used but the balance is taken directly from the
account which is O(1). 'return m_accountList[id].balance();'

> So, every call for balance reloads all transactions for that account,
> even if it could use the today's balance that is already cached.
>
> Thomas can confirm if my observations are true. If so, I would change,
> where possible, the calls to balance() to get today's balance.
As I've said before the calls to balance() and value() are only lets
say 20% of the total duration of the function load() which is OK if we
consider that they load data for 2 of the 6 columns.
The problem here is that load() is called on every small change but I
can't think of another way to do it with what we have right now.

Regards,
Cristian