Re: [lmi] Pagination anomaly

[Greg Chicares]

On 2018-02-06 00:16, Vadim Zeitlin wrote:
> On Mon, 5 Feb 2018 22:04:02 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC> I think it's better for me to "apply your pull-request" (or "merge
> GC> your branch", or however a git maven would express that).
> 
>  "Merge" works with both "PR" and "branch", and I definitely prefer if you
> could please do it like this, this makes things simpler for me (and,
> hopefully, not at your expense).

It's actually easier for me, too. Let me emphasize that when I "merged"
this, I did not run 'git merge': instead, I ran 'git cherry-pick', as
documented here:
  
http://git.savannah.nongnu.org/cgit/lmi.git/commit/?id=d60420219f8bcf4462627bb91b14171381d8ec67
which makes reference to this mailing-list discussion:
  On the merits of cherry-picking vs. merging, see:
    http://lists.nongnu.org/archive/html/lmi/2017-11/msg00013.html

> I post small patches here just to show
> them to you immediately, saving you a click on the URL

Don't feel obligated to do that in general (unless there's a
particular reason to inline a small patch, or a small part of one,
to make the discussion clearer). Usually I click the URL anyway.
(It's easier to read, because 'chrome' themes are readily
configured, but 'thunderbird' is balky.)

> GC> In some ideal sense it seems that it ought to be possible to abstract
> GC> out the pagination code into a small, easily-testable routine. But
> GC> AFAICS that would essentially mean taking a vector of the vertical
> GC> sizes of a set of blocks, and comparing its total to the available
> GC> page size.
> GC>   P = Page length
> GC>   H = Header (vertical) size
> GC>   F = Footer size
> GC>   Q = size of each Quinquennial group of table elements
> GC> Then the problem is determining the maximum N such that
> GC>   P - H - F >= N*Q
> GC> with due regard to padding between blocks.
> 
>  Yes, it's indeed as simple as that and looks totally trivial. As you can
> see, this didn't prevent me from making at least 4 errors in it :-(

Then would it make sense to abstract out the pagination code,
and add a unit test like the one you posted yesterday?

[... page_with_tabular_report::render() and get_extra_pages_needed():
using one to test the other now, and then using only one in production ...]

> GC> > One disadvantage of doing it like this is that
> GC> > get_extra_pages_needed() is currently more efficient than render(),
> GC> > but I'm not sure if it's really noticeable.
> GC> 
> GC> Can you measure it easily?
[...]
> 40000000 calls to each function took 0.3s for the version
> directly computing the number of pages, as get_extra_pages_needed() does,
> and 5.4s for the version doing it by iterating over all years, as render()
> does, i.e. a slowdown of 18 times, which is much more than I expected.
> 
>  Of course, this is a very artificial benchmark as it does it so many times
> and for the number of years up to 200, doing the same 40000000 calls to it
> but with the years only up to 100 bring the time of the slow version down
> to 2.7s, and while even this is still 9 times slower, it almost certainly
> doesn't matter inside lmi where other things should dominate it. But I
> won't be sure of this before I can update lmi and re-profile it.

If 40M calls take X seconds, then 40 calls take X microseconds, so the
difference is only microseconds in practice, and no one will notice.

> GC> BTW, we currently have a header that only defines
> GC> 
> GC> enum enum_output_mode
> GC>     {e_output_normal
> GC>     ,e_output_measure_only
[...and I'd like to...]
> GC> rename everything so that whole entity is self-documenting--e.g.:
> GC> 
> GC> enum oenum_render_or_measure
> GC>     {oe_render
> GC>     ,oe_measure
> GC>     };
[...]
>  No, although I slightly regret the loss of "only" as it underscored that
> rendering also does measuring anyhow.

Yes, thanks, that's a good point, so I put 'only' back in. This change
(recently pushed) raises an incidental question: have we agreed that
it's generally preferable for enumerative 'switch' statements to write
a 'case' for every enumerator, and rely on the compiler to identify
any missing case? The
    switch(output_mode)
statements in 'pdf_writer_wx.cpp' list every possible 'case' and
also 'default:'. It looks like we're using the appropriate gcc
warning option, because if I comment out one case and 'default:'
I get:

error: enumeration value �oe_only_measure� not handled in switch 
[-Werror=switch]
     switch(output_mode)

and IIRC we concluded that this compile-time diagnostic is more
useful than a run-time diagnostic that requires extra code.
Maybe I should remove 'default:' wherever possible throughout
lmi, once and for all.

[...two pagination implementations...]

>  The problem is embarrassingly simple, but there is just enough
> comparisons/divisions in it to make it easy to make a mistake in one of
> them... It doesn't make it hard, but it does make it error-prone, and even
> if I'm reasonably certain that I did do it correctly, finally, after
> writing my test and verifying that it passes for all values, it might still
> be worth to keep both functions to ensure it doesn't happen again.
> 
>  The main negative of doing it I see is that it will make modifying this
> code more difficult in the future, as it will need to be done in 2 (albeit
> neighboring) places. However I don't believe this code would need to be
> modified often, so it's probably not a big deal.

More difficult, for a limited future time only, as long as we decide
to keep only one after the conclusion of acceptance testing.