Re: [lmi] Micro-optimization in ledger_format

[Greg Chicares]

On 2018-11-26 15:53, Vadim Zeitlin wrote:
> 
>  One of results of profiling the PDF generation is that applying the diff
> from https://github.com/vadz/lmi/pull/104/files reduces the total time by
> more than 10% (~14% in our tests), i.e. creating a new stream object and
> imbuing it with a custom locale every time the function is called is very
> expensive.

I was distracted by an "emergency" in early December, but would like to
return to this now before it gets too stale. First of all, is the patch
available through the git remote I normally use, viz.,
  [remote "xanadu"]
    url = https://github.com/vadz/lmi.git
    fetch = +refs/heads/*:refs/remotes/xanadu/*
? Perhaps not, as I see this on the PR page:
  thesiv wants to merge 1 commit into vadz:master from 
thesiv:speed_up_ledger_format
so maybe I'd need to add another remote for Ilja?

[The question is kind of academic in this case, as the patch is tiny
and I could easily copy it from the screen. But it would be useful to
know the answer for future work.]

>  I'm not sure if the patch is acceptable in its current form, which
> probably looks unusual because it uses foreign, but still useful in C++
> IMO, concept of IIFE[*], i.e. it defines a lambda only to execute it
> immediately. Personally, I like this syntax as, after having seen it once,
> I think it's very clear what it does and it has the advantage of, first,
> keeping all the related code in one place and, second, of not encumbering
> it with helper boolean variables used only in order to initialize the
> object once. But if you indeed don't like using this pattern, please let me
> know how would you prefer to change it. As per above, I know of 2 commonly
> used options: either add a separate function (which suffers from not having
> all the code in the same place any more) or add a boolean flag (which is
> more verbose and a bit clumsy). So could you please tell me which one of
> those options would you like to use or, maybe, if there is another one that
> could be even better?

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

  class X {...};
  X foo()
    {
    X x;
    return std::move(x);
    }

...is wrong--not just non-optimal, but actually wrong? Here:
  
https://isocpp.org/blog/2013/02/no-really-moving-a-return-value-is-easy-stackoverflow
under the heading "Third example", Howard Hinnant seems to be saying
that it's non-optimal but non-wrong, as long as we capture the return
value immediately as an rvalue reference.

Or is it the case that the original returned a reference, e.g.:

  static std::stringstream&& interpreter{[]() {

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

>  But the important thing is that we really should avoid recreating and
> reinitializing this object all the time as it's really wasteful.

I completely agree. Using std::stringstream operators >> and << is
probably not fast anyway:
  https://lists.nongnu.org/archive/html/lmi/2018-02/msg00036.html
but here Ilja has found a simple way to make it less costly, so
it's well worth doing.

>  Finally, we've, of course, tested this change by checking that the PDF
> illustrations are still produced correctly, but there doesn't seem to be
> any unit test that would allow to exercise this function directly. It's
> used in format_as_{html,tsv}() which are themselves used in various other
> places, but don't seem to appear in any unit (or even integration) tests
> neither. Do you think it would be worth adding a new unit test just for
> this function or is it too trivial for this?
Here I have a concrete idea that I'll explore independently, and
commit if it seems worthwhile.