Op maandag 5 november 2018 23:27:31 CET schreef John Ralls: > > On Nov 6, 2018, at 2:50 AM, Geert Janssens <geert.gnuc...@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 _______________________________________________ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel