Re: [lmi] Micro-optimization in ledger_format

[Greg Chicares]

On 2019-01-16 14:08, Vadim Zeitlin wrote:
> On Wed, 16 Jan 2019 12:17:49 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2018-11-26 15:53, Vadim Zeitlin wrote:

[... https://github.com/vadz/lmi/pull/104/files ...]

Thanks for explaining why I couldn't read the git history I was looking for.
I saw that there had been a '--force' commit, but thought github was showing
me a diff of revisions preceding and following that forcible change. I guess
such magic is not to be expected.

> GC> I'm not sure whether I like the IIFE technique or not.
> 
>  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.
I'll take another IIFE look at it tomorrow. Today, though, I'm not asking you
to do anything in any repository. I do have a substantive question or two.
Please take a look at
  commit cbb6cade2f89
  Measure speed of various stream-cast implementations
which I'll push presently. There, I added some alternatives to the existing
'stream_cast_test.cpp', which uses the same fundamental
  std::stringstream ss;
  ss << source;
  ss >> destination;
  assert(ss.eof());
technique, and can benefit from the same optimizations as ledger_format().

I've left IIFE out of that commit, because my unfamiliarity with that
technique is just an obstacle to understanding. We can always reconsider
re-IIFE-ifying the code later. But already you'll see that I wrote
  std::stringstream& imbued() // The name will make sense when you read it.
to return a reference, while PR 104 has
  static std::stringstream interpreter{[]() {... return ss;} ()};
where I don't see a reference. How can a std::stringstream be returned
other than by reference, given that it lacks the special member functions
that would seem to be required? Or are reference semantics used implicitly,
somehow?

I'm seeing timings like this:

  Speed tests...
  stream_cast     : 4.707e-006 s mean;          4 us least of 2125 runs
  minimalistic    : 3.638e-006 s mean;          3 us least of 2750 runs
  static stream   : 2.335e-006 s mean;          2 us least of 4283 runs
  static facet too: 2.271e-006 s mean;          2 us least of 4404 runs
  without str()   : 2.207e-006 s mean;          2 us least of 4532 runs

The comments in the left-hand column are terribly terse, but comments in
the code explain what's going on. Because you reported an overall speedup
in PDF generation on the order of ten percent, I figured that this
subroutine would have to become about twice as fast--which is approximately
what I observe, so...no surprise there.

The most important improvement is making the std::stringstream static.
Imbuing a facet statically, OAOO, is a comparatively tiny improvement, at
least for the particular facet used in this unit test. It looks like just
adding the word 'static' (instead of full-blown IIFE) suffices for the one
really dramatic improvement.

I imagine that there's a possibility for error when we call clear() without
at the same time calling str(""). We expect the EOF flag in the static
stream object after each use, so we certainly need to clear that flag
before each reuse; yet clear() would also wipe out the 'bad' and 'fail'
flags, potentially leaving unwanted data in the buffer. We could of course
use rdstate() to test EOF only, but that sounds too complicated, and it's
cheap to call str().

I think this unit test should suffice for deciding what to change; we'll
run a few manual tests anyway to see how much faster PDF generation
becomes on our machines, but ledger_format() doesn't need its own unit
test at this time.