[lmi] UpdateViews() IRR optimization [was: _M_range_check fails when running "new" PDF]

[Greg Chicares]

On 2018-02-15 10:51, Greg Chicares wrote:

[...patch to make sure all IRR columns are initialized and contain the
expected number of values, using -100% each year in cases where no IRR
can be calculated...]

> I'd hesitate to make such a change anyway--especially the first
> section in the patch above--because in 'ledger_text_formats.cpp'
> we have this code:
> 
>     if(length != invar_.IrrCsvCurrInput.size())
>         {
> ..
>         if(...something else too...)
>             {
>             unclean.CalculateIrrs(ledger_);
>             }
>         else
>             {
>             unclean.IrrCsvCurrInput.resize(length, -1.0);
>             [etc.]
> 
> which is conditional on an IRR variable having an unexpected size,
> for some reason that must have seemed good at the time but isn't
> apparent from `git show 6703fd36` or explained in 'ChangeLog'.

I now see what the motivation was.

> This command:
>   grep 'IrrCsv.*size\|IrrDb.*size' *.?pp |less -S
> suggests that no such conditional is used anywhere else. Although
> it's written as:
>     if(length != invar_.IrrCsvCurrInput.size())
> perhaps the idea was really:
>     if(invar_.IrrCsvCurrInput.empty())
> because I can't think of any size it could have other than zero
> or 'length'. In that case, the condition apparently means
>     if(CalculateIrrs() hasn't been called yet)
> but I can't see how it can already have been called.

This code is reached not only when freshly-calculated results
are shown in a calculation summary, but also when the set of
columns to be shown changes (i.e., when Skeleton::UpdateViews()
is triggered). In the latter case, CalculateIrrs() may indeed
already have been called, in which case it needn't be called
again; or...

...or the IRR columns may have been filled with -100% (because
there was no IRR column in the original selection), but now
need to be calculated...but that calculation doesn't occur
because the condition
    if(length != invar_.IrrCsvCurrInput.size())
is satisfied by the -100% filler. IOW, the condition is wrong
in this case.

> Even if it
> had, calling it again couldn't make the IRR values incorrect.

Calling CalculateIrrs() unconditionally would guarantee
correctness, but it has a cost, which we already measured:

http://lists.nongnu.org/archive/html/lmi/2018-02/msg00098.html
| it takes about twelve milliseconds.

and which, in the UpdateViews() case, may apply to multiple
child windows.

Typical case: The number of IllustrationView windows is anywhere
from zero to maybe three. Three times twelve ms isn't a lot,
though it does mean the application feels less "snappy".

Extreme case: One hundred such windows are open, and any user
who reaches this point presumably recognizes that opening any
more would transcend absurdity. Optimization saves twelve
hundred ms here, which is about one second, but we shouldn't
make a general decision based on a silly extreme scenario.

Conclusion: We've already decided that the calculation summary
must be snappy, so we won't call CalculateIrrs() unconditionally.
The problem is that the original condition was wrong, so that's
what we should fix.

> I really think that this conditional was intended only as a
> speed optimization, but never fulfilled such an intention:
> "das ist nicht nur nicht richtig, es ist nicht einmal falsch!"

Apparently it's simply wrong, as opposed to "not even wrong".
To see that it certainly is wrong (before commit a874072b6):

  File | Preferences
    "View" tab: make sure no 'Irr...' column is selected
  OK

  File | New | Illustration
  OK

  File | Preferences
    "View" tab: select an 'Irr...' column (e.g., 'IrrDbGuaranteed')
  OK

The IRR column has been added to the calculation summary, and it
shows "-100.00%" in all years.

  Illustration | Edit cell
    "Comments" field: type "x"
    [i.e., change anything at all]
  OK

Now the IRR column contains calculated values, not all "-100.00%".

Consider a slightly different scenario:

  [deselect all IRR columns as above]

  File | New | Census
  Census | Run case

  [select one or more IRR columns as above]

The composite shows "-100.00%" as above, but this time there's no
simple trick like adding an "x" to "Comments" and just hitting "OK":
the entire census has to be rerun in order to get real IRRs, and
that's yicky. The yickiness goes away with commit a874072b6.