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

[Christian Stimming]

Am Mittwoch, 20. Juni 2018, 17:45:42 schrieb John Ralls:
> > On Jun 20, 2018, at 2:29 PM, Christian Stimming <christian at cstimming.de>
> >>>> 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.
> > 
> > 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.

Thanks, this was the missing hint: To cover the value setting of 
qof_book_set_option, I added the cache invalidation in that very function, 
regardless of the actual option that is being set. This seems to be the 
easiest way to ensure a valid cache value, or a suitable lookup on invalid 
cache at the next call of the respective getter.

> 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.

I thought redirecting qof_book_set_property into qof_book_set_option doesn't 
really help here, because the GObject property system could have been used 
somewhere else to access the GObject's property directly. (This is not the 
case, though [1].) The callback of the GObject property covers that case, and 
also the qof_instance_set calls, so I just left it there.

For qof_book_set_option the easiest solution is to make this setter invalidate 
the cached value on each call. Next time the getter is called, the potentially 
changed value is then looked up and cached again. This seems to have covered 
all cases, and now works fine.

Using qof_book_set_option in qof_book_set_property would require to convert 
the std::vector Path to GSList, which seems to be the type that comes from 
Scheme. To be more precise, qof_book_set_option would need to be refactored so 
that the interface from Scheme is kept available, but the C++ interface with 
the std::vector Path must be added in some efficient way. As my original 
problem was solved as outlined above, I though I'd rather stay away from this 
additional refactoring, sorry...

https://github.com/cstim/gnucash/commit/92ea3ba8a60bf4eb19d9b6932fb3ed8b582551a5
and I'll push to code.gnucash.org's maint once I can connect to it again.

Thanks for the reviews and ideas!

Regards,

Christian

[1] (Although this would require to use the property's GParam name, "split-
action-num-field", and we can globally search for this string inside gnucash's 
source code. It appears in qofbook.cpp and in the unittests, but nowhere else, 
so we can rule out this