Refactor String's exception handling

[Ulf Zibis]

Am 30.03.2010 00:17, schrieb Martin Buchholz:
> Hi Ulf,
>
> I will sponsor your initiative to refactor the exception handling.
>
> Before this can go in, we should have just the exception handling
> changes contained in one patch, since it is such a big change.
>    

You mean, that I had "surreptitiously" included some beautification, 
even in the first patch?
Yes, often I can't resist, hit me. Example:
It seems, that someone before had tried to standardize the this-triple 
in String's constructors. Looking closer, you can see, that they 
slightly differ, so for my taste it looked best, ordering them in the 
member variables order, having the real value at first.

On the other hand, I think it's too much overhead, to manage separate 
bugs for such beautifications.
What you think is a reasonable threshold for such on-the-fly 
beautifications?


> I'd like you to try to port my related RangeCheckMicroBenchmark
> to string handling and hopefully demonstrate some measurable
> performance improvement.
>    

That would be great. :-)

> ----
>
> In the code below, I think some if's need to be changed to "else if"s.
> (but don't just fix it - make sure we have a failing test with your
> current code (you do run the regression tests religiously, right?))
>
> +    static void checkPositionIndexes(int srcLen, int begin, int end) {
> +        assert (srcLen>= 0);
> +        int index;
> +        if (begin<  0)
> +            index = begin;
> +        if (end>  srcLen)
> +            index = begin>srcLen ? begin:end-begin;
> +        if (end<  begin)
> +            index = begin>srcLen ? begin : end<0 ? end : end-begin;
> +        else
> +            return;
> +        throw new StringIndexOutOfBoundsException(index);
>    

Good catch. The throws, I had replaced, had implicated the elses before.
In Google code style it would have been:  if (begin < 0 || end > srcLen 
|| end < begin)

You seem to like how I merged the different variations into one central 
standard behaviour. Is that valid for AbstractStringBuilder too?
I think it best matches to current behavior.
Exception message refers to ...
1. <begin>, if begin itself is invalid referring to 0 and srcLen
2. <end>, if end itself is invalid referring to 0 and srcLen
3. <end-begin>, if end is invalid in combination with given begin
Alternative:
2+3. <end>, if end is invalid referring to 0 and srcLen or in 
combination with given begin

The alternative may be easier to track for the developers, but less 
compatible with current behaviour, and a likly negative value speaks 
kinda for itself.
In the checkSubsequence(..., offset, count) case, unfortunately there is 
a good chance to have positive values as result of offset+count.


> ----
> it's =>  its
>
> +     *              following values are referred in it's message:
>    

Yes.

> ----
>
> badIndex might be a better name for "index" below.
>
> +        int index;
> +        if (begin<  0)
> +            index = begin;
>    

Very good idea!

> ----
> Run at least the following tests
> (below is how I test this code myself)
>
> /home/martinrb/jct-tools/3.2.2_03/linux/bin/jtreg -v:nopass,fail
> -vmoption:-enablesystemassertions -automatic "-k:\!ignore"
> -testjdk:/usr/local/google/home/martin/ws/upstream/build/linux-amd64
> test/sun/nio/cs test/java/nio/charset test/java/lang/StringCoding
> test/java/lang/StringBuilder test/java/lang/StringBuffer
> test/java/lang/String test/java/lang/Appendable
>    

Unfortunately I still haven't managed to even partly build a patched JDK 
on my Windows notebook.
- CygWin crashes from too big work, e.g webrev on more than ~20 files.
- Very few support on <nb-projects-dev at openjdk.java.net> mailing list.
- I'm wondering, that there is so few collaboration between NetBeans and 
JDK developers in same software company.

So as workaround, I'm fine with running my patches via -Xbootclasspath 
in NetBeans IDE.
So running jtreg tests I don't know how.
I exclusively had written my test using JUnit, because there is a 
beautiful support from NetBeans.
I remember, there was a email from Mark Reinold some months ago, that 
JUnit tests are too supported by jtreg from now.

Maybe you have some suggestions to me.

> ----
>
> I think returning len below is too confusing.
> Just make the return type void.
>
> +    int checkPositionIndex(int index) {
> +        int len = count; // not sure, if JIT recognizes that it's final ?
> +        checkPositionIndex(len, index);
> +        return len;
> +    }
>    

Returning the len is to prevent from 2 times slowly loading the member 
variable into local register/variable.
 From performance side I think, we only have to choices. Using the 
return trick or dropping those convenient methods at all.
The latter would be faster for the interpreter and/or non inlined case.

> ----
>
> We will need a significant merge once I commit
> related changes.
>    

Maybe we could announce this on this list, so other's could decide, if 
they hurry to commit there changes before, or have to do there own merge 
later.

-Ulf

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20100330/429bf002/attachment.html>