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

[Andreas Nahr]

Hi,

comments in text

> Hello,
>
> Ok, a few general comments:
>       * You have no changelog. As such, I have a very hard time guessing
>         what you tried to do. Please include a detailed changelog
>         stating the improvements in each class.

Sorry, forgot that one. However it isn't much more detailed that the mailing
was. As I wrote this was basically a reimplementation. I started from
scratch and copied things over that I felt were ok (weren't really a lot)

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
public TimeSpan Add (TimeSpan ts): Minor Bugfix: Throw elaborate exception
public int CompareTo (object value): Bugfix, spelling in exception
public TimeSpan Duration (): Minor Bugfix: Throw elaborate exception
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
public TimeSpan Negate (): Minor Bugfix: Localize exception
public static TimeSpan Parse (string s): Bugfix: incorrect exception
public TimeSpan Subtract (TimeSpan ts): Minor Bugfix: Throw elaborate
exception
public override string ToString (): 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+++
public static bool operator <= (TimeSpan t1, TimeSpan t2): Perf+++
public Parser (string src): Removed stupid (unneeded) method
+ Several typofixes in error texts
This are the changes that I can remember right now if looking over both
classes, probably more
And that is just the result. Obviously there are much more changes in the
methods to archive this.

>       * 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.

>               * Attach *ONE* patch per bug. If a bug appears in multiple
>                 areas, it is OK to attach one patch that fixes the issue
>                 in each method. In general, there should be one patch
>                 per bug that would be filed in bugzilla

The bugs are currently not in bugzilla (at least I didn't check if any of
them are there). I just encountered them (mostly) while doing the perfomance
test. And obviously while testing my implementation. And I really just do
this all for fun in my spare time and starting to file bugs for things I
immediately fix is not really fun for me.

>               * Attach one patch for each optimization.
>       * ALL performance improvements should come with benchmarks.

There are A LOT of them. Would anybody have much for insight if I posted
40-50 individual patches for that?
I've done a lot of microbenchmarking (which fortunatelly is relatively
simple when dealing mostly with longs). However I do not have any of those
available for inclusion as I just use one app and modify as needed.

> 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

These need to be longs because the values can reach long values

>         operations could be moved out of checked contexts to make them
>         faster.

I choose all of the checked contexts very carefully. If you can tell me any
context where the check is not needed please tell me. But if I made no
mistakes the checks are crucial at  the positions where they are now. (These
places could overflow, but must never do)

>       * When you use stringbuilder, try to guess at how long the string
>         will be -- this avoids reallocs.

Yes - played around with that one for a bit of time, but in the end the
default value of 16 elements seemed just right (This is one of the cases (in
this struct) were usage profile would matter)

> I would really encourage you to consider where you place performance
> improvements. Frankly, not that many apps use TimeSpan. I have never
> seen it be a bottleneck in those that do. Here are a few ideas -- maybe
> they can get you started:
>       * Try any method in System.Array on a large array of ints (like
>         Array.Sort (my_int_array). Try it under the profiler.
>       * Buffer.BlockCopy is slower than Array.Copy. It would be
>         interesting to take a look at that.
>       * Can we implement String.Concat in C#? There is a thread from a
>         while back about this. This method would be interesting also as
>         a case study for jit optimizations.
>       * Many classes have static ctors that do not need them. Also, some
>         classes use the static CLASS () {} construct which prevents
>         beforefieldinit. It would be nice to do an audit for this. An
>         example of the problem is IntPtr, which has a static ctor to
>         init a static field to 0 (this is not needed, as it is inited to
>         zero by default).
>       * Many components of reflection could use some love.
>       * There are many places where we could use stackalloc to avoid the
>         cost of hitting the GC. Example -- many of the .ToString methods
>         -- they use very small buffers. Good case for stackalloc.
>
> 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.



Also IMHO we should not make too much assumptions about when and how a
application uses the core classes. There MAY be applications out there that
depend on TimeSpan in a very perfomance sensitive way and it is one of the
CORE classes. So an area where we are 100ds of times slower than MS impl.
would even seem like a showstopper to me. And it's also just nice that some
are now faster than MS ;)