lmi
[Top][All Lists]
Advanced

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

[lmi] Micro-optimization in ledger_format


From: Vadim Zeitlin
Subject: [lmi] Micro-optimization in ledger_format
Date: Mon, 26 Nov 2018 16:53:11 +0100

 Hello,

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

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

 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?

 Thanks in advance for your answers,
VZ

[*] https://en.wikipedia.org/wiki/Immediately-invoked_function_expression


reply via email to

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