[m-dev.] For review: another change to the external debugger

[Erwan Jahier]

Fergus Henderson wrote:

> Isn't that rationale the rationale for both (1) and (2), not just the
> rationale for (2)?  If it's not, then what's the rationale for (1)?

Both (1) and (2). The last sentence was the rationale for (1). I will
re-word that.

Note that (1) and (2) are dependants: as arguments slot can be very big,
we want to be able to retrieve arguments separately from other slots. So
if we don't test whether or not an attribute is needed, we need to
separate the retrieval of the arguments.

> I'm not sure I believe this rationale.  What makes you think that
> testing whether or not an attribute is wanted is going to be more
> expensive than sending it across a socket?

My intuition: do not perform the test is more efficient then performing
it. The size of the message send to the socket is roughly the same, if
not smaller. For example if you consider a request where only the
chronological event number is wanted, which is the worse case with my
method:

[deb -> mercury] current(yes, no, no, no, no, no, no, no, no, no). 
[mercury -> deb] current(yes(3), no, no, no, no, no, no, no, no, no).

[deb -> mercury] current_slots. 
[mercury -> deb] current_slot(3, 3, 2, exit, "module_name", "pred_name",
3, 0, 0, "").

it's still taking less place.

 
> It may well be that your change does improve efficiency.
> But if so, the rationale given above does not explain it.
> So you need to come up with a more convincing rationale.

What about:

There are 2 parts in this change (1) do not test whether or not an
attribute is wanted to retrieve it and (2) make the retrieval of
arguments and other slots done separately. The rationale for (1) is that
this test is very expensive (11 attributes times possibly millions of
events) and it does not allow to reduce (when not increase)
significantly the size of messages sent to the socket. The rationale
for (2) is that since arguments slot can be very big, we still want to
be able to retrieve any slot without retrieving arguments; so we
retrieve that slot separately.


> But I have some comments on the details of your change anyway.
> > library/debugger_interface.m:
> >       Split 'current' requests into 'current_var' and 'current_slots';
> >       'current_var' requests for live variables and 'current_slots' for the
> >       other slots.
> 
> I think it should be named `current_vars' not `current_var'.

Agreed.

> 
> > -                     case MR_REQUEST_CURRENT:
> > +
> > +                     case MR_REQUEST_CURRENT_VAR:
> >                               if (MR_debug_socket) {
> >                                       fprintf(stderr, "\nMercury runtime: "
> >                                               "REQUEST_CURRENT\n");
> 
> s/REQUEST_CURRENT/MR_REQUEST_CURRENT_VAR/
> 
> And also s/VAR/VARS/ here and elsewhere.

yop.

> 
> > +                     case MR_REQUEST_CURRENT_SLOTS:
> > +                             if (MR_debug_socket) {
> > +                                     fprintf(stderr, "\nMercury runtime: "
> > +                                             "REQUEST_CURRENT\n");
> 
> s/REQUEST_CURRENT/MR_REQUEST_CURRENT_SLOTS/
> 
> > +                             MR_output_current_slots(layout,
> > +                                                     port,
> > +                                                     seqno,
> > +                                                     depth,
> > +                                                     path);
> 
> How about writing things horizontally rather than vertically?
> 
>                                 MR_output_current_slots(layout, port, seqno,
>                                                         depth, path);

Sure.

> 
> >  static void
> > -MR_output_current(const MR_Stack_Layout_Label *layout,
> > -     MR_trace_port port, Unsigned seqno, Unsigned depth,
> > -     Word var_list,
> > -     const char *path, Word current_request)
> > +MR_output_current_slots(const MR_Stack_Layout_Label *layout,
> > +     MR_trace_port port, int seqno, int depth, const char *path)
> 
> Why are the types here `int' instead of `Unsigned'?

No reason. I'll change it back.

> 
> > +++ mercury_init.h    1998/07/24 15:25:52
> > +void ML_DI_output_current_slots(Integer, Integer, Integer, Word, String,
> > +             String, Integer, Integer, Integer, String, Word);
> > +             /* normally ML_DI_output_current_slots (output_current_slots/13) */
> 
> s/13/11/ ?

I don't think so; output_current_slots has 13 arguments ("the same first
11 ones as in ML_DI_output_current_slots" + io_state::in +
io_state::out).

------------------------------------------------------

I will send another diff when you are happy with my log message.



-- 
R1.