Re: implementation plan for music streams

[Erik Sandberg]

Hi,

Sorry for the delay. Here's a new revision of the dispatcher system. Known 
issues:
- Some trivial changes should be done to other files, e.g. lily.scm and 
lily-proto.hh.
- I added the unique_ member to Context. It's just an int that's supposed to 
be unique for each context. unique might not be the best name; suggestios for 
better names are welcome.

And some comments:

On Wednesday 05 April 2006 14.10, Han-Wen Nienhuys wrote:
> Erik Sandberg wrote:
> > Some known issues:
> > - scm/define-event-classes.scm contains rather unsorted functions which
> > are
>
> i'm missing that file.

sorry, attached.

> > - The Stream_event class duplicates its 'context property with a context_
> > member; this was originally intended to give speedups, but it is broken
> > in this version and requires some modifications to Context in order to
> > work. I'll probably remove the context_ member altogether in the next
> > revision.
>
> yes please do.
>
> > /*
> > Event dispatching:
> > - Collect a list of listeners for each relevant class
> > - Send the event to each of these listeners, in increasing priority
> > order. This is done by keeping a bubble-sorted temporary list of listener
> > lists, and iteratively send the event to the lowest-priority listener. -
> > An event is never sent twice to listeners with equal priority. */
> > IMPLEMENT_LISTENER (Dispatcher, dispatch) (Stream_event *ev)
> > {
> >   SCM class_symbol = ev->get_property ("class");
> >   if (!scm_symbol_p (class_symbol))
> >     {
> >       warning (_f ("Unknown event class %s", ly_symbol2string
> > (class_symbol).c_str ())); return;
> >     }
> >
> >   SCM class_list = scm_primitive_eval (class_symbol);
>
> ugh. WTF is this? Where does this come from, in what module should it be
> defined. Why does this do an eval() for every dispatch() call?

I used eval as a poor man's hashq. I have cleaned it up a bit now, by 
abstracting the eval call.

> >   bool sent = false;
> >
> >   // TODO: fix this loop.
> >   int num_classes = 0;
> >   for (SCM cl = class_list; cl != SCM_EOL; cl = scm_cdr (cl))
> >     num_classes++;
>
> scm_ilength

thanks

> >   // Collect all listener lists.
> >   struct { int prio; SCM list; } lists[num_classes+1];
> >   int i = 0;
> >   for (SCM cl = class_list; cl != SCM_EOL; cl = scm_cdr (cl))
> >     {
> >       SCM list = scm_hashq_ref (listeners_, scm_car (cl), SCM_EOL);
> >       if (list == SCM_EOL)
> >     num_classes--;
> >       else
> >     {
> >           // bubblesort.
> >           int prio = scm_to_int (scm_caar (list));
> >       int j;
> >       for (j = i; j > 0 && lists[j-1].prio > prio; j--)
> >         lists[j] = lists[j-1];
> >       lists[j].prio = prio;
> >       lists[j].list = list;
> >       i++;
> >         }
> >     }
> >   lists[num_classes].prio = INT_MAX;
>
> can you use a Scheme sort routine to do this?
>
> do I understand correctly that for every time step, we get multiple
> bubble sorts? That doesn't look very clean?

The bubble sorts are just a primitive implementation of a priority queue. The 
queue typically has two elements (the height of the event-class tree), so I 
felt that using pure C and simple bubble-sort would be the most efficient way 
to do it. The main reason for C is that the stack is used for memory 
allocation; I suspect this would be much slower in guile due to GC.

> > #if 0
> > /*
> >  New listeners are appended to the end of the list.
> >  This way, listeners will listen to an event in the order they were
> > added. */
>
> why if 0 ?

sorry, obsolete code

> >   // We just remove the listener once.
> >   bool first = true;
> >
> >   SCM dummy = scm_cons (SCM_EOL, list);
> >   SCM e = dummy;
> >   while (scm_cdr (e) != SCM_EOL)
> >     if (*unsmob_listener (scm_cdadr (e)) == l && first)
> >       {
> >     scm_set_cdr_x (e, scm_cddr(e));
> >     first = false;
> >     break;
> >       }
> >     else
> >       e = scm_cdr (e);
> >   list = scm_cdr (dummy);
>
> try to use scm_delq or similar, if not possible, devise an appropriate
> del() routine yoursefl.

Thanks, scm_delete seems to do the job (I didn't realise I had defined an 
equality predicate)

> > #ifndef NDEBUG
> > //  assert (SCM_EOL == scm_hashq_ref (listeners_, ly_symbol2scm
> > ("StreamEvent"), SCM_EOL)); #endif
>
> idem.
>
> > LY_DEFINE (ly_make_dispatcher, "ly:make-dispatcher",
>
> as a matter of style, this should be in dispatcher-scheme.cc
>
> > /*
> >   listener-scheme.cc -- Connect listeners to Scheme through Scm_listener
> >
> >   source file of the GNU LilyPond music typesetter
> >
> >   (c) 2005-2006 Erik Sandberg  <address@hidden>
> > */
> >
> > #include "listener.hh"
> > #include "ly-smobs.icc"
> > #include "stream-event.hh"
> >
> > class Scm_listener
>
> this should be in scm-listener.cc
>
> > {
> > public:
> >   Scm_listener (SCM callback);
> >   DECLARE_LISTENER (listener);
> > protected:
> >   DECLARE_SMOBS (Scm_listener,);
> > private:
> >   SCM callback_;
> > };
> >
> > IMPLEMENT_LISTENER (Scm_listener, listener) (Stream_event *ev)
>
> Please change the def of this macro so we  can have
>
> IMPLEMENT_LISTENER (Scm_listener, listener);
> Scm_listener::real_declaration (Stream_event *)
>
> otherwise tools like TAGS get very confused.

I have changed the definition to:
IMPLEMENT_LISTENER (Scm_listener, listener, (Stream_event *ev))
{
...
}

Your suggestion doesn't work well because of some magic inside the macro.

> > LY_DEFINE (ly_make_listener, "ly:make-listener",
>
> scm-listener-scheme.cc

Scm_listener is only intended to be used locally by that function; splitting 
the file into two modules would feel artificial/meaningless. Perhaps I should 
rename the class to Listener_scheme?

> > // ES todo: Add stuff to lily-proto.hh: Stream_event and its subclasses,
> > Stream_creator, etc.
>
> yes. in any case, I'm missing a patch.
>
> > SCM
> > Stream_event::internal_get_property (SCM sym) const
> > {
> >   SCM s = scm_sloppy_assq (sym, property_alist_);
> >   if (s != SCM_BOOL_F)
> >     return scm_cdr (s);
> >   return SCM_EOL;
> > }
>
> you might want to consider basing these objects on Prob; see prob.cc

Is there a point in doing that? There are no immutable properties

> > #define SEND_EVENT_TO_CONTEXT(ctx, cl, ...)                         \
> >   {                                                                 \
> >     Stream_event *_e_ = new Stream_event (ctx, ly_symbol2scm (cl)); \
> >     __VA_ARGS__;                                                    \
> >     ctx->event_source ()->distribute (_e_);                         \
> >     scm_gc_unprotect_object (_e_->self_scm ());                             
> > \
> >   }
> >
> > #define EVENT_PROPERTY(prop, val)   \
> >   (_e_->set_property (prop, val))
>
> what's this? Is it ever used?  It looks fishy anyway.

It's used about 15 times so far. It's a shorthand for creating and reporting 
an event; e.g., the following code generates and broadcasts a Revert event:

      SEND_EVENT_TO_CONTEXT (get_outlet (), "Revert",
                             EVENT_PROPERTY("symbol", sym),
                             EVENT_PROPERTY("property", eprop));


> In general, it seems that Dispatcher is not connected to Stream_event at
> all. Why not make

I guess you mean listeners. I have now generalised them.

-- 
Erik

define-event-classes.scm
Description: Text Data

context-unique.diff
Description: Text Data

dispatcher.cc
Description: Text Data

stream-event.cc
Description: Text Data

stream-event-scheme.cc
Description: Text Data

dispatcher-scheme.cc
Description: Text Data

listener-scheme.cc
Description: Text Data

listener.cc
Description: Text Data

listener.hh
Description: Text Data

dispatcher.hh
Description: Text Data

stream-event.hh
Description: Text Data