[Mono-devel-list] [PATCH] String speedup

[Paolo Molaro]

On 02/22/04 Ben Maurer wrote:
>       * String.Equals -- Two things here. First, we use pointers rather
>         than the string indexer. This allows us to avoid Array Bounds
>         Checks. The second is that when possible we compare things one
>         int at a time. This basically gives a doubling of perf from the
>         above change. Thanks to Miguel for pointing me out to this
>       * StringBuilder.Append (char []) -- Rather than setting char by
>         char, I added an icall so we can use memcpy
> 
> Now, for what you have all been waiting for, numbers:
> 
> Test -------+ Before ---+ After ---+ Improvement factor +
> Equals      | 6.797s    | 1.933s   | 3.516              |

Ben, please take the time to write better benchmarks: this number is
meaningless. Benchmark numbers are nice, but they are useful to us only
when they are relevant. Now, ask yourself how often 50-character strings
are compared and how often 10 or so character strings are compared
instead. So, if you really want to just present one single number, do
it for a string length that is relevant, but the best option is to
actually know what optimization you're doing and try to explore both its
strengths and weaknesses. You'd have more credit if you also showed
that the worst case for your change wasn't slower than the current code.
This is important, because right now we don't have any overhead when
pinning, since we use a non-moving GC: later it's quite possible that
for small string lengths (and when bound check removal will be
implemented), the existing code will perform better.
FWIW, for strings that range from 1 char to about 100, on my machine the
speedup from your change is between 0 and 2.1 (with a value of about 1.7
for common string lengths). So the Equals change is fine to commit.

> @@ -1213,11 +1232,7 @@
>  
>  		internal unsafe void InternalSetChar(int idx, char val) 
>  		{
> -			if ((uint) idx >= (uint) Length)
> -				throw new ArgumentOutOfRangeException("idx");

This change should obviously not be committed.

> @@ -1307,6 +1322,9 @@
>  		[MethodImplAttribute(MethodImplOptions.InternalCall)]
>  		internal extern static void InternalStrcpy(String dest, int destPos, String src);
>  
> +		[MethodImplAttribute(MethodImplOptions.InternalCall)]
> +		internal extern static void InternalStrcpy(String dest, int destPos, char [] src, int startPos, int count);

Your patch suffers from overpatchitis: trying to cram unrelated changes
in the same diff.
As was discussed in the list maybe a year or so ago, no new icalls
should be added to string unless it is shown that an implementation in
managed code is significantly slower. Of course all the existing ones
should be reviewed and hopefully dropped.
So this InternalStrcpy change should not be committed: please implement
it in managed code and report with some meaningful numbers how it
performs that way.

Thanks.
lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better