[Mono-devel-list] StringBuilder patch

Torstensson, Patrik patrik.torstensson at intel.com
Fri Dec 12 12:20:15 EST 2003


>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




More information about the Mono-devel-list mailing list