[m-rev.] for post-commit review: monomorphize some folds
Peter Wang
novalazy at gmail.com
Sat Dec 3 11:39:11 AEDT 2022
On Fri, 02 Dec 2022 20:39:56 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
>
>
> On Fri, 02 Dec 2022 19:22:38 +1100 (AEDT), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> > I see. The culprit must be the change chunk starting at line 626
> > of DIFF.mono, which replaced this
> >
> > - foldl2_maybe_stop_at_error(KeepGoing,
> > - foldl2_maybe_stop_at_error_maybe_parallel(KeepGoing,
> > - make_module_target),
> > - Globals, [Int1s, Opts], Succeeded, !Info, !IO)
> >
> > with two sequential folds, one over Int1s, and one over Opts.
> > These should both be parallel folds. I am bootchecking that fix now.
>
> The attached committed diff should fix the regression.
Thanks.
> However,
> you may want to try replacing the sequential fold making the .int3
> files with a parallel fold. I am pretty sure it would work, though
> I am not sure it would yield a speedup, because the job control
> overheads may cost more time than what the parallelism may
> buy you. It should also be possible to make the .int0 files in
> parallel for separate submodules, though I am skeptical
> about that being also true for nested submodules,
> and I don't think the data structures available in that predicate
> distinguish the two.
I will try it next week. As you say the savings to be made would be
small.
>
> I also note that forkable_module_compilation_task_type returns
> NO for making .intN for all values of N, though I have no idea
> how the call to that predicate on line 388 of make.module_target.m
> is supposed to interact with the test on the next line; why test
> for forkability on a code path that you take only if you cannot fork?
forkable_module_compilation_task_type is misleadingly named and should
be renamed. Its meaning is, "SHOULD/MUST this task, to be performed by
the Mercury compiler, be performed in a separate process or the current
process?"
If a task SHOULD/MUST be performed in a separate process but we cannot
fork, then mmc --make will need to invoke the Mercury compiler with a
bunch of command line arguments. This potentially runs into issues with
the maximum length of a command line, so line 391 on
make.module_target.m deals with creating a temporary file to hold those
command line arguments.
(I had to investigate this, so this should be explained in comments,
yes.)
Peter
More information about the reviews
mailing list