I was working through it; I think this will ideally require an upgrade to the xaccAccountRecomputeBalance function to also keep track of closing balances. Then the estimate_budget_helper will choose the appropriate xaccAccountBalance function.

Alternatively (more ugly) the estimate_budget_helper will launch a qof_query to retrieve closing splits in each account, add them, and minus that amount from the account delta amount.

IMHO the budget reports should also be upgraded to offer ignore closing entries.

having fixed bug 797390 this is now committed in maint for 3.8
https://github.com/Gnucash/gnucash/commit/8b8c957e

oh fwiw contrary to the bug report there's no closing-entries string pattern matching; the new estimate will ignore closing-entries tagged properly with kvp flag. this will be fine for all closing-entries entered from 2008 onwards -- see the original https://github.com/Gnucash/gnucash/commit/4b28001459d93e078a1dd729d41302eac049ab0c

Hmm needs further work. budget reports will still include closing-entries.

gnc-budget-get-account-period-actual-value needs to be able to ignore them in reports too. this is related to, but outside scope for this bug, so, no need to reopen.

Ok @jralls and @gjanssens a quick'n'easy fix to ensure budget reports pick up actual values ignore closing is:

* gnc-budget.c gnc_budget_get_account_period_actual_value is only used by budget reports
* Recurrence.c  recurrenceGetAccountPeriodValue is only used by gnc_budget_get_account_period_actual_value
* Recurrence.c recurrenceGetAccountPeriodValue can call xaccAccountGetNoclosingBalanceChangeForPeriod instead of xaccAccountGetBalanceChangeForPeriod which already exists.

Basically it's an API change instead of refactoring budget reports.

The 2nd method is to refactor budget reports to query accounts' balances ignoring closing - I can make it faster too using gnc:account-get-balances-at-dates as well.

Opinions?

Created attachment 373433 [details]
implements both changes

option 1: modify C Recurrence.c to modify function which is only used by reports. one-line fix.

option 2: modify reports to choose different methods to query accounts. this means some C can be deprecated, and reports can be faster.

Comment on attachment 373433 [details]
implements both changes

Aside from the confusing naming (gnc:gnc-.*; yes, I see that Bill Gribble perpetrated the original offense of that sort 19 years ago, but still...), what's the advantage of the Scheme function over the C one?

see the very fluid branch https://github.com/christopherlam/gnucash/tree/maint-797326-bis

* the scm function scans account only once instead of twice to get delta amount
* scm can pre-generate accounts' balances at period date boundaries, and requery the balances for each period.

I think I'll need to pre-generate account balances *twice* -- one for period-start-balance, second for period-end-balance.

Main advantage of schemifying the retrieval of account's periods actuals is we don't need to redefine C API, and can pre-generate accounts balance-list. 

Consider a very large asset/liability/income/expense account with future-oriented budgets. Generating actuals from C for multiple periods means each account is scanned via xaccAccountGetBalanceChangeForPeriod which eventually calls xaccAccountGetXxxBalanceAsOfDateInCurrencyRecursive twice for each period. The latter function needs to scan account from beginning to period-date and find the exact balance.

And repeat for each account's children.



NOTE TO SELF: need to modify SCM to recurse into accounts' children.

Don't be silly. gnc:account-get-balances-at-dates can be implemented in C just as well as in Scheme using the same logic.

And for recursing the account's there's already gnc_account_foreach_descendant, which takes a C function, not a Scheme one.

gnc_account_foreach_descendant's user_data parameter gets a pointer to the data structure to be filled in. That could be 

typedef struct
{
    time_t date;
    gnc_numeric balance;
} BalanceOnDate;

typedef struct
{
   const gnc_currency* report_currency;
   unsigned int num_dates;
   BalanceOnDate* balances;
} BalanceList;

BalanceList.balances is really a dynamically allocated array of balances, so before the call to gnc_account_foreach_descendant you'll figure out how many periods you need to check and then

BalanceOnDate* balances = (BalanceOnDate*)malloc(sizeof(BalanceOnDate) * num_dates);
BalanceList balance_list = {report_currency, num_dates, balances};
assert(balances); // If balances == NULL we're out of memory and need to bail out.

The function will then scan balance_list.balances and the current account's split list in tandem and update each balance in balance_list. Having balance_list as a single memory block should keep it in the cache and make its accesses pretty fast; the split lists will be splattered about and likely generate a lot of cache misses so it's good to scan them only once.

You could also do the array as a GArray or std::vector, or use g_new0 instead of malloc.

Ah I hadn't interpreted the question as why scheme is better than C... of course they're equally good, but as the method to avoid modifying an existing exported function gnc_budget_get_account_period_actual_value which behaved in a particular way and would need to be changed to avoid closing entries.

The speed-up in scheme (or C) is another issue altogether, and I'm still working through it.

Realistically I do not think I have enough C skill to perform this so, may I suggest doing the one-line Recurrence.c fix?

I realize now that I misread your gnc:gnc-budget-get-account-period-actual-value function: You're not creating a list of all of the dates you want, you're just getting the begin and end dates for the period and traversing the split list once to get each of those. It would be vastly better to fix xaccAccountGetFooBalanceForPeriod to do that instead of traversing twice. It would actually be vastly better to just call xaccAccountGetNoclosingBalanceForPeriod! Here's why: gnc:account-get-balances-at-dates calls xaccAccountGetSplits, but because it's scheme it actually is calling _wrap_xaccAccountGetSplits in the swig-generated swig-engine.c (it's in the build directory after building). That traverses the split list and copies it to a Scheme list, *mallocing each node as it goes*. So you really have 3 traversals: One to copy the list to a Scheme list, one to get the two balances, and one to free the Scheme list. The first and third are 10-100x slower because of all of the calls to malloc and free.

As for the Recurrence.c change, sure. If you want to improve the efficiency, fix the xaccAccountFooBalanceChangeForPeriod functions to traverse the split list only once. gnc:gnc-budget-get-account-period-actual-value is a lose, stick with gnc-buget-get-account-period-actual-value.


The recursion into child accounts is sort of a separate problem because the current code doesn't do that either. It will be something like what I outlined in comment 10 either in C/C++ or Scheme and doing it in Scheme would require that you also replicate gnc_account_foreach_descendant in Scheme so C would be less work. I think you do have the C skill, you're just a little afraid of the pointers and memory allocation.

(In reply to John Ralls from comment #13)
> gnc:account-get-balances-at-dates calls xaccAccountGetSplits, but because
> it's scheme it actually is calling _wrap_xaccAccountGetSplits in the
> swig-generated swig-engine.c (it's in the build directory after building).
> That traverses the split list and copies it to a Scheme list, *mallocing
> each node as it goes*. So you really have 3 traversals: One to copy the list
> to a Scheme list, one to get the two balances, and one to free the Scheme
> list. The first and third are 10-100x slower because of all of the calls to
> malloc and free.

Thanks for explanation. Always had a suspicion xaccAccountGetSplits could be inefficient. Perhaps it would be useful to define and cache split->next and split->prev in Split structure (and gnc:account-get-balances-at-dates would use them) but it'd be a lot of work to ensure data consistency.

> As for the Recurrence.c change, sure. If you want to improve the efficiency,
> fix the xaccAccountFooBalanceChangeForPeriod functions to traverse the split
> list only once. gnc:gnc-budget-get-account-period-actual-value is a lose,
> stick with gnc-buget-get-account-period-actual-value.

Ok Recurrence.c it is then. Nobody's really complained about budgets being slow with large accounts. Only Windows was slow, and this is now resolved in 3.x onwards.