[Mono-devel-list] StringBuilder patch

[Torstensson, Patrik]

Here is the updated patch.

StringBuilder is now using the length property of the string instead of
_capacity. It's also using _cached_str both for caching and instead of a
bool keeping track of the string has been handed out. 

Also updated the marshalling code and object layout. I think when we
commit this patch we need to update the corlib revision int in
Environment, or?

Cheers,
 Patrik

-----Original Message-----
From: mono-devel-list-admin at lists.ximian.com
[mailto:mono-devel-list-admin at lists.ximian.com] On Behalf Of
Torstensson, Patrik
Sent: den 12 december 2003 17:20
To: Paolo Molaro; mono-devel mailing list
Subject: RE: [Mono-devel-list] StringBuilder patch

>Ok, I gave a quick look at the patch (please attach it as a text file
>next time, not as octect-stream, so it's easier to view/reply to).
>It looks good. Please provide also the benchmarking programs, so we can
>compare the numbers too.

I will check in the benchmark programs under mono/benchmark.

Regarding the octect-stream I'm not able to control that, sorry, Outlook
sucks.

>I have a few (random) comments:
>*) why do you use an additional _capacity field? Isn't _str.Length the
>   same value?

Yes, it's the same value. I just keept the implementation with
StringBuilder, avoiding one call to a property. It's easy to change if
you think it worth it (object size over one more call)

>*) the pattern:
>	if (immutable)
>		immutable = false;
>I think can be written simply as: 
>	immutable = false;
>The check and cond branch is going to cost more than the memory store.

Thanks. Going to replace this with cached_str = null; instead.

>*) I would replace the immutable flag with a string, as in the previows
>implementation: this gives no object size changes on 32 bit systems
>anyway and it can be used to avoid the worst case in your code
>(ToString() with a less than 50% full _str will always and repeatedly
>allocate new strings all equal: just store the result of Substring in
>the new string field and return that. The string is simply set to null
>on any changes, and if it's set, it's returned in ToString(), like in
>the previous implementation). Any reasons not to follow the previous
>pattern?

No reasons, the 50% rule was added after discussions on IRC and was not
in the original design from the begining. I also did think of adding the
caching functionality later but you make a good point, i can remove the
boolean and replace it with a string object. Thanks.

>At the end of next week we have a deadline, so, once you have a revised
>patch, please wait for me to test it on ppc, since we don't want to
>make changes in the core that could impact it. Thanks.

Sure, just seen a lot of patches floating around on the list, I want the
code to be tested before commiting. I will make the changes tonight and
repost to the list. 

Cheers,
 Patrik

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better
_______________________________________________
Mono-devel-list mailing list
Mono-devel-list at lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list

_______________________________________________
Mono-devel-list mailing list
Mono-devel-list at lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: metadata.diff.txt
Url: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20031212/27563979/attachment.txt 
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: corlib.diff.txt
Url: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20031212/27563979/attachment-0001.txt