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

[Andreas Nahr]

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

Testing was done using microbenchmarks. I'll post some of the results
(probably valid to +- 10 ms):
FromDays (not raising exception): 22583ms -> 3645ms
FromMinutes (not raising exception): 13289ms -> 3656ms
(and these functions include additional checks that were missing before)
Equals (value): 3826 -> 3785
== operator: 2183 -> 1512
> operator: 2173 -> 1713

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

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

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

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.

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

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

Makes perfect sense. Will get even faster ;)

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

OK

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

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

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.

The thing that I would like to see most is a nice, fast, generational GC.
Because this limits EVERYTHING right now.

Andreas