[GNC-dev] Convert Imap to Flat

[Geert Janssens]

Op maandag 5 november 2018 23:27:31 CET schreef John Ralls:
> > On Nov 6, 2018, at 2:50 AM, Geert Janssens <geert.gnucash at kobaltwit.be>
> > wrote:> 
> > Op maandag 5 november 2018 17:07:25 CET schreef Robert Fewell:
> >> Hi,
> >> I was poking around with the CSV importer and I noticed the following,
> >> this
> >> may also be an issue with other importers on first use after creating a
> >> new
> >> file...
> >> With a new empty xml file, I used a one line transaction csv file with
> >> appropriate settings and the 'Account' set to 'Assets:Current
> >> Assets:Checking Account' and observed the following when I got to the
> >> match
> >> page...
> >> 
> >> With the 'Checking Account' register open it would partly show the
> >> imported
> >> transactions, (this can be fixed by suspending GUI changes which I was
> >> going to propose in a PR) and the register would reload seven times.
> >> This reloading is caused by the triggering of the
> >> 'imap_convert_bayes_to_flat' function and as it is a new file you would
> >> not
> >> expect it to do any thing but it does. Adding a few print statements I
> >> get
> >> the following...
> >> 
> >> matchmap_find_destination
> >> imap_convert_bayes_to_flat
> >> convert_imap_account_bayes_to_flat 'Assets'
> >> 
> >>  gnc_split_register_load called for account 'Assets:Current
> >> 
> >> Assets:Checking Account' with list of 1
> >> convert_imap_account_bayes_to_flat 'Current Assets'
> >> 
> >>  gnc_split_register_load called for account 'Assets:Current
> >> 
> >> Assets:Checking Account' with list of 1
> >> convert_imap_account_bayes_to_flat 'Checking Account'
> >> 
> >>  gnc_split_register_load called for account 'Assets:Current
> >> 
> >> Assets:Checking Account' with list of 1
> >> convert_imap_account_bayes_to_flat 'Liabilities'
> >> 
> >>  gnc_split_register_load called for account 'Assets:Current
> >> 
> >> Assets:Checking Account' with list of 1
> >> convert_imap_account_bayes_to_flat 'Income'
> >> 
> >>  gnc_split_register_load called for account 'Assets:Current
> >> 
> >> Assets:Checking Account' with list of 1
> >> convert_imap_account_bayes_to_flat 'Expenses'
> >> 
> >>  gnc_split_register_load called for account 'Assets:Current
> >> 
> >> Assets:Checking Account' with list of 1
> >> convert_imap_account_bayes_to_flat 'Equity'
> >> 
> >>  gnc_split_register_load called for account 'Assets:Current
> >> 
> >> Assets:Checking Account' with list of 1
> >> 
> >> As you can see, seven accounts get updated forcing the register reload
> >> seven times, (not sure why those other accounts force a reload either),
> >> and
> >> this gets even worse if this first import is 100 transactions which would
> >> equate to 700 reloads. I have not worked out why all these accounts are
> >> updated or why after the first pass the converted flag is not set/noticed
> >> there by eliminating the convert for the rest of the transactions, it
> >> only
> >> seems to be noticed on subsequent imports.
> >> 
> >> You also get this behaviour if you start the 'Import Map Dialogue' which
> >> may be the source of a report about that dialogue freezing but that needs
> >> more investigating.
> > 
> > This calls gnc_account_imap_get_info_bayes, which also calls
> > imap_convert_bayes_to_flat so the it will trigger the same account
> > refreshes.> 
> >> Any idea why these accounts are updated and why it runs on every import
> >> transaction row ?
> > 
> > Why the accounts are updated: while only a run in the debugger will verify
> > it, this is what I have gathered from reading the code:
> > 
> > imap_convert_bayes_to_flat's sub functions will call xaccAccountBeginEdit
> > and xaccAccountCommitEdit at some point. This happens because it changes
> > the account's kvp frames that store the import maps.
> > 
> > On the other side, the register code has set a watch on the register's
> > account(s) via the component manager. So each time the account signals a
> > change (or more precisely a successful run of xaccAccountCommitEdit) the
> > component manager will tell the register to refresh itself.
> > 
> > As you suggest you can probably disable this by a call to
> > gnc_suspend_gui_refresh.
> > 
> > Why it runs on every import transaction row ? I suspect this is because
> > there are no imap records stored yet and hence the feature flag that
> > blocks the conversion is not set yet. So for each transaction it will try
> > to do the conversion, find there's no converted imap record to store and
> > skip setting the feature flag. This will probably continue forever if the
> > user doesn't use bayesian matching at all.
> > This is a difficult issue to solve. We don't want to set the flat_bayes
> > conversion flag if there are no bayes maps because that would needlessly
> > break backwards compatibility. We could make the conversion code more
> > careful and have it only commit to accounts if there really are changes
> > to commit. And add a run time flag that signals the conversion has run
> > already once. With that conversion should only start if this flag is
> > false AND the feature flag is false.
> > However it may all be unnecessary and perhaps just blocking register
> > updates while the transaction matching (or the whole import code) is
> > running may eliminate the performance bottleneck already.
> > So I would start with that: block gui updates.
> > Then secondly, make the imap code more careful not to do unnecessary
> > account updates and lastly consider a run time flag.
> > 
> > Like you though I suspect this may be at least one cause of the import
> > issues we see. Thanks for poking at it!
> 
> I think that we do want to set the feature flag in this case: The import map
> uses the flat layout and a pre-flat-bayes GnuCash won’t be able to read it,
> causing user frustration.

The timing matters. We want to set the flag from the first flat_bayes_imap 
entry that's actually written to file, not at the start of a conversion run. 
That's how the code is constructed now.

I don't want the flag to be set if at the end of a conversion attempt there 
still aren't any flat_bayes_imap entries. That would needlessly break 
backwards compatibility.
> 
> That’s not to say that we shouldn’t also do everything we can to speed up
> the code--it’s really slow--and certainly suspending UI events while
> computing the import matches is a good idea.
> 
> There’s something else going wrong, though: convert_imap_bayes_to_flat calls
> xaccAccountBeginEdit at the start and xaccAccountCommitEdit at the end.
> That should raise the edit level and prevent any interior calls to
> xaccAccountCommitEdit from doing anything.

This doesn't look wrong per se to me.
convert_imap_account_bayes_to_flat is run once for each account. So there will 
still be the same number of gui refreshes as there are accounts. I don't think 
we can reduce this further on the account begin/commit level. A complete gui 
refresh suspend may do better.

Regards,

Geert