Request for Review : CR#6924259: Remove String.count/String.offset

[Alan Bateman]

On 24/05/2012 21:45, Mike Duigou wrote:
> Hello all;
>
> For a long time preparations and planing have been underway to remove the offset and count fields from java.lang.String. These two fields enable multiple String instances to share the same backing character buffer. Shared character buffers were an important optimization for old benchmarks but with current real world code and benchmarks it's actually better to not share backing buffers. Shared char array backing buffers only "win" with very heavy use of String.substring. The negatively impacted situations can include parsers and compilers however current testing shows that overall this change is beneficial.
>
> The current plan is to commit this change to jdk8 before build 40 followed by jdk7u6. Removing offset/count is a complicated process because HotSpot has special case code for String that depends upon the two fields. Vladimir Kozlov has committed the required HotSpot changes already (http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/8f972594effc).
>
> A patch of the changes to java.lang.String for JDK 8 are at<http://cr.openjdk.java.net/~mduigou/6924259/0/webrev/>. The changes for JDK 7 have only superficial differences (line offsets in the patch).
>
> Comments are welcome.
>
> Mike
I went through the latest webrev and don't see anything obviously wrong. 
EMPTY_STRING_VALUE is new, at least to me, as I don't think it was there 
when removing these fields was prototyped a few years ago.

A minor point in the String(char[]) constructor is that you could move 
the "int len = .." to the top of the method to be consistent with other 
places where a local holds the value.length.

The "use name version for caching" comment in StringCoding.java might be 
confusing to readers, maybe "use charset name as this is faster due to 
caching".

A non-material comment is that there are a couple of style changes that 
I found annoying. Everyone is an expert on such matters, I'm not, but 
the one that bugged me a bit was adding the space after the cast, eg:
(toffset > (long) value.length - len)
I found myself needing to re-read it to see if the cast applied to 
value.length or value.length-len. It's a minor point but you know what I 
mean.

-Alan.