Re: [lmi] PDF generation performance

[Vadim Zeitlin]

On Wed, 7 Feb 2018 23:01:50 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-02-07 21:49, Vadim Zeitlin wrote:
GC> > On Wed, 7 Feb 2018 21:40:46 +0100 I wrote:
GC> > 
GC> > Me>  I think it should be possible to obtain much more than microscopic
GC> > Me> refinements.
GC> > 
GC> >  Just FYI, applying this simple and still not really optimal (because
GC> > operator<<() is very slow on its own) patch:
GC> > 
GC> > ---------------------------------- >8 
--------------------------------------
GC> > diff --git a/ledger_text_formats.cpp b/ledger_text_formats.cpp
GC> [...]
GC> > -    std::stringstream interpreter;
GC> > -    std::locale loc;
GC> > -    std::locale new_loc(loc, new comma_punct);
GC> > -    interpreter.imbue(new_loc);
GC> 
GC> Gospodi, pomiluy! Please tell me we don't really do such a thing.

 Well...

 Can I skip answering this?

GC> > speeds the code by 50% for me, i.e. makes it exactly twice faster (status
GC> > bar shows ~6800ms instead of ~13700ms for me) without any non-trivial
GC> > change in the generated output files (they're still different solely
GC> > because the "CreationDate" field in the generated PDFs changes -- I think
GC> > it would be nice to have some way to call wxPdfDocument::SetCreationDate()
GC> > with some fixed date for testing to allow checking for the changes in
GC> > output just using diff, instead of having to use diffpdf as I do now).
GC> 
GC> We've always used a fixed date in this part of 'ledger_xml_io.cpp':
GC> 
GC>     // Skip authentication for non-interactive regression testing.
GC>     if(!global_settings::instance().regression_testing())
GC>         {
GC>         authenticate_system();
GC>         }
GC>     else
GC>         {
GC>         // For regression tests,
GC>         //   - use an invariant string as version
GC>         //   - use EffDate as date prepared
GC>         // in order to avoid gratuitous failures.
GC>         LmiVersion = "Regression testing";
GC>         
prep_date.julian_day_number(static_cast<int>(ledger_invariant_->EffDateJdn));
GC>         }

 Yes, but this code is twice removed from pdf_writer_wx.cpp, which is the
only place where wxPdfDocument is used. Would it shock you to redo the same
regression_testing() test in that file and use some arbitrary fixed date
(any preference?) for the PDF creation date in this case?


GC> >  I don't necessarily advocate applying the patch above (although neither 
do
GC> > I see anything really wrong with doing this), but IMO this definitely
GC> > proves my quoted statement in the beginning of this email.
GC> 
GC> Although value_cast<> doesn't do the same thing (it shows all the
GC> numeric precision it can, instead of rounding), I wonder if some
GC> implementation layer beneath it does exactly what we want here.

 I don't think so, it doesn't support putting commas between digit groups
at all AFAICS, and this is presumably important here. So I'm afraid we'd
need to write a new function just for formatting (the integral part of)
numbers with commas. Of course, it's not exactly rocket science, even if
it's probably less trivial than it seems.

 For the reference (because I don't really think you would want to
integrate it just to solve this particular problem), I've been following
the development of https://github.com/fmtlib/fmt library for quite some
time and while I didn't use it in any real projects yet, it seems very nice
and I definitely prefer its syntax to stream insertion operators (well, to
be fair, I prefer just about anything to stream insertion operators). And
it does support this functionality as well.

 Anyhow, please let me know what do you think about my suggestions to:

(a) Use fixed data in pdf_writer_wx.cpp when regression_testing()
(b) Write a fast helper function for number formatting

 Thanks in advance,
VZ