[Mono-dev] [PATCH] Optimizations for FileSystemWatcher and MulticastDelegate
Andreas Färber
andreas.faerber at web.de
Thu Jul 19 14:20:25 EDT 2007
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
More information about the Mono-devel-list
mailing list