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

[Andreas Färber]

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