[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [lmi] Micro-optimization in ledger_format

From: Vadim Zeitlin
Subject: Re: [lmi] Micro-optimization in ledger_format
Date: Wed, 16 Jan 2019 15:08:03 +0100

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

GC> Are you saying that code like this...
GC>   class X {...};
GC>   X foo()
GC>     {
GC>     X x;
GC>     return std::move(x);
GC>     }
GC> ...is wrong--not just non-optimal, but actually wrong? Here:
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>   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

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]