[m-rev.] for review: Optimise modechecking of coerce for large types.

[Peter Wang]

On Tue, 20 Apr 2021 15:44:21 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 2021-04-20 15:16 GMT+10:00 "Peter Wang" <novalazy at gmail.com>:
> > @@ -179,6 +180,15 @@ modecheck_coerce_vars(ModuleInfo0, X, Y, InstX, InstY, Res, !ModeInfo) :-
> >     uniqueness::in, mer_type::in, mer_inst::out) is det.
> >  make_bound_inst_for_type(ModuleInfo, Seen0, Uniq, Type, Inst) :-
> > +    make_bound_inst_for_type_with_cache(ModuleInfo, Seen0, Uniq, Type, Inst,
> > +        map.init, _Cache).
> 
> This code throws away the cache after each call to make_bound_inst_for_type.
> Are you sure that the benefit of keeping the cache *across* calls, either
> in a data structure kept by its caller(s) or in a mutable, is less than the
> cost of the required code complexity?
> 

You're right. It would be useful to keep the cache around, in the
mode_info.

> Have you measured the benefit you get from the cache?
> 

I was experimenting with subtypes in a few places in the Prince code base.
One particular coercion was very slow to modecheck: the overall time to
errorcheck that module went from ~9.3s to ~1.5s after the changes in
this patch.

But without that coercion, the time to error check the module was
~0.67s, so that's obviously still terrible. I'm going to abandon this
patch and try to implement modechecking of coerce in some other way that
does not involve generating potentially gigantic insts.

> > @@ -292,6 +316,16 @@ ground_or_unique_inst(Uniq) =
> >  %---------------------------------------------------------------------------%
> > +:- type bound_inst_cache == map(type_and_bound_inst, mer_inst).
> > +
> > +:- type type_and_bound_inst
> > +    --->    type_and_bound_inst(
> > +                mer_type,
> > +                % These fields are from a bound() inst.
> > +                uniqueness,
> > +                list(bound_inst)
> > +            ).
> > +
> >     % Check that a bound() inst only includes functors that are constructors of
> >     % the given type, recursively. Insts are otherwise assumed to be valid for
> >     % the type, and not checked to be valid for the type in other respects.
> > @@ -303,6 +337,15 @@ ground_or_unique_inst(Uniq) =
> 
> I would have expected to see the definition of the cache type
> either before the first code that uses the cache. or just before the code
> that implements the cache. Seeing it in the middle, next to code that it is
> not strongly related to, is ... strange.
> 
> Since in this case the cache is added to in several places, putting the
> type definition up front would seem to be better.

Not that it matters now, but there were two caches involved.
You may have confused them.

> I think there should already be something in the type table entry for TypeCtor
> that should do the job of CtorsMap, even if it is not exactly equivalent to it.
> That should be built just once, when TypeCtor is defined. If I am wrong, and
> there no nothing relevant in the type table, then I think we should fix that.
> 

You're right, the cons_table would have worked.

Peter