Re: [Mono-list] RE: [Mono-devel-list] REGRESSION: StringBuilder

2004-01-13 Thread Paolo Molaro
On 01/14/04 Iain McCoy wrote:
> I get the same problem, on a freshly checked-out-from-anoncvs-and-built
> mcs and mono. I did some digging (just because I could) and I suspect
> the problem is that StringBuilder uses String.InternalStrcpy to move
> everything to the right of the insert point across. From my reading of
> the code, that would go to the method
> ves_icall_System_String_InternalStrcpy_StrN in
> mono/metadata/string-icalls.c. That function's basically a call to
> memcpy, which can't be used on overlapping memory areas (and breaking
> that rule is what causes the regression). I think there's a few options
> to fix it:
> 1 Add a String.InternalStrmov that uses memmove instead of memcpy
> 2 Change String.InternalStrcpy so that it uses memmove
> 
> Both of these options fix the problem. I suspect Option 2 is detrimental
> to performance because it would mean that overlapping buffers were being
> dealt with even when they weren't actually there.

I committed a change memcpy -> g_memmove, mostly because it's the
smaller change.

> I'm attaching a little patch I wrote that does Option 1. I make no
> claims about this patch, except that it works for me. I didn't look over
> StringBuilder particularly thoroughly, but I think I caught all of the
> cases that needed to be changed. 
> 
> I imagine I have broken 5 rules and 10 guidelines with this patch, but
> oh well - I hope it is useful.

The patch looks good:-)
Thanks for tracking down the issue.
If we get some real data that shows the memmove to be significantly
slower than the memcpy, we'll go for the added complexity of having the
additional icalls.

Thanks!
lupus

-- 
-
[EMAIL PROTECTED] debian/rules
[EMAIL PROTECTED] Monkeys do it better
___
Mono-list maillist  -  [EMAIL PROTECTED]
http://lists.ximian.com/mailman/listinfo/mono-list


Re: [Mono-list] RE: [Mono-devel-list] REGRESSION: StringBuilder

2004-01-13 Thread Iain McCoy
I get the same problem, on a freshly checked-out-from-anoncvs-and-built
mcs and mono. I did some digging (just because I could) and I suspect
the problem is that StringBuilder uses String.InternalStrcpy to move
everything to the right of the insert point across. From my reading of
the code, that would go to the method
ves_icall_System_String_InternalStrcpy_StrN in
mono/metadata/string-icalls.c. That function's basically a call to
memcpy, which can't be used on overlapping memory areas (and breaking
that rule is what causes the regression). I think there's a few options
to fix it:
1 Add a String.InternalStrmov that uses memmove instead of memcpy
2 Change String.InternalStrcpy so that it uses memmove

Both of these options fix the problem. I suspect Option 2 is detrimental
to performance because it would mean that overlapping buffers were being
dealt with even when they weren't actually there.

I'm attaching a little patch I wrote that does Option 1. I make no
claims about this patch, except that it works for me. I didn't look over
StringBuilder particularly thoroughly, but I think I caught all of the
cases that needed to be changed. 

I imagine I have broken 5 rules and 10 guidelines with this patch, but
oh well - I hope it is useful.

Iain


On Tue, 2004-01-13 at 23:55, Jaroslaw Kowalski wrote:
> I've just rebuilt mcs and mono and I still get the same error.
>  
> BTW. I'm using:
>  
> :pserver:[EMAIL PROTECTED]:/mono
>  
> Jarek
> - Original Message - 
> From: Torstensson, Patrik
> To: Torstensson, Patrik ; Jaroslaw Kowalski ;
> [EMAIL PROTECTED] ; Mono Development
> Sent: Tuesday, January 13, 2004 1:12 PM
> Subject: [Mono-list] RE: [Mono-devel-list] REGRESSION:
> StringBuilder
> 
> Have you rebuilt everything? I just did the same ression test
> here and that returns correct value.
>  
> Can anyone else test this also with the latest CVS?
>  
> Cheers,
>  Patrik
> 
> __
> From: [EMAIL PROTECTED]
> [mailto:[EMAIL PROTECTED] On Behalf Of
> Torstensson, Patrik
> Sent: den 13 januari 2004 13:02
> To: Jaroslaw Kowalski; [EMAIL PROTECTED]; Mono
> Development
> Subject: RE: [Mono-devel-list] REGRESSION: StringBuilder
> 
> 
> Looking into it right now, it's strange because SB passed all
> the test.
>  
> Be back in 2..
>  
> Sorry for the problems caused!
>  
> Cheers,
>  Patrik
> 
> __
> From: [EMAIL PROTECTED]
> [mailto:[EMAIL PROTECTED] On Behalf Of
> Jaroslaw Kowalski
> Sent: den 13 januari 2004 12:33
> To: [EMAIL PROTECTED]; Mono Development
> Subject: [Mono-devel-list] REGRESSION: StringBuilder
> Importance: Low
> 
> 
> Looks like there's been a regression introduced in mono
> yesterday.
>  
> This snippet:
>  
> StringBuilder sb = new StringBuilder();
>  
> sb.Append("testtesttest");
> sb.Insert(0, '^');
> Console.WriteLine("sb: {0}", sb);
> 
> produces:
>  
> sb: ^teetteetteet
>  
> It obviously should be:
> 
> sb: ^testtesttest
>  
> This is kind of critical since NAnt no longer runs on
> mono/Linux.
>  
> Jarek
-- 
Iain McCoy <[EMAIL PROTECTED]>
? mono/autom4te.cache
? mono/mint.pc
? mono/mono/interpreter/interp.lo
? mono/mono/interpreter/libmint.la
? mono/scripts/soapsuds
? mcs/class/Mono.Posix/Mono.Posix/Mono.Posix.dll
? mcs/class/Mono.Posix/Mono.Posix/map.c
? mcs/class/Mono.Posix/Mono.Posix/map.h
Index: mono/mono/metadata/icall.c
===
RCS file: /mono/mono/mono/metadata/icall.c,v
retrieving revision 1.395
diff -u -r1.395 icall.c
--- mono/mono/metadata/icall.c  12 Jan 2004 07:42:17 -  1.395
+++ mono/mono/metadata/icall.c  13 Jan 2004 16:19:43 -
@@ -4452,6 +4452,8 @@
"System.String::InternalAllocateStr", 
ves_icall_System_String_InternalAllocateStr,
"System.String::InternalStrcpy(string,int,string)", 
ves_icall_System_String_InternalStrcpy_Str,
"System.String::InternalStrcpy(string,int,string,int,int)", 
ves_icall_System_String_InternalStrcpy_StrN,
+   "System.String::InternalStrmove(string,int,string)", 
ves_icall_System_String_InternalStrmove_Str,
+   "System.String::InternalStrmove(string,int,string,int,int)", 
ves_icall_System_String_InternalStrmove_StrN,
"System.String::InternalIntern", ves_icall_System_String_InternalIntern,
"System.String::InternalIsInterned

Re: [Mono-list] RE: [Mono-devel-list] REGRESSION: StringBuilder

2004-01-13 Thread Jaroslaw Kowalski



I've just rebuilt mcs and mono and I still get the same 
error.
 
BTW. I'm using:
 
:pserver:[EMAIL PROTECTED]:/mono
 
Jarek

  - Original Message - 
  From: 
  Torstensson, Patrik 
  To: Torstensson, Patrik ; Jaroslaw Kowalski ; [EMAIL PROTECTED] ; Mono Development 
  Sent: Tuesday, January 13, 2004 1:12 
  PM
  Subject: [Mono-list] RE: 
  [Mono-devel-list] REGRESSION: StringBuilder
  
  Have you rebuilt everything? I just did the same ression 
  test here and that returns correct value.
   
  Can anyone else test this also with the latest 
  CVS?
   
  Cheers,
   Patrik
  
  
  From: [EMAIL PROTECTED] 
  [mailto:[EMAIL PROTECTED] On Behalf Of 
  Torstensson, PatrikSent: den 13 januari 2004 
  13:02To: Jaroslaw Kowalski; [EMAIL PROTECTED]; Mono 
  DevelopmentSubject: RE: [Mono-devel-list] REGRESSION: 
  StringBuilder
  
  Looking into it right now, it's strange because SB passed 
  all the test.
   
  Be back in 2..
   
  Sorry for the problems caused!
   
  Cheers,
   Patrik
  
  
  From: [EMAIL PROTECTED] 
  [mailto:[EMAIL PROTECTED] On Behalf Of Jaroslaw 
  KowalskiSent: den 13 januari 2004 12:33To: 
  [EMAIL PROTECTED]; Mono DevelopmentSubject: 
  [Mono-devel-list] REGRESSION: StringBuilderImportance: 
  Low
  
  Looks like there's been a regression introduced 
  in mono yesterday.
   
  This snippet:
   
      
  StringBuilder sb = new StringBuilder();
   
      
  sb.Append("testtesttest");    
  sb.Insert(0, '^');    
  Console.WriteLine("sb: {0}", sb);
  produces:
   
      sb: 
  ^teetteetteet
   
  It obviously should be:
      
          sb: 
  ^testtesttest
   
  This is kind of critical since NAnt no longer 
  runs on mono/Linux.
   
  Jarek