[GNC-dev] Slow user interface in 3.x gnucash (for large files), and potential optimization

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[GNC-dev] Slow user interface in 3.x gnucash (for large files), and potential optimization

Christian Stimming-4
Dear all,

as discussed before, I am only recently starting to use the 3.x series of
gnucash in daily production work. However, there are still some issues that
keep bugging me, compared to the 2.6.x version of gnucash.

One of the issues is that the user interface in the register is surprisingly
slower compared to 2.6.x. I have a somewhat large data file [1], so I saw some
behaviour that is probably not the usual case. The slowness is that when
activating the auto-complete by typing the first letter in the txn
description, then pressing <tab>, there is a delay of approx. 1.5 seconds with
100% CPU until the cursor appears in the next field and the data is filled in.
This delay is quite recognizable and it is annoying. In previous gnucash
versions, it was not there.

Hence I tried to narrow down the cause of this slowness using
valgrind/callgrind with switching on and off the instrumentation just around
this very keypress using callgrind_control. In the result, I recognized a
surprising high number of std::string constructor calls and similarly
destructor calls - which, for this single lookup operation, looked suspicious
to me.

Turns out that the std::string constructor and destructor is used a lot when
calling qof_instance_get_path_kvp() with a std::vector<std::string>, but the
std::vector is created on-the-fly using the { } syntax, AND (this is the
expensive part) all the std::string's inside that vector are also created on-
the-fly from string literals that are expanded from #defines. It occurred to
me that this last construction/destruction can be avoided rather easily: Why
don't we declare std::string constants and use those, instead of the const
char* literals expanded from the #defines? I implemented a very simple test of
this idea, see
https://github.com/cstim/gnucash/commit/99c97736dda4afb615aad4b70bd5b7bd761af459

Testing this change with callgrind again brought down the instruction count of
the described operation in the user interface from 3.2e9 to 2.6e9, which in my
opinion is a significant optimization. What do you think - would it be a good
idea to use kind of construct (no pun intended) in this and other places? Or
did I miss something here?

Regards,

Christian


[1] Some numbers about my data file: 4MB compressed XML, 30,000 transactions,
200 accounts, 30,000 kvp slots.
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] Slow user interface in 3.x gnucash (for large files), and potential optimization

John Ralls-2


> On Jun 12, 2018, at 12:57 PM, Christian Stimming <[hidden email]> wrote:
>
> Dear all,
>
> as discussed before, I am only recently starting to use the 3.x series of
> gnucash in daily production work. However, there are still some issues that
> keep bugging me, compared to the 2.6.x version of gnucash.
>
> One of the issues is that the user interface in the register is surprisingly
> slower compared to 2.6.x. I have a somewhat large data file [1], so I saw some
> behaviour that is probably not the usual case. The slowness is that when
> activating the auto-complete by typing the first letter in the txn
> description, then pressing <tab>, there is a delay of approx. 1.5 seconds with
> 100% CPU until the cursor appears in the next field and the data is filled in.
> This delay is quite recognizable and it is annoying. In previous gnucash
> versions, it was not there.
>
> Hence I tried to narrow down the cause of this slowness using
> valgrind/callgrind with switching on and off the instrumentation just around
> this very keypress using callgrind_control. In the result, I recognized a
> surprising high number of std::string constructor calls and similarly
> destructor calls - which, for this single lookup operation, looked suspicious
> to me.
>
> Turns out that the std::string constructor and destructor is used a lot when
> calling qof_instance_get_path_kvp() with a std::vector<std::string>, but the
> std::vector is created on-the-fly using the { } syntax, AND (this is the
> expensive part) all the std::string's inside that vector are also created on-
> the-fly from string literals that are expanded from #defines. It occurred to
> me that this last construction/destruction can be avoided rather easily: Why
> don't we declare std::string constants and use those, instead of the const
> char* literals expanded from the #defines? I implemented a very simple test of
> this idea, see
> https://github.com/cstim/gnucash/commit/99c97736dda4afb615aad4b70bd5b7bd761af459
>
> Testing this change with callgrind again brought down the instruction count of
> the described operation in the user interface from 3.2e9 to 2.6e9, which in my
> opinion is a significant optimization. What do you think - would it be a good
> idea to use kind of construct (no pun intended) in this and other places? Or
> did I miss something here?

Christian,

A very good catch indeed. But pre-constructing the string in qofbook.cpp only saves two string constructions per invocation as the vector still has to make its own copies. I guess that its much worse for you because the ancient gcc on Ubuntu 14.04 (gcc4.8) doesn't do small-string optimization.

KVP paths aren't really a good use for std::string anyway: It's a lot of weight for something that's used as a human-readable index. Even static char[] is heavy for this purpose. We could get a huge boost if we use something like Glib's quarks [1]. They can be expanded to their string value for storage so that we don't break file/db compatibility.

Want to have a go at that?


[1] https://developer.gnome.org/glib/stable/glib-Quarks.html
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] Slow user interface in 3.x gnucash (for large files), and potential optimization

Derek Atkins
John Ralls <[hidden email]> writes:

> A very good catch indeed. But pre-constructing the string in
> qofbook.cpp only saves two string constructions per invocation as the
> vector still has to make its own copies. I guess that its much worse
> for you because the ancient gcc on Ubuntu 14.04 (gcc4.8) doesn't do
> small-string optimization.

Is there any reason we cant use std::string& in the vector?  Or do we
think that we might lose references there?

> KVP paths aren't really a good use for std::string anyway: It's a lot
> of weight for something that's used as a human-readable index. Even
> static char[] is heavy for this purpose. We could get a huge boost if
> we use something like Glib's quarks [1]. They can be expanded to their
> string value for storage so that we don't break file/db compatibility.
>
> Want to have a go at that?

-derek
--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] Slow user interface in 3.x gnucash (for large files), and potential optimization

John Ralls-2


> On Jun 15, 2018, at 9:16 AM, Derek Atkins <[hidden email]> wrote:
>
> John Ralls <[hidden email]> writes:
>
>> A very good catch indeed. But pre-constructing the string in
>> qofbook.cpp only saves two string constructions per invocation as the
>> vector still has to make its own copies. I guess that its much worse
>> for you because the ancient gcc on Ubuntu 14.04 (gcc4.8) doesn't do
>> small-string optimization.
>
> Is there any reason we cant use std::string& in the vector?  Or do we
> think that we might lose references there?
>

In this instance the string is static so it would be safe to use const std::string&, but while there's an is_volatile type trait there's no is_static so we can't insert a static assert to catch cases where the actual std::string is on the stack. That could lead to some ugly bugs.

Besides, using strings is still leaving performance on the table: A string comparison is n times longer than an int comparison where n is the number of consecutive leading characters that are the same in the two strings. I got a huge speedup on loading a few months ago because GncGUID's operator==() was doing a character-wise comparison of the ascii representation instead of comparing the two int64_ts of the actual GUID.

Regards,
John Ralls

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] Slow user interface in 3.x gnucash (for large files), and potential optimization

Christian Stimming-4
Am Freitag, 15. Juni 2018, 10:05:09 schrieb John Ralls:

> >> A very good catch indeed. But pre-constructing the string in
> >> qofbook.cpp only saves two string constructions per invocation as the
> >> vector still has to make its own copies. I guess that its much worse
> >> for you because the ancient gcc on Ubuntu 14.04 (gcc4.8) doesn't do
> >> small-string optimization.
> >
> > Is there any reason we cant use std::string& in the vector?  Or do we
> > think that we might lose references there?
>
> In this instance the string is static so it would be safe to use const
> std::string&, but while there's an is_volatile type trait there's no
> is_static so we can't insert a static assert to catch cases where the
> actual std::string is on the stack. That could lead to some ugly bugs.

The std containers are specified in a way so that they make only sense to
contain the values by-value (ok, this might be formulated a bit too short, but
not too long ago this was a good rule of thumb). It is up to the
implementation to make optimizations (copy-on-write and such), but the
interface requires the programmer to think of the stored type as "by-value".

Nevertheless I think my change made good use of the std::string's already
existing optimizations. In particular, the saving was so surprisingly large
here that I think gcc's std::string itself implements some sort of copy-on-
write optimization, and this means its content isn't deep-copied in these
calls.

Some numbers from the valgrind/callgrind evaluation. I got the following
number of calls, starting from qof_query_run and skipping some uninterestings
calls in between:

- 6 qof_query_run
- 6 g_list_sort_with_data
- 360,000 (approx.) sort_func - due to the number of splits and txn in my file
- 360,000 xaccSplitOrder
    The most expensive part in this function is this next callee:
- 360,000 qof_book_use_split_action_for_num_field
- 360,000 (approx.) qof_book_get_property

This function in turn has approx. 1,100,000 calls to std::basic_string
constructor and destructor. The change in my commit is this:
Before my commit, these calls also caused 1,100,000 calls to
std::string::_S_create and std::string::_M_destroy i.e. the by-value
constructor and destructor. After my commit, I see a mere 22,000 calls to
std::string::_S_create and std::string::_M_destroy.

> Besides, using strings is still leaving performance on the table: A string
> comparison is n times longer than an int comparison where n is the number
> of consecutive leading characters that are the same in the two strings. I
> got a huge speedup on loading a few months ago because GncGUID's
> operator==() was doing a character-wise comparison of the ascii
> representation instead of comparing the two int64_ts of the actual GUID.

Sounds good. The above numbers seem to indicate that std::string already
optimizes away a long comparison as long as the string memory itself is
identical. In my opinion this is good enough of an optimization.

> KVP paths aren't really a good use for std::string anyway: It's a lot of
> weight for something that's used as a human-readable index. Even static
> char[] is heavy for this purpose. We could get a huge boost if we use
> something like Glib's quarks [1].
> Want to have a go at that?

Err... this would mean a major refactoring of any access to the KVP system.
No, I don't feel motivated to work into that direction. Also, if I understnad
the above valgrind evaluation correctly, we can already achieve almost optimum
(i.e. pointer comparison) by ensuring a set of std::string constants to be
used. I think this is the most efficient way to proceed here, and I would just
push this commit after some more cleanup.

Regards,
Christian


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] Slow user interface in 3.x gnucash (for large files), and potential optimization

John Ralls-2


> On Jun 15, 2018, at 12:55 PM, Christian Stimming <[hidden email]> wrote:
>
> Am Freitag, 15. Juni 2018, 10:05:09 schrieb John Ralls:
>>>> A very good catch indeed. But pre-constructing the string in
>>>> qofbook.cpp only saves two string constructions per invocation as the
>>>> vector still has to make its own copies. I guess that its much worse
>>>> for you because the ancient gcc on Ubuntu 14.04 (gcc4.8) doesn't do
>>>> small-string optimization.
>>>
>>> Is there any reason we cant use std::string& in the vector?  Or do we
>>> think that we might lose references there?
>>
>> In this instance the string is static so it would be safe to use const
>> std::string&, but while there's an is_volatile type trait there's no
>> is_static so we can't insert a static assert to catch cases where the
>> actual std::string is on the stack. That could lead to some ugly bugs.
>
> The std containers are specified in a way so that they make only sense to
> contain the values by-value (ok, this might be formulated a bit too short, but
> not too long ago this was a good rule of thumb). It is up to the
> implementation to make optimizations (copy-on-write and such), but the
> interface requires the programmer to think of the stored type as "by-value".
>
> Nevertheless I think my change made good use of the std::string's already
> existing optimizations. In particular, the saving was so surprisingly large
> here that I think gcc's std::string itself implements some sort of copy-on-
> write optimization, and this means its content isn't deep-copied in these
> calls.
>
> Some numbers from the valgrind/callgrind evaluation. I got the following
> number of calls, starting from qof_query_run and skipping some uninterestings
> calls in between:
>
> - 6 qof_query_run
> - 6 g_list_sort_with_data
> - 360,000 (approx.) sort_func - due to the number of splits and txn in my file
> - 360,000 xaccSplitOrder
>    The most expensive part in this function is this next callee:
> - 360,000 qof_book_use_split_action_for_num_field
> - 360,000 (approx.) qof_book_get_property
>
> This function in turn has approx. 1,100,000 calls to std::basic_string
> constructor and destructor. The change in my commit is this:
> Before my commit, these calls also caused 1,100,000 calls to
> std::string::_S_create and std::string::_M_destroy i.e. the by-value
> constructor and destructor. After my commit, I see a mere 22,000 calls to
> std::string::_S_create and std::string::_M_destroy.
>
>> Besides, using strings is still leaving performance on the table: A string
>> comparison is n times longer than an int comparison where n is the number
>> of consecutive leading characters that are the same in the two strings. I
>> got a huge speedup on loading a few months ago because GncGUID's
>> operator==() was doing a character-wise comparison of the ascii
>> representation instead of comparing the two int64_ts of the actual GUID.
>
> Sounds good. The above numbers seem to indicate that std::string already
> optimizes away a long comparison as long as the string memory itself is
> identical. In my opinion this is good enough of an optimization.
>
>> KVP paths aren't really a good use for std::string anyway: It's a lot of
>> weight for something that's used as a human-readable index. Even static
>> char[] is heavy for this purpose. We could get a huge boost if we use
>> something like Glib's quarks [1].
>> Want to have a go at that?
>
> Err... this would mean a major refactoring of any access to the KVP system.
> No, I don't feel motivated to work into that direction. Also, if I understnad
> the above valgrind evaluation correctly, we can already achieve almost optimum
> (i.e. pointer comparison) by ensuring a set of std::string constants to be
> used. I think this is the most efficient way to proceed here, and I would just
> push this commit after some more cleanup.

Christian,

Hardly major, particularly considering that Aaron has already completely rewritten KVP into C++ and separately changed the access from '/'-delimited string paths to std::vector<std::string> of the tokens.
All that's required is lifting Glib's GQuark and converting it to C++, changing KVP frame's std::vector<std::string> to std::vector<GncQuark>, and using g_quark_intern_from_static_string instead of static std::string.

The situation you've tested is a bit of a special case because all of the uses of the KVP_OPTIONS keys are in one file. Any keys used in more than one file will have different static std::strings and pointer comparison won't work. Examples include anything accessed with qof_instance_set/get. That said, the mallocs/deallocs are 99% or more of the performance hit. If you're willing and have time to do it your way and have time to get it done quickly then by all means go ahead.

Regards,
John Ralls
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] Slow user interface in 3.x gnucash (for large files), and potential optimization

David Carlson-4
Christian Stimming and all developers,

First, I want to thank all of you for your great effort to roll out the 3.0
release and the quick followups to fix bugs and get releases 3.1, and 3.2
out in quick succession.

Christian has made substantial progress at isolating and solving several
major sources of lagging response with Gnucash.  I would like to ask you to
see if there is room for improvement or at least some low hanging fruit in
a couple of areas that bother me.  Actually, I would like all developers to
take an interest in this issue and to at least keep in mind that whatever
code they implement should be designed to run as efficiently as possible.

I am still using releases 2.6.15 through 2.6.17 on various computers and I
have recently observed a couple of things that are definitely not new but
that may still impact releases 3.2 and future 3.3.  These are more
pronounced when using SSH to run GnuCash on a faster computer from a slower
(Windows) computer.  Because SSH seems to be vulnerable, I suspect that
there may be repetitive screen refreshes happening as the code is
executed.

For me, the most noticeable lag happens every time an automatic file save
happens.  GnuCash does not accept user input during the save.  This is
stretched dramatically if the file resides on a network resource instead of
the local hard drive or ssd but it is also substantially slower when using
SSH to run GnuCash remotely.  This lag is 1 minute 10 seconds for me.  This
is with a local network NAS serving the foo.gnucash standard format
compressed file via Ethernet to a virtual Debian Stretch Linux machine.

When opening Edit > Preferences there is a long lag.  For my situation, the
lag is about 24 seconds before the Preferences window appears.  This is not
something users do frequently, but it is quite noticeable.

I have also noticed that after a file is opened that was previously closed
with open reports, the first time that the user touches the report tab
there is an obvious flicker as the report in the window is reconstructed
before the the users very own eyes.  This is most apparent for linechart
reports and probably barchart reports.

Thank You All,

David C


On Fri, Jun 15, 2018 at 3:47 PM, John Ralls <[hidden email]> wrote:

>
>
> > On Jun 15, 2018, at 12:55 PM, Christian Stimming <[hidden email]>
> wrote:
> >
> > Am Freitag, 15. Juni 2018, 10:05:09 schrieb John Ralls:
> >>>> A very good catch indeed. But pre-constructing the string in
> >>>> qofbook.cpp only saves two string constructions per invocation as the
> >>>> vector still has to make its own copies. I guess that its much worse
> >>>> for you because the ancient gcc on Ubuntu 14.04 (gcc4.8) doesn't do
> >>>> small-string optimization.
> >>>
> >>> Is there any reason we cant use std::string& in the vector?  Or do we
> >>> think that we might lose references there?
> >>
> >> In this instance the string is static so it would be safe to use const
> >> std::string&, but while there's an is_volatile type trait there's no
> >> is_static so we can't insert a static assert to catch cases where the
> >> actual std::string is on the stack. That could lead to some ugly bugs.
> >
> > The std containers are specified in a way so that they make only sense
> to
> > contain the values by-value (ok, this might be formulated a bit too
> short, but
> > not too long ago this was a good rule of thumb). It is up to the
> > implementation to make optimizations (copy-on-write and such), but the
> > interface requires the programmer to think of the stored type as
> "by-value".
> >
> > Nevertheless I think my change made good use of the std::string's
> already
> > existing optimizations. In particular, the saving was so surprisingly
> large
> > here that I think gcc's std::string itself implements some sort of
> copy-on-
> > write optimization, and this means its content isn't deep-copied in
> these
> > calls.
> >
> > Some numbers from the valgrind/callgrind evaluation. I got the following
> > number of calls, starting from qof_query_run and skipping some
> uninterestings
> > calls in between:
> >
> > - 6 qof_query_run
> > - 6 g_list_sort_with_data
> > - 360,000 (approx.) sort_func - due to the number of splits and txn in
> my file
> > - 360,000 xaccSplitOrder
> >    The most expensive part in this function is this next callee:
> > - 360,000 qof_book_use_split_action_for_num_field
> > - 360,000 (approx.) qof_book_get_property
> >
> > This function in turn has approx. 1,100,000 calls to std::basic_string
> > constructor and destructor. The change in my commit is this:
> > Before my commit, these calls also caused 1,100,000 calls to
> > std::string::_S_create and std::string::_M_destroy i.e. the by-value
> > constructor and destructor. After my commit, I see a mere 22,000 calls
> to
> > std::string::_S_create and std::string::_M_destroy.
> >
> >> Besides, using strings is still leaving performance on the table: A
> string
> >> comparison is n times longer than an int comparison where n is the
> number
> >> of consecutive leading characters that are the same in the two strings.
> I
> >> got a huge speedup on loading a few months ago because GncGUID's
> >> operator==() was doing a character-wise comparison of the ascii
> >> representation instead of comparing the two int64_ts of the actual GUID.
> >
> > Sounds good. The above numbers seem to indicate that std::string already
> > optimizes away a long comparison as long as the string memory itself is
> > identical. In my opinion this is good enough of an optimization.
> >
> >> KVP paths aren't really a good use for std::string anyway: It's a lot of
> >> weight for something that's used as a human-readable index. Even static
> >> char[] is heavy for this purpose. We could get a huge boost if we use
> >> something like Glib's quarks [1].
> >> Want to have a go at that?
> >
> > Err... this would mean a major refactoring of any access to the KVP
> system.
> > No, I don't feel motivated to work into that direction. Also, if I
> understnad
> > the above valgrind evaluation correctly, we can already achieve almost
> optimum
> > (i.e. pointer comparison) by ensuring a set of std::string constants to
> be
> > used. I think this is the most efficient way to proceed here, and I
> would just
> > push this commit after some more cleanup.
>
> Christian,
>
> Hardly major, particularly considering that Aaron has already completely
> rewritten KVP into C++ and separately changed the access from '/'-delimited
> string paths to std::vector<std::string> of the tokens.
> All that's required is lifting Glib's GQuark and converting it to C++,
> changing KVP frame's std::vector<std::string> to std::vector<GncQuark>, and
> using g_quark_intern_from_static_string instead of static std::string.
>
> The situation you've tested is a bit of a special case because all of the
> uses of the KVP_OPTIONS keys are in one file. Any keys used in more than
> one file will have different static std::strings and pointer comparison
> won't work. Examples include anything accessed with qof_instance_set/get.
> That said, the mallocs/deallocs are 99% or more of the performance hit. If
> you're willing and have time to do it your way and have time to get it done
> quickly then by all means go ahead.
>
> Regards,
> John Ralls
> _______________________________________________
> gnucash-devel mailing list
> [hidden email]
> https://lists.gnucash.org/mailman/listinfo/gnucash-devel
>
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] Slow user interface in 3.x gnucash (for large files), and potential optimization

Christopher Lam
Hi David,

Just as a matter of experimentation, try the new charting tree available
at https://github.com/christopherlam/gnucash/tree/master-chartjs - it
replaces the antiquated jqplot with shiny ChartJS - it is awaiting beta
testers, and it seems you can build from source :-)

Features:

- URL jumps from barcharts and piecharts have been resurrected
- much more responsive
- much more code-efficient... html-line/pie/scatter/barcharts.scm have
been merged into one html-charts.scm

Otherwise it's much of a likeness with old jqplot.

C


On 05/07/18 08:33, David Carlson wrote:

> Christian Stimming and all developers,
>
> First, I want to thank all of you for your great effort to roll out the 3.0
> release and the quick followups to fix bugs and get releases 3.1, and 3.2
> out in quick succession.
>
> Christian has made substantial progress at isolating and solving several
> major sources of lagging response with Gnucash.  I would like to ask you to
> see if there is room for improvement or at least some low hanging fruit in
> a couple of areas that bother me.  Actually, I would like all developers to
> take an interest in this issue and to at least keep in mind that whatever
> code they implement should be designed to run as efficiently as possible.
>
> I am still using releases 2.6.15 through 2.6.17 on various computers and I
> have recently observed a couple of things that are definitely not new but
> that may still impact releases 3.2 and future 3.3.  These are more
> pronounced when using SSH to run GnuCash on a faster computer from a slower
> (Windows) computer.  Because SSH seems to be vulnerable, I suspect that
> there may be repetitive screen refreshes happening as the code is
> executed.
>
> For me, the most noticeable lag happens every time an automatic file save
> happens.  GnuCash does not accept user input during the save.  This is
> stretched dramatically if the file resides on a network resource instead of
> the local hard drive or ssd but it is also substantially slower when using
> SSH to run GnuCash remotely.  This lag is 1 minute 10 seconds for me.  This
> is with a local network NAS serving the foo.gnucash standard format
> compressed file via Ethernet to a virtual Debian Stretch Linux machine.
>
> When opening Edit > Preferences there is a long lag.  For my situation, the
> lag is about 24 seconds before the Preferences window appears.  This is not
> something users do frequently, but it is quite noticeable.
>
> I have also noticed that after a file is opened that was previously closed
> with open reports, the first time that the user touches the report tab
> there is an obvious flicker as the report in the window is reconstructed
> before the the users very own eyes.  This is most apparent for linechart
> reports and probably barchart reports.
>
> Thank You All,
>
> David C
>
>
> On Fri, Jun 15, 2018 at 3:47 PM, John Ralls <[hidden email]> wrote:
>
>>
>>> On Jun 15, 2018, at 12:55 PM, Christian Stimming <[hidden email]>
>> wrote:
>>> Am Freitag, 15. Juni 2018, 10:05:09 schrieb John Ralls:
>>>>>> A very good catch indeed. But pre-constructing the string in
>>>>>> qofbook.cpp only saves two string constructions per invocation as the
>>>>>> vector still has to make its own copies. I guess that its much worse
>>>>>> for you because the ancient gcc on Ubuntu 14.04 (gcc4.8) doesn't do
>>>>>> small-string optimization.
>>>>> Is there any reason we cant use std::string& in the vector?  Or do we
>>>>> think that we might lose references there?
>>>> In this instance the string is static so it would be safe to use const
>>>> std::string&, but while there's an is_volatile type trait there's no
>>>> is_static so we can't insert a static assert to catch cases where the
>>>> actual std::string is on the stack. That could lead to some ugly bugs.
>>> The std containers are specified in a way so that they make only sense
>> to
>>> contain the values by-value (ok, this might be formulated a bit too
>> short, but
>>> not too long ago this was a good rule of thumb). It is up to the
>>> implementation to make optimizations (copy-on-write and such), but the
>>> interface requires the programmer to think of the stored type as
>> "by-value".
>>> Nevertheless I think my change made good use of the std::string's
>> already
>>> existing optimizations. In particular, the saving was so surprisingly
>> large
>>> here that I think gcc's std::string itself implements some sort of
>> copy-on-
>>> write optimization, and this means its content isn't deep-copied in
>> these
>>> calls.
>>>
>>> Some numbers from the valgrind/callgrind evaluation. I got the following
>>> number of calls, starting from qof_query_run and skipping some
>> uninterestings
>>> calls in between:
>>>
>>> - 6 qof_query_run
>>> - 6 g_list_sort_with_data
>>> - 360,000 (approx.) sort_func - due to the number of splits and txn in
>> my file
>>> - 360,000 xaccSplitOrder
>>>     The most expensive part in this function is this next callee:
>>> - 360,000 qof_book_use_split_action_for_num_field
>>> - 360,000 (approx.) qof_book_get_property
>>>
>>> This function in turn has approx. 1,100,000 calls to std::basic_string
>>> constructor and destructor. The change in my commit is this:
>>> Before my commit, these calls also caused 1,100,000 calls to
>>> std::string::_S_create and std::string::_M_destroy i.e. the by-value
>>> constructor and destructor. After my commit, I see a mere 22,000 calls
>> to
>>> std::string::_S_create and std::string::_M_destroy.
>>>
>>>> Besides, using strings is still leaving performance on the table: A
>> string
>>>> comparison is n times longer than an int comparison where n is the
>> number
>>>> of consecutive leading characters that are the same in the two strings.
>> I
>>>> got a huge speedup on loading a few months ago because GncGUID's
>>>> operator==() was doing a character-wise comparison of the ascii
>>>> representation instead of comparing the two int64_ts of the actual GUID.
>>> Sounds good. The above numbers seem to indicate that std::string already
>>> optimizes away a long comparison as long as the string memory itself is
>>> identical. In my opinion this is good enough of an optimization.
>>>
>>>> KVP paths aren't really a good use for std::string anyway: It's a lot of
>>>> weight for something that's used as a human-readable index. Even static
>>>> char[] is heavy for this purpose. We could get a huge boost if we use
>>>> something like Glib's quarks [1].
>>>> Want to have a go at that?
>>> Err... this would mean a major refactoring of any access to the KVP
>> system.
>>> No, I don't feel motivated to work into that direction. Also, if I
>> understnad
>>> the above valgrind evaluation correctly, we can already achieve almost
>> optimum
>>> (i.e. pointer comparison) by ensuring a set of std::string constants to
>> be
>>> used. I think this is the most efficient way to proceed here, and I
>> would just
>>> push this commit after some more cleanup.
>> Christian,
>>
>> Hardly major, particularly considering that Aaron has already completely
>> rewritten KVP into C++ and separately changed the access from '/'-delimited
>> string paths to std::vector<std::string> of the tokens.
>> All that's required is lifting Glib's GQuark and converting it to C++,
>> changing KVP frame's std::vector<std::string> to std::vector<GncQuark>, and
>> using g_quark_intern_from_static_string instead of static std::string.
>>
>> The situation you've tested is a bit of a special case because all of the
>> uses of the KVP_OPTIONS keys are in one file. Any keys used in more than
>> one file will have different static std::strings and pointer comparison
>> won't work. Examples include anything accessed with qof_instance_set/get.
>> That said, the mallocs/deallocs are 99% or more of the performance hit. If
>> you're willing and have time to do it your way and have time to get it done
>> quickly then by all means go ahead.
>>
>> Regards,
>> John Ralls
>> _______________________________________________
>> gnucash-devel mailing list
>> [hidden email]
>> https://lists.gnucash.org/mailman/listinfo/gnucash-devel
>>
> _______________________________________________
> gnucash-devel mailing list
> [hidden email]
> https://lists.gnucash.org/mailman/listinfo/gnucash-devel

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel