[m-rev.] the grade library is ready for review

[Paul Bone]

On Tue, Apr 05, 2016 at 12:37:24AM +1000, Zoltan Somogyi wrote:
> 
> 
> On Mon, 4 Apr 2016 14:42:10 +1000, Paul Bone <paul at bone.id.au> wrote:
> > How about:
> > 
> >     $ ./choose_grade debug=debug deep_prof=deep_prof
> > 
> > This also succeeds.  But does this grade work?  Even if it does work it
> > probably doesn't make sense.
> 
> Actually, that combination does not only work, I specifically
> designed it to work. Both debugger and deep profiling need the
> identities of procedures, and I made sure that the executable
> has only one copy of the procedure id of every procedure
> even if both are enabled.
> 
> Without that grade combination, it would been significantly
> harder to debug the implementation of deep profiling itself.
> Non-developer users may not find the combination useful,
> but I did.

Cool.  Can (should?) the grade library distinguish between sensible grades
(for users) / grades that work / grades that do not work?

This is what I would call a not-sensible grade.

> > I also made it crash:
> > 
> >     $ ./choose_grade pregen=pregen
> >     ...
> >     Uncaught Mercury exception:
> >     Software Error: function
> >     `grade_lib.grade_structure.encode_low_tags_floats'/3: Unexpected: pregen
> >     but MercFloat != boxed double
> >     Stack dump not available in this grade.
> 
> That is strange. I just tried that command, and for me,
> it worked just fine. Did you test the current version
> of the library?

Yes, I tried this yesterday against the current "master":
    28ba5e98862976a5e66dc0f3d1234c561f20b0a9 (My change at 13:34:46)

Tested again now against the current master:
    fd440c6b69b3247dded3cfea24f41769f8feeb3c (Your change at Apr 5 9:12:18)
    and the result is the same.

Here's the full output:

$ ./choose_grade pregen=pregen
SUCCESS
gcc_regs_avail = gcc_regs_avail
gcc_gotos_avail = gcc_gotos_avail
gcc_labels_avail = gcc_labels_avail
low_tag_bits_avail = low_tag_bits_avail_3
size_of_double = size_of_double_eq_ptr
mercuryfile = no_mercuryfile
backend = mlds
datarep = heap_cells
target = c
nested_funcs = no_nest
gcc_regs_use = dont_use_gcc_regs
gcc_gotos_use = dont_use_gcc_gotos
gcc_labels_use = dont_use_gcc_labels
low_tag_bits_use = low_tag_bits_use_2
stack_len = stfix
trail = no_trail
trail_segments = trfix
minmodel = no_mm
thread_safe = not_thread_safe
gc = bdw
deep_prof = no_deep_prof
mprof_call = no_mprof_call
mprof_time = no_mprof_time
mprof_memory = no_mprof_memory
tscope_prof = no_tscope_prof
term_size_prof = no_term_size_prof
debug = nodebug
ssdebug = no_ssdebug
lldebug = no_lldebug
rbmm = no_rbmm
rbmm_debug = no_rbmm_debug
rbmm_prof = no_rbmm_prof
pregen = pregen
single_prec_float = no_spf
merc_float = unboxed_double
Uncaught Mercury exception:
Software Error: function `grade_lib.grade_structure.encode_low_tags_floats'/3: Unexpected: pregen but MercFloat != boxed double
Stack dump not available in this grade.


> > Twice:
> > 
> >     $ ./choose_grade ssdebug=ssdebug tscope_prof=tscope_prof 
> >     Uncaught Mercury exception:
> >     Software Error: predicate
> >     `grade_lib.grade_solver.accumulate_why_var_is_not_values'/5: Unexpected:
> >     is_possible
> >     Stack dump not available in this grade.
> 
> That is an expected failure: tscope requires LLDS, but ssdebug
> does not make sense in LLDS grades, since the C debugger is
> significantly better.
> 
> The problem you found is in how the failure is presented: it should
> generate an error message, not a stack dump. I will fix it.

Yes, I expected this to fail, I reported it because it crashed.

It didn't occur to me that ssdebug will work with llds.  However that's
something I'd consider to be "not sensible".

> > > I tried to make the code of the grade library itself easy to read.
> > > Please tell me whether I succeeded.
> > 
> > I've looked over parts of it.  It seems fine.  I like the combinators in
> > grade_solve.m.
> 
> Thanks, but I don't know what combinators you refer to.

init_requirement_specs in grade_spec.m: specifically the use of the
constructors 'being' 'is_one_of' etc in the infix notation.  I didn't look
to see if they were actually combinators (I don't think they are).  Anyway,
what I was referring to was this infix style with English names.  It's a
style that can be used to great effect and IMHO can often be overdone,
making code less readable.  This style has made your code _more_ readable,
so I like it.

> > Some additional questions:
> > 
> >  + Does labeling in the solver choose the "best" grade?
> 
> The labeling step makes its choice using the algorithm described
> in the comment on init_solver_var_specs in grade_specs.m:
> it always sets the first variable in the variable priority list
> whose value hasn't been fixed yet by propagation, and it always
> sets that variable to the first of its values that has not yet been
> ruled out by propagation. That is what its notion of "best" is.
> 
> As for whether this notion of "best" agrees with our subjective
> perception of what is best, that is one of the questions before the court.
> 
> People can try to answer that question by themselves both
> by just trying to stump choose_grade, and by looking at the priorities
> implicit in the list returned by init_solver_var_specs.

These preferences make sense, including the version parameter.  We may want to
tweak the order of the variables if constraints between them are added or other
preferences change.  If we decide to make thread safety the default then it
needs to be moved ahead of stack segments, so that we don't commit to a fixed
size stack before labeling thread safety.

Hrm, this is a form of weak constraint: thread safety in the llds backend
weakly implies a segmented stack.  It's still okay to use fixed size stacks,
but a segmented one improves performance.

> >  + Does it attempt to evaluate all possibilities or will it commit to the
> >    first valid solution?
> 
> When computing an absolute grade, the solver always commits
> to the choices it makes during labeling; it never revisits such choices.
> This is for two reasons: one, I think backtracking, if implemented,
> would take too long, and two, I was pretty sure that backtracking
> would not be needed to avoid failures.

That makes sense to me.  I also don't believe that this would need to
backtrack.

> When computing a grade relative to an installed grade set (i.e. choosing
> the installed grade that best fits the user's requirements), neither of
> those reasons held, so I did implement backtracking. It is optional:
> it is controlled by the should_commit argument to solve_best_installed_grade.
> However, for none of the 160 tests done by test_grades.m does it get
> a different results when backtracking is enabled than when it is not enabled.
> The code of  run_installed_grade_set_test runs each of those tests
> both with and without backtracking, and prints INCONSISTENT if they get
> different results. I never saw that in any output.
> 
> On that basis, I tentatively conclude that backtracking should never be needed
> to find the best solution. I would have to revisit that conclusion if anyone adds
> some tests to test_grades.m that cause it to generate INCONSISTENT.

That may also change if we add constraints or change preferences.  But maybe
not, the more I think about this problem the more I believe that propagation
handles many of the "important" variables early on - the variables that are
likely to break constraints.

> As for attempting to evaluate all possibilities: the solver never does that.
> The try_all_grade_structs tool does, and it shows that there are over a million
> valid grade combinations (modulo the conversation above wrt thread safety).
> While you would only get a fraction of that for any real invocation of the solver,
> you can still easily get thousands, differing from each other in the presence
> or absence of some feature that the user hasn't mentioned at all. I don't see
> any situation whatever in which printing them all would help the user, and
> without that, I don't see why the grade library should ever compute them all.

Depends how the grade library is used to choose a grade relative to the
installed grades.  If the installed grades are used to setup the constraint
problem (need only first solution) or filter the results of the problem (may
need all solutions).

> 
> >  + Can we add / do you plan to add support for other requirements that
> >    aren't compilation model things.  For example, it'd be nice if the user
> >    could say "I want parallel conjunctions", even though this is not a
> >    compilation model setting it implies other settings, in this case:
> >
> >      backend=llds
> >      thread_safe=thread_safe
> > 
> >    Likewise "threading model" might be another thing.  The LLDS has an N:M
> >    threading model while the MLDS grades have 1:1 threading (unless their
> >    targets override this, some old JVMs used to implement N:M).
> 
> I think those would only need three additional solver vars (want par conj,
> want 1:1 thread model, want N:M thread modules) and two requirements
> for each of those vars (e.g. want par conj requires backend=llds, want par
> conj requires thread safe). The cost should be minimal: the solver throws
> away every requirement that it knows won't be needed again. So if want par
> conj is set to "no", both those requirements will be thrown away after being
> tested just once.

While writing this e-mail I thought of other instances of this kind of
thing:

    The user wants "debugging", but doesn't care which debugger is used.
    This relieves the user from needing to know that different debuggers are
    available (to some extent).

        debugging -> debug=debug \/ debug=decldebug \/ debug=ssdebug

    Kind-of the same with profiling.  However the time, memory and deep
    profilers have different capabilities (threadscope doesn't handle normal
    profiling tasks so I'm not including it).  So the user saying "I want
    profiling" doesn't tell us enough, they may get a profiler that can't do
    what they want.  Maybe this is also true for the debuggers but I don't
    know how the normal and ssdebuger are different in terms of capability.

    Of course the user still needs to know which debugger or profiler they
    got so that they can invoke it.


> Thanks for the review.

No problem.


-- 
Paul Bone