[Mono-dev] [PATCH] Optimizations for FileSystemWatcher and MulticastDelegate

[Juraj Skripsky]

Hello,

I thought about it some more. Yes, you're right, returning an array
containing this.Clone() is the safest thing to do. Otherwise we might
mix up the chaining...

I'm looking into a more MS.net-like implementation (using an
'invocationList' array instead of a linked list). I think the resulting
code will be much simpler. And the invocation of a chain will be done
iteratively rather than recursively, which is much nicer.

Right now, I'm trying to wrap my head around
mono_marshal_get_delegate_invoke() in marshal.c...

- Juraj


On Thu, 2007-07-19 at 20:20 +0200, Andreas Färber wrote:
> Hello,
> 
> Am 19.07.2007 um 18:34 schrieb Juraj Skripsky:
> 
> > On Thu, 2007-07-19 at 12:04 -0400, Miguel de Icaza wrote:
> >>     can you explain the rationale for the changes in  
> >> MulticastDelegate,
> >> for example, why not return this.Clone () instead of the new  
> >> construct
> >> new Delegate [1] { this }?
> >
> > The new code distinguishes between two cases (see the diff below):
> >
> > a) We don't have a chain, the only delegate to return is 'this'
> >    (or rather an array only containing one element: 'this').
> >
> >    The old code did far too much work in this case:
> >    - clone this
> >    - build forward linked list (0 iterations as d.prev == null)
> >    - enter the if-block (as d.kpm_next was set to null)
> >    - clone the clone
> >    - set other.prev and other.kpm_next to null (they already are!)
> >    - return "new Delegate[1] { other }" where other.Equals(this)
> >
> >    So it cloned the delegate twice and finally returned an
> >    array containing the second clone (which equals 'this').
> >
> >    The new code does the same without all the unnecessary steps.
> >
> > b) This stays the same.
> >
> >
> >      public sealed override Delegate[] GetInvocationList ()
> >      {
> > -        MulticastDelegate d;
> > -        d = (MulticastDelegate) this.Clone ();
> > +        // does the list only contain ourself?
> > +        if (prev == null)
> > +            return new Delegate [1] { this };
> > +
> > +        // walk the list backwards, connecting the links the other  
> > way
> > +        MulticastDelegate d = this;
> >          for (d.kpm_next = null; d.prev != null; d = d.prev)
> >              d.prev.kpm_next = d;
> >
> > -        if (d.kpm_next == null) {
> > -            MulticastDelegate other = (MulticastDelegate) d.Clone ();
> > -            other.prev = null;
> > -            other.kpm_next = null;
> > -            return new Delegate [1] { other };
> > -        }
> > -
> > +        // walk the list, building the invocation list
> >          ArrayList list = new ArrayList ();
> >          for (; d != null; d = d.kpm_next) {
> >              MulticastDelegate other = (MulticastDelegate) d.Clone ();
> 
> I concur with Miguel: The original code apparently returns this.Clone 
> ().Clone() but intended was probably this.Clone(), not this. In  
> general that's a question of reference equality, safety etc.
> 
> Andreas
>