[GNC-dev] More need for speed improvement around XML

[Christian Stimming]

Am Samstag, 29. Dezember 2018, 01:45:32 CET schrieb John Ralls:
> > When saving to XML file, for each transaction the call stack with the
> > expensive code walks down like this:
> > 
> > gnc_transaction_dom_tree_create
> > add_time64
> > time64_to_dom_tree
> > gnc_print_time64
> > GncDateTime::format
> > GncDateTimeImpl::format
> 
> Christian,
> 
> I don't have time to fully test it out right now, but give
> https://github.com/jralls/gnucash/tree/maint a go.
> 
> As noted in the HEAD commit it has a side effect of recording times in XML
> files in UTC, no timezones. Users who want to keep their files in version
> control will like that, but it needs to be checked for backward
> compatibility with 2.6 before it can be merged into maint.

Thanks for working at this. Indeed the main issue is that we don't need a 
user-visible format conversion (which must respect the locale) but instead a 
serialization to a fixed iso8601 format. Fixing this issue also means that the 
XML files will no longer depend on the local timezone, which is an issue that 
has been asked for a long time.

Thanks for your patch - this fixes performance issues with 
gnc_time64_to_iso8601_buff() and all of its callers. However, the Save-to-XML 
function calls gnc_print_time64() instead (from time64_to_dom_tree), which is 
a different code path. Consequently, your patch alone does not modify the 
saved XML (just verified). Your call to GncDateTime::format_iso8601 needs to 
be used in gnc_print_time64, too, like so:

 xmlNodePtr
 time64_to_dom_tree (const char* tag, const time64 time)
 {
     xmlNodePtr ret;
     g_return_val_if_fail (time != INT64_MAX, NULL);
-    auto date_str = gnc_print_time64 (time, "%Y-%m-%d %H:%M:%S %q");
-    if (!date_str)
-        return NULL;
+    GncDateTime gncdt(time);
+    auto sstr = gncdt.format_iso8601();
+    gchar* date_str = g_strdup(sstr.c_str());


However, with this patch, all time64 timestamps in the XML file will then 
change from time+timezone to UTC time only. To my surprise, although we knew 
about this issue for such a long time and changed most timestamps 
appropriately, there are still timestamps that will change date due to this 
change. With a quick glance, the "reconciled-date" timestamp contains 
beginning-of-day in a local timezone. Also maybe user-entered commodity 
prices. Others might still show up, oh well.

I haven't checked for backward compatibility with 2.6, but will do during the 
next days.

Regards,

Christian