[Mono-devel-list] Patch for System.Timespan

[Ben Maurer]

On Sat, 2004-04-17 at 14:08, Andreas Nahr wrote: 
> > > >       * You are intermingling formatting changes, bug fixes, and
> > > >         performance improvements. What I would suggest you do is the
> > > >         following:
> > > >               * Check in all formatting related changes that you want.
> > >
> > > That didn't seem to make sense to me with the reimpl. approach.
> > It actually makes alot of sense. If you check in formatting changes
> > *FIRST* and then do other fixes, the diff is alot cleaner. Right now,
> > the diff basically looks like you removed all methods from the class and
> > rewrote it. The diff should show:
> 
> Maybe it look like this because I did it exactly that way ;) Reimplemented
> it from scratch copying over the few parts that worked.
> I simply did not see any sense to make individual patches because that would
> simply be too many of them.

Ok, let me give you an example of what I am talking about:


> -	public int Seconds
> -	{
> -		get
> +		public TimeSpan (int days, int hours, int minutes, int seconds)
> +			: this (days, hours, minutes, seconds, 0)
>  		{
> -			return (int) (_ticks % TicksPerMinute / TicksPerSecond);
>  		}
> -	}
..
> -	public TimeSpan Add (TimeSpan ts)
> -	{
> -		checked {
> -			return new TimeSpan (_ticks + ts.Ticks);
> +		public int Seconds {
> +			get {
> +				return (int) (_ticks % TicksPerMinute / TicksPerSecond);
> +			}
>  		}
> -	}
So, in the Seconds property, the only thing you changed was some whitespace. However:
      * The property got moved around the file. I had to search for it
        in the diff 
      * In order to see what changes you made, I had to compare the two
        versions of the property.
It is just not possible to review patches like that. The way to fix this
is to first, format the timespan file to your content. When you do diff
-b, you should get no changes.

After this, you can reintroduce the changes you made to exception
messages, etc. Those are not really `meaningful' so you can check them
in (but separately from the whitespace changes).

Once these two things are done, you can merge your patch back in (you
will likely have to do it by hand).

> > > There are A LOT of them. Would anybody have much for insight if I posted
> > > 40-50 individual patches for that?
> > I would rather review 10-20 atomic patches than 1 patch that I could not
> > understand. As it stands, your patch is unlikely to be reviewed, meaning
> > it is very unlikely to see the light of day.
> 
> I surely can understand that a atomic patch makes sense if you would have to
> compare the new TimeSpan to the old one. If one or some lines in a function
> are CHANGES.
The old method and the new method should be next to each other in the
patch, which is why I want you to do formatting changes first. This
eases reviewing.

> However for all non-tivial changes that require review there is
> not a single original line left in those functions (e.g. ToString, FromXXX,
> Operators). So I do not see any advantage in sending 40 individual patches.
> Do you REALLY feel that reviewing 40 individual patches (which is about how
> much patches get sent to this list in a month or few weeks) is less work
> than a 11kb source file. I would not.
Here is the problem -- if you submit one patch, you are putting all your
eggs in one basket. In order for the patch to be reviewed, every part
must be reviewed.

The number 40 is very much overblown. Lets take a look at the quick
changelog you made, and see how you can partition the patches:


> This is a (probably not complete) list of content changes (no
> stylings):
> public TimeSpan (int hours, int minutes, int seconds): Perf+, better
> called
> method
> public TimeSpan (int days, int hours, int minutes, int seconds):
> Perf+, same
> public TimeSpan (int days, int hours, int minutes, int seconds, int
> milliseconds): Perf+, Own impl., BugFix: thows correct and elaborate
> exception
> private TimeSpan (bool sign, int days, int hours, int minutes, int
> seconds,
> long ticks): BugFix: thows correct and elaborate exception
These changes can probably be in one patch.

> public TimeSpan Add (TimeSpan ts): Minor Bugfix: Throw elaborate
> exception
> public TimeSpan Duration (): Minor Bugfix: Throw elaborate exception
> public TimeSpan Subtract (TimeSpan ts): Minor Bugfix: Throw elaborate
> exception
These are minor, you can probably check them in while you do the
exception message fixups

> 
> public int CompareTo (object value): Bugfix, spelling in exception
> public TimeSpan Negate (): Minor Bugfix: Localize exception
> public static TimeSpan Parse (string s): Bugfix: incorrect exception
> + Several typofixes in error texts
Minor spelling errors dont need to go to the list.

> 
> public static TimeSpan FromDays (double value): Perf+++++, huge speed
> improvement, completely rewritten, Also fixes several bugs: Throws
> ArgumentExceptions for NaN, Fixes bug with wrong overflows with very
> big or
> very small values (did throw exceptions before), Correctly throws
> Overflowexceptions
> public static TimeSpan FromHours (double value): Perf+++++, huge speed
> improvement, completely rewritten, Also fixes several bugs: Throws
> ArgumentExceptions for NaN, Fixes bug with wrong overflows with very
> big or
> very small values (did throw exceptions before), Correctly throws
> Overflowexceptions
> public static TimeSpan FromMinutes (double value): Perf+++++, huge
> speed
> improvement, completely rewritten, Also fixes several bugs: Throws
> ArgumentExceptions for NaN, Fixes bug with wrong overflows with very
> big or
> very small values (did throw exceptions before), Correctly throws
> Overflowexceptions
> public static TimeSpan FromSeconds (double value): Perf+++++, huge
> speed
> improvement, completely rewritten, Also fixes several bugs: Throws
> ArgumentExceptions for NaN, Fixes bug with wrong overflows with very
> big or
> very small values (did throw exceptions before), Correctly throws
> Overflowexceptions
> public static TimeSpan FromMilliseconds (double value): Perf++++, huge
> speed
> improvement, completely rewritten, Also fixes several bugs: Throws
> ArgumentExceptions for NaN, Fixes bug with wrong overflows with very
> big or
> very small values (did throw exceptions before), Correctly throws
> Overflowexceptions
If I understand it, all of these patches do the same thing in different
places, so one patch here.

> public override string ToString (): Perf+++
One patch

> public static bool operator == (TimeSpan t1, TimeSpan t2): Perf+++
> public static bool operator > (TimeSpan t1, TimeSpan t2): Perf+++
> public static bool operator >= (TimeSpan t1, TimeSpan t2): Perf+++
> public static bool operator != (TimeSpan t1, TimeSpan t2): Perf+++
> public static bool operator < (TimeSpan t1, TimeSpan t2): Perf+++
> public static bool operator <= (TimeSpan t1, TimeSpan t2): Perf+++
One patch

> public Parser (string src): Removed stupid (unneeded) method
One patch.

So, this is *5* patches, not 40.


> The problem is simply that i just have a class which I just edit to the need
> that I have for testing. After it is done I usually do not keep any of these
> because they are an entire mess afterwards.
Well, for very obvious ones (like the operator fixes), you dont really
need one (i can tell from looking at it why it is faster).

But for something like ToString, you should really give some benchmarks.

> > >
> > > > Some more specific ones:
> > > >       * Be careful about large arithmetic. For example, you have
> > > >         TicksPerXXX as a long each time. However, if some of these
> were
> > > >         ints, the JIT could make more optimizations. Also, some
> > >
> 
> TicksPerXXX is a PUBLIC constant. I doubt that we should change the type of
> them just to make it potentially optimizable.
Actually, it turns out that MCS emits a ldc.i4/conv.i8 pair, so it is
fine.


> > > > Am betting that a 100% improvement in any one of these issues will net
> > > > far more benefit than a 3000% improvement in TimeSpan.
> > >
> > > This is probably right. I've also done a LOT of test/ checks on String
> > > members. However testing string takes MUCH more time as it
> > > a) is alredy somewhat optimized (also there is a lot of improvement
> > > possible). However in String lots depend upon usage profile (length and
> > > structure of used strings), which makes it so hard to find completely
> > > superior solutions.
> > Agreed. However, taking the easier optimizations reduces the fun (which
> > you are after anyways). However, some of those opts are things that can
> > be done in a way that they will always increase perf. Take a look at the
> > Array.Sort thing I suggested. You just have to make an icall that will
> > handle sorting int[], char[], etc. using glibc's qsort (which is VERY
> > fast).
> 
> Personally I think we should use as little c code as possible. So I will not
> touch and especially not create any c parts. Each icall makes our libraries
> less portable and reusable. Moreover icalls themselves are relatively slow
> and are probably going to become even slower (afaik mono does not do any
> security checks right now with icalls)
> Also I think whatever makes fun to me is probably my thing to find out ;)
> (And I'm hacking for about 15 Years, so I think I already know)

Well, the issue with Array.Sort is that it is making O(n^2) ICalls,
because each time it needs to get/set an element, it must make an icall
(because it only has a System.Array, not a foo []). So, for
int/char/long/etc, we really just need an icall. The overhead of an
ICall in a case like this is negated by the performance benefit.
Anyways, in the array methods, we will be netting fewer, not more,
icalls, because of the Get/Set calls.

> As I said I already worked on String. But as I sent a patch to the list (and
> it was just one perfectly atomic one;) - I did not get any useful response -
> so I simply stopped working on that out of frustration.
There were a few questions about why the long operations were faster, no
response was given to those.

Also, I asked if we would be better off using rep prefixed instructions,
there was no response.

> The thing that I would like to see most is a nice, fast, generational GC.
> Because this limits EVERYTHING right now.
Agreed. But that's a ways off.

Again, I really want this patch to get in. However, if we are not able
to review it, it will never see the light of day. I know you dont like
making changelogs, writing atomic patches etc; I don't either. However,
in the end, it gets patches into cvs faster, meaning mono is better
faster -- which is what we really want.

-- Ben