Re: async-calls and Dispatchers

On Thu, 2007-12-20 at 22:30 -0700, Alex Rousskov wrote:
> On Fri, 2007-12-21 at 09:00 +1100, Robert Collins wrote:
>
> > Please do read the
> > 'proactor' ACE pattern if you haven't.
> > http://www.cs.wustl.edu/~schmidt/PDF/proactor.pdf
>
> I only had time to skim that paper, but I think what they call
> Completion Dispatcher is what we call "comm". Their Completion
> Dispatcher class is I/O-aware and converts I/O notifications (possibly
> supporting various models) into callbacks to the core.

Its the callback side of comm + aufs + diskd + aio + disk - all the
things that actually gather results from concurrent operations.

> I see no connection between CompletionDispatcher in Squid and the
> Completion Dispatcher in that article, except for the name.

For two reasons; I only got the core in place before I ran out of time
to hack at that point; secondly with the single exception of signals and
aufs we don't have anything that is directly OS proactive - all the
interfaces we use are reactive, and as such they are thunked onto the
proactor design (as the other website you referenced discusses) by
polling the os level reactive tasks and generating callbacks to issue
for them. Aufs should be converted to hand off directly to a dispatcher
so that calling its engine in each loop becomes simply a check for 'has
outstanding requests'.

> The above does not explain to me why you need both classes. You can do
> all of the above with the current AsyncEngine interface alone. Perhaps
> you assign some unimplemented semantics to Dispatchers and we end up
> discussing two different things: you talk about the intended/future API,
> while I talk about the deleted code.

I think that is likely the case.

> > But looking at the patch I can see that all you've done is move the
> > abstract interface one level further out. So the entire discussion is
> > really moot - I don't care if its a level further out or not, though I
> > think the extra indirection you are adding via
> > AsyncCallQueue::Instance().fire() is more complexity not less.
>
> I do not know what interface levels are here, but I am glad you do not
> have a problem with deleting CompletionDispatcher classes after all.

I'm not happy about it, but you are working on a larger cleanup which
I'm very keen to see happen. I don't think that the bit we're talking
about substantially influences the cleanup you are doing either way, but
I would be deeply unhappy if I caused that cleanup to be delayed or not
happen because of this discussion. So I'd rather see the
CompletionDispatcher stuff deleted and the other cleanups come in, and
revisit it later, than block up-front on a design discussion which is
essentially about future work (such as better native win32 support).

> > More importantly I can't see any unit tests. If we want to end up with a
> > rigourously tested code base - which is both well worth doing and
> > something I thought we as a group were interested in - replacing tested
> > code with untested code is a step backwards.
>
> We are not moving in just one dimension here. I agree that rigorously
> tested code base is a goal. Yes, we will probably lose some old unit
> tests while simplifying the code. Better code and/or new tests will
> probably compensate for that.

I'd like to note that what you are removing and replacing was *new code*
and *new unit tests* not so long ago. I think its reasonable to ask that
when replacing tested code, it be replaced with tested code. (I do take
your point about moving in > 1 dimension).

> >
> > I'm not sure if you are saying that you are adding global variables
> > because its 'simpler' (its not, and I'm *really* against more
> > globals)
>
> I do not recall adding global variables. The old code just hid them
> behind register(Instance()) calls. We can quickly hide them again if
> needed.

The old code had the following structure IIRC:
in main.cc an instance of EventLoop.
various globals inherited from the squid2 event loop registered with the
EventLoop instance (using Instance() calls).

So the code in EventLoop was free of globals. You can run multiple
EventLoops (e.g. one in each thread), or do other interesting things.

Other parts of the code base not migrated yet still used globals, which
is simply reflecting that the job of removing globals is not complete.

Making EventLoop use another classes Instance() is a step back in
removing globals IMO.

-Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.