Re: [GNC-dev] Next shot at "Slow user interface in 3.x gnucash (for large files)"
Am Mittwoch, 20. Juni 2018, 17:45:42 schrieb John Ralls: > > On Jun 20, 2018, at 2:29 PM, Christian Stimming > 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 ___ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Re: [GNC-dev] Next shot at "Slow user interface in 3.x gnucash (for large files)"
> On Jun 20, 2018, at 2:29 PM, Christian Stimming > 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 "". >>> 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 "" >>> 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 ___ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Re: [GNC-dev] Next shot at "Slow user interface in 3.x gnucash (for large files)"
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 "". > > 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 "" > > 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. Regards, Christian [1] https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#GObject-notify ___ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Re: [GNC-dev] Next shot at "Slow user interface in 3.x gnucash (for large files)"
> On Jun 18, 2018, at 1:32 PM, Christian Stimming > wrote: > > Am Sonntag, 17. Juni 2018, 20:09:24 schrieb John Ralls: >>> I.e. the function qof_book_use_split_action_for_num_field is very very >>> expensive. Currently it does a KVP lookup on each call. What keeps us from >>> turning this KVP value into a normal gboolean value in the struct >>> _QofBook? >> >> Christian, >> >> 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. > >> One alternative would be to redo the backends so that member variables can >> be designated as stored in KVP, effectively moving KVP out of engine. That >> would be a bit of work but it’s an alternative to changing the schemas. >> It’s a route we’ve discussed before but nobody’s been inclined yet to take >> it on. >> >> 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: > https://github.com/cstim/gnucash/commit/376cc19b7143983ce297f2272abf1a44e72fd851 > This change gets the register UI back to the old speed again. No more 1 > second > delay after hitting enter. (I wonder why I seem to be the only one who got > bugged by this, but whatever.) > > However, I broke the original feature in that change. To reproduce: Open some > register where there is some text in the "Num" field, say "". 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 "" 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. Regards, John Ralls ___ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Re: [GNC-dev] Next shot at "Slow user interface in 3.x gnucash (for large files)"
Am Sonntag, 17. Juni 2018, 20:09:24 schrieb John Ralls: > > I.e. the function qof_book_use_split_action_for_num_field is very very > > expensive. Currently it does a KVP lookup on each call. What keeps us from > > turning this KVP value into a normal gboolean value in the struct > > _QofBook? > > Christian, > > 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. > One alternative would be to redo the backends so that member variables can > be designated as stored in KVP, effectively moving KVP out of engine. That > would be a bit of work but it’s an alternative to changing the schemas. > It’s a route we’ve discussed before but nobody’s been inclined yet to take > it on. > > 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: https://github.com/cstim/gnucash/commit/376cc19b7143983ce297f2272abf1a44e72fd851 This change gets the register UI back to the old speed again. No more 1 second delay after hitting enter. (I wonder why I seem to be the only one who got bugged by this, but whatever.) However, I broke the original feature in that change. To reproduce: Open some register where there is some text in the "Num" field, say "". 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 "" 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. Regards, Christian ___ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Re: [GNC-dev] Next shot at "Slow user interface in 3.x gnucash (for large files)"
> On Jun 17, 2018, at 1:38 PM, Christian Stimming > wrote: > > Dear all, > > after the initial success in resolving some of the malloc/free calls due to > vector allocation for speeding up the user interface for large files, > I looked into some more causes of slower reaction time in the user interface. > > The function xaccSplitOrder is one that is called quite often. This may or > not > may not be ok, but there's a huge performance hit of that function: > > 80% of its runtime instructions are needed for the following line > (Split.c:1479 in maint): > > action_for_num = qof_book_use_split_action_for_num_field >(xaccSplitGetBook (sa)); > > I.e. the function qof_book_use_split_action_for_num_field is very very > expensive. Currently it does a KVP lookup on each call. What keeps us from > turning this KVP value into a normal gboolean value in the struct _QofBook? > What steps are needed to turn a KVP value into a normal data member? (We > don't > happen to have a short todo list for that case, do we :) Does anyone happen > to > know the necessary steps from memory? This would help a lot here. Thank you > very much. Christian, 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. One alternative would be to redo the backends so that member variables can be designated as stored in KVP, effectively moving KVP out of engine. That would be a bit of work but it’s an alternative to changing the schemas. It’s a route we’ve discussed before but nobody’s been inclined yet to take it on. 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. Regards, John Ralls ___ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
