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

[Ben Maurer]

On Sat, 2004-04-17 at 04:43, Andreas Nahr wrote:

> 
> This is a (probably not complete) list of content changes (no stylings):
> public TimeSpan (int hours, int minutes, int seconds): Perf+, better called
> method
Ok, I did not ask for a one to five scale of `how much did I improve
performance'. Tell me HOW you did it. Specific numbers (ie, a benchmark)
should also be attached.

> >       * 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:
public void Foo () {
- ...
+ ...
}

So that we can see the old and the new code for each method. This allows
us to better understand what you are doing.   

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

Note "Would be filed in bugzilla" -- There is no need to file tons of
bugs in bugzilla (though, it would not be a bad idea, and would allow us
to track each patch). What I am trying to say is:
        You are fixing different bugs. Attach different a patch for each
        issue. An issue is the smallest diff that you can create that
        fixes a specific behavior. An exception is if the exact same
        behavior occurs in `n' places, in which case, you should attach
        a patch that fixes the issue in each method.

Also, note that doing atomic patches is to your own benefit. It results
in quicker reviews, fewer rants on overpatchitis, and gets your code
into CVS faster.

> 
> >               * 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 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'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.
I have no issue if you attach a suite of benchmarks. If the suite is
above the mono-devel-list limit, feel free to zip them.

> 
> > 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
Yes, but the jit can optimize:
int a, b;
long c = (long) a * (long) b;

into one mul instruction. It cant do that with:
long a, b;
long c = a * b;

> 
> >         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)

> +		public TimeSpan (int days, int hours, int minutes, int seconds, int milliseconds)
>  		{
> +			try {
> +				checked {
> +					_ticks = TicksPerDay * days + 
> +						TicksPerHour * hours +
> +						TicksPerMinute * minutes +
> +						TicksPerSecond * seconds +
> +						TicksPerMillisecond * milliseconds;
> +				}
> +			}
> +			catch {
> +				throw new ArgumentOutOfRangeException (Locale.GetText ("The timespan is too big or too small."));
> +			}
>  		}

In all the cases where TicksPerXXX is smaller than 2^32, the multiplies will not overflow, so you dont need checked.


> 
> >       * 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)
A comment to that effect would help future optimizers.


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


> 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 ;)

Absolutely we should not. However, opts on Array and String should come
before more uncommon classes.


In short, I really want your patch to get in. However, there is no way
it can possibly be reviewed as is. It is in your best interest to submit
small, atomic patches that can quickly land on CVS.

-- Ben