Re: [lmi] Micro-optimization in ledger_format

[Vadim Zeitlin]

On Wed, 16 Jan 2019 12:17:49 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-11-26 15:53, Vadim Zeitlin wrote:
GC> > 
GC> >  One of results of profiling the PDF generation is that applying the diff
GC> > from https://github.com/vadz/lmi/pull/104/files reduces the total time by
GC> > more than 10% (~14% in our tests), i.e. creating a new stream object and
GC> > imbuing it with a custom locale every time the function is called is very
GC> > expensive.
GC> 
GC> I was distracted by an "emergency" in early December, but would like to
GC> return to this now before it gets too stale.

 Thanks for getting back to it!

GC> First of all, is the patch
GC> available through the git remote I normally use, viz.,
GC>   [remote "xanadu"]
GC>     url = https://github.com/vadz/lmi.git
GC>     fetch = +refs/heads/*:refs/remotes/xanadu/*
GC> ? Perhaps not,

 No, not yet, because I was waiting for the answer concerning the use of
IIFE before merging it into vadz/lmi from thesiv/lmi, which is the only
place where this code lives currently.

GC> as I see this on the PR page:
GC>   thesiv wants to merge 1 commit into vadz:master from 
thesiv:speed_up_ledger_format
GC> so maybe I'd need to add another remote for Ilja?

 You could do this, but the idea was to use me as the first line of defence
against bugs/problems, i.e. Ilja would submit PRs to my repository, I would
review and tweak them if necessary and then create branches in vadz
repository itself with them. Again, the only reason this hasn't happened in
this case yet is because I wanted to check if the current patch was
acceptable to you or if it should be rewritten in another form.

GC> I'm not sure whether I like the IIFE technique or not. In this particular
GC> case, I'm much more concerned that I don't understand the review comment:
GC>   https://github.com/vadz/lmi/pull/104#discussion_r235774795

 Sorry, this is confusing, but this comment referred to an old version of
the code which is not visible any more as Ilja has changed it after my
review.

GC> Are you saying that code like this...
GC> 
GC>   class X {...};
GC>   X foo()
GC>     {
GC>     X x;
GC>     return std::move(x);
GC>     }
GC> 
GC> ...is wrong--not just non-optimal, but actually wrong? Here:
GC>   
https://isocpp.org/blog/2013/02/no-really-moving-a-return-value-is-easy-stackoverflow
GC> under the heading "Third example", Howard Hinnant seems to be saying
GC> that it's non-optimal but non-wrong

 Howard is correct, of course, and in this case, when the function returns
a value, it's non-wrong -- but also non-optimal and definitely non-needed.

GC> Or is it the case that the original returned a reference, e.g.:
GC> 
GC>   static std::stringstream&& interpreter{[]() {

 I believe that this was indeed the case. At the very least, I remember
thinking that it was the case. So either it was, or I was wrong, but I
can't say with certainty which exactly it was. In any case, Howard is
right.

GC> and I'm just not seeing that due to my unfamiliarity with the github
GC> web interface?

 No, you're not seeing this because the original version just doesn't exist
any more because it was overwritten by a later force-push, so I can't check
what was the original version neither any more.

GC> >  But the important thing is that we really should avoid recreating and
GC> > reinitializing this object all the time as it's really wasteful.
GC> 
GC> I completely agree. Using std::stringstream operators >> and << is
GC> probably not fast anyway:
GC>   https://lists.nongnu.org/archive/html/lmi/2018-02/msg00036.html
GC> but here Ilja has found a simple way to make it less costly, so
GC> it's well worth doing.

 OK, but I'm still not sure whether I should change this or if I can leave
it as is and create a branch in vadz/lmi repository with these changes?
Please let me know what would you prefer me to do.

 Thanks in advance,
VZ