[GNC-dev] Next shot at "Slow user interface in 3.x gnucash (for large files)"

[John Ralls]

> On Jun 20, 2018, at 2:29 PM, Christian Stimming <christian at cstimming.de> wrote:
> 
> Am Mittwoch, 20. Juni 2018, 09:59:52 schrieb John Ralls:
>>>> It’s all about file compatibility, remember? As it stands, if you make
>>>> something a regular member variable then you have to change the schema to
>>>> add the element/column, and write a scrub to update older data. We
>>>> strongly
>>>> prefer not to do that during a stable release cycle and generally require
>>>> that the last version of the previous stable series be able to read both
>>>> formats.
>>> 
>>> Yes. Although what I'm concerned with is only the lookup performance, not
>>> the serialization place of that option. The serialization may be kept
>>> unchanged completely.
>> 
>> Right. So thinking about that some more, persistence (I think that’s a
>> better description of this than serialization) is driven by the backends
>> rather than the objects, so you can create a member variable and as long as
>> you don’t tell the backends about it it won’t be a problem. That seems to
>> be what you’ve tried to do in your patch.
> 
> Yes, exactly. This can be applied in this case, and maybe others, if there are 
> more data fields that turn out to be a performance bottleneck.
> 
>>>> The less involved approaches would be to cache the value or to make KVP
>>>> retrieval more efficient. I suspect in this case that caching will be the
>>>> easiest.
>>> 
>>> Yes. I've introduced some caching of this value here, but I still need a
>>> little bit of help: 
>>> ...
>>> 
>>> However, I broke the original feature in that change. To reproduce: Open
>>> some register where there is some text in the "Num" field, say "1111".
>>> Switch on the "Double Line" view mode. Then open File -> Properties, and
>>> on the first tab, activate the option "Use Split Action Field for
>>> Number". Press Ok. Before my commit, in the opened register the "1111"
>>> now moved from the first line of the txn to the second line, and vice
>>> versa after changing that option again. Unfortunately my commit broke
>>> that feature. Maybe someone has a good idea why? Thanks for some
>>> pointers.
>> 
>> As a first guess I’d look at the hook list execution to make sure that your
>> adding qof_book_option_num_field_source_changed_cb isn’t somehow preventing
>> gnc_plugin_page_register_sort_book_option_changed (in
>> gnucash/gnome/gnc-plugin-page-register.c) from running.
> 
> I've gotten rid of that weird function from engine-helpers.[hc] and used the 
> plan GObject's "notify" signal [1]. Also, the first implementation broke the 
> respective unittests. I've got another try, this time with the unittests ok 
> again:
> https://github.com/cstim/gnucash/commit/c915ec0954c49ecdd65af84680354c38fe192614
> Unfortunately, the UI feature still seems to be broken as described above, 
> which is somewhat weird since this is what the unittests should have caught.
> 
> Changing the option in the UI is done inside the Scheme option system. This 
> particular option is defined in business-prefs.scm. Maybe the changed value of 
> the book's property in this case does not trigger the "notify" signal of the 
> GObject type system, but all the calls to qof_instance_set in the unittests do 
> trigger that signal?
> 
> In any case the pattern for the performance speed-up should be rather clear: 
> Define a cached value of the KVP, register a callback on each write access, 
> and have each write access invalidate the previously cached value so that it 
> is retrieved again and stored as valid cached value. The open question here 
> seems to be where to find the correct callbacks on write access.
> 

Christian,

Yeah, the Scheme option code calls  qof_book_set_option and that sets the slot directly with KvpFrame::set_path instead of calling qof_instance_set (which calls g_object_set) so it won’t emit the notify::spilt_action_num_field signal when you change the setting in File>Options. Are you sure that’s working? The unit test on the other hand uses qof_instance_set which wraps g_obect_set_valist and that does fire the notify signal.

qof_book_set_option, being defined in qofbook.cpp, offers a simpler approach: Instead of dealing with callbacks just set the cached value along with the slot. You can fix qof_book_set_property:PROP_OPT_NUM_FIELD_SOURCE to call qof_book_set_option instead of qof_instance_set_path_kvp. You’ll still need qof_book_use_split_action_for_num to read the slot on its first call because qof_book_init can’t set cached_num_field_source from KVP because at least in the SQL backend the book is constructed before the slots are loaded.

Regards,
John Ralls