[I trimmed the distribution list] On Jun 3 2012, at 13:44 , Peter Levart wrote:

On Thursday, May 31, 2012 03:22:35 AM mike.duigou@oracle.com wrote:

...

Changeset: 2c773daa825d Author: mduigou Date: 2012-05-17 10:06 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2c773daa825d

6924259: Remove offset and count fields from java.lang.String Summary: Removes the use of shared character array buffers by String along with the two fields needed to support the use of shared buffers.

Wow, that's quite a change.

Indeed. It was a long time in development. It is a change which is expected to be overall beneficial though and in the general case a positive win.

So .substring() is not O(1) any more?

No. Though with object allocation it probably was only ever roughly O(1) anyway.

Doesn't this have impact on the performance of parsers and such that rely on the performance caracteristics of the .substring() ?

It does have an impact. We've seen as much as a couple of percent on some benchmarks. Parsers which use substring for extraction are definitely impacted by this change.

Have you considered then implementing .subSequence() not in terms of just delegating to .substring() but returning a special CharSequence view over the chars of the sub-sequence?

It does look that String.subSequence() returning a special view rather than a substring would be a good optimization and probably a very good compromise for parser developers. Please create an issue and if you have the time and expertise a patch would speed things along (though unfortunately almost certainly too late for inclusion in 7u6). Mike

Regards, Peter

...

Reviewed-by: alanb, mduigou, forax, briangoetz Contributed-by: brian.doherty@oracle.com

! src/share/classes/java/lang/Integer.java ! src/share/classes/java/lang/Long.java ! src/share/classes/java/lang/String.java ! src/share/classes/java/lang/StringCoding.java

For the record, javadoc uses substring very heavily, and might be impacted by this change. -- Jon On 06/03/2012 02:35 PM, Mike Duigou wrote:

[I trimmed the distribution list]

On Jun 3 2012, at 13:44 , Peter Levart wrote:

...

On Thursday, May 31, 2012 03:22:35 AM mike.duigou@oracle.com wrote:

...

Changeset: 2c773daa825d Author: mduigou Date: 2012-05-17 10:06 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2c773daa825d

6924259: Remove offset and count fields from java.lang.String Summary: Removes the use of shared character array buffers by String along with the two fields needed to support the use of shared buffers. Wow, that's quite a change. Indeed. It was a long time in development. It is a change which is expected to be overall beneficial though and in the general case a positive win.

...

So .substring() is not O(1) any more? No. Though with object allocation it probably was only ever roughly O(1) anyway.

...

Doesn't this have impact on the performance of parsers and such that rely on the performance caracteristics of the .substring() ? It does have an impact. We've seen as much as a couple of percent on some benchmarks. Parsers which use substring for extraction are definitely impacted by this change.

...

Have you considered then implementing .subSequence() not in terms of just delegating to .substring() but returning a special CharSequence view over the chars of the sub-sequence? It does look that String.subSequence() returning a special view rather than a substring would be a good optimization and probably a very good compromise for parser developers. Please create an issue and if you have the time and expertise a patch would speed things along (though unfortunately almost certainly too late for inclusion in 7u6).

Mike

...

Regards, Peter

...

Reviewed-by: alanb, mduigou, forax, briangoetz Contributed-by: brian.doherty@oracle.com

! src/share/classes/java/lang/Integer.java ! src/share/classes/java/lang/Long.java ! src/share/classes/java/lang/String.java ! src/share/classes/java/lang/StringCoding.java

Hi Mike, Yes, all that you say below is true. CharSequence is an interface that does not define the contract of identity when implementations/subtypes of CharSequence do - each in it's own way. Much like java.util.Collection and List vs. Set. It's always dangerous for methods that return such interfaces when the implementation class of the returned instance changes so that it defines a different "hidden" contract. It is unfortunately also true that now there is no "official" way of comparing/equating/hashing a substring without copying the characters. It feels like standard Java SE API is lacking a part of functionality. Maybe this space could be filled with the addition of a new public immutable CharSequence subtype - like Rope: http://ahmadsoft.org/ropes/doc/index.html Regards, Peter On Friday, June 22, 2012 03:15:40 PM Mike Duigou wrote:

I've made a test implementation of subSequence() utilizing an inner class with offset and count fields to try to understand all the parts that would be impacted. My observations thus far:

- The specification of the subSequence() method is currently too specific. It says that the result is a subString(). This would no longer be true. Hopefully nobody assumed that this meant they could cast the result to String. I know, why would you if you can just call subString() instead? I've learned to assume that somebody somewhere does always does the most unexpected thing. - The CharSequences returned by subSequence would follow only the general CharSequence rules for equals()/hashCode(). Any current usages of the result of subSequence for equals() or hashing, even though it's not advised, would break. We could add equals() and hashCode() implementations to the CharSequence returned but they would probably be expensive. - In general I wonder if parsers will be satisfied with a CharSequence that only implements identity equals(). - I also worry about applications that currently do use subSequence currently and which will fail when the result is not a String instance as String.equals() will return false for all CharSequences that aren't Strings. ie. CharSequence token = line.subSequence(line, start, end); if (keyword.equals(token)) ... This would now fail.

At this point I wonder if this is a feature worth pursuing.

Mike

On Jun 3 2012, at 13:44 , Peter Levart wrote:

...

On Thursday, May 31, 2012 03:22:35 AM mike.duigou@oracle.com wrote:

...

Changeset: 2c773daa825d Author: mduigou Date: 2012-05-17 10:06 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2c773daa825d

6924259: Remove offset and count fields from java.lang.String Summary: Removes the use of shared character array buffers by String along with the two fields needed to support the use of shared buffers.

Wow, that's quite a change.

So .substring() is not O(1) any more?

Doesn't this have impact on the performance of parsers and such that rely on the performance caracteristics of the .substring() ?

Have you considered then implementing .subSequence() not in terms of just delegating to .substring() but returning a special CharSequence view over the chars of the sub-sequence?

As usual, an excellent idea Jason. I'll probably run an internal test/benchmark with both this and the CharSequence inner class implementation to see what breaks and where there are performance differences between the two. I was planning to also test a version of the CharSequence implementation which implemented equals() and hashCode() compatible with String and see if this produced fewer (or different) test failures. Good idea. Mike On Jun 24 2012, at 12:06 , Jason Mehrens wrote:

Mike,

Why not implement subSequence as 'java.nio.CharBuffer.wrap(data, beginIndex, endIndex).asReadOnlyBuffer()' ? Easy to implement and test. The nice thing is that parsers would know what a 'CharBuffer' vs. a sub sequence String internal class.

Jason

...

Subject: String.subSequence and CR#6924259: Remove offset and count fields from java.lang.String From: mike.duigou@oracle.com Date: Fri, 22 Jun 2012 15:15:40 -0700 To: peter.levart@gmail.com CC: core-libs-dev@openjdk.java.net

I've made a test implementation of subSequence() utilizing an inner class with offset and count fields to try to understand all the parts that would be impacted. My observations thus far:

- The specification of the subSequence() method is currently too specific. It says that the result is a subString(). This would no longer be true. Hopefully nobody assumed that this meant they could cast the result to String. I know, why would you if you can just call subString() instead? I've learned to assume that somebody somewhere does always does the most unexpected thing. - The CharSequences returned by subSequence would follow only the general CharSequence rules for equals()/hashCode(). Any current usages of the result of subSequence for equals() or hashing, even though it's not advised, would break. We could add equals() and hashCode() implementations to the CharSequence returned but they would probably be expensive. - In general I wonder if parsers will be satisfied with a CharSequence that only implements identity equals(). - I also worry about applications that currently do use subSequence currently and which will fail when the result is not a String instance as String.equals() will return false for all CharSequences that aren't Strings. ie. CharSequence token = line.subSequence(line, start, end); if (keyword.equals(token)) ... This would now fail.

At this point I wonder if this is a feature worth pursuing.

Mike

On Jun 3 2012, at 13:44 , Peter Levart wrote:

...

On Thursday, May 31, 2012 03:22:35 AM mike.duigou@oracle.com wrote:

...

Changeset: 2c773daa825d Author: mduigou Date: 2012-05-17 10:06 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2c773daa825d

6924259: Remove offset and count fields from java.lang.String Summary: Removes the use of shared character array buffers by String along with the two fields needed to support the use of shared buffers.

Wow, that's quite a change.

So .substring() is not O(1) any more?

Doesn't this have impact on the performance of parsers and such that rely on the performance caracteristics of the .substring() ?

Have you considered then implementing .subSequence() not in terms of just delegating to .substring() but returning a special CharSequence view over the chars of the sub-sequence?

If String.substring(int, int) now performs a copy of the underlying char[] array and if there is no String.subSequence(int, int) providing the old functionality, maybe the following implications should be investigated? StringBuilder.append(...) -------------------- Since, in order to avoid a useless array copy, the users may be advised to replace the following pattern: StringBuilder.append(string.substring(lower, upper)); by: StringBuilder.append(string, lower, upper); would it be worth to add a special-case in the AbstractStringBuilder.append(CharSequence, int, int) implementation for the String case in order to reach the efficiency of the AbstractStringBuilder.append(String) method? The later copies the data with a single call to System.arraycopy, as opposed to the former which invoke CharSequence.charAt(int) in a loop. Integer.parseInt(...) ---------------- There was a thread one years ago about allowing Integer.parseInt(String) to accept a CharSequence. http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-April/thread.html#... One invoked reason was performance, since the cost of calling CharSequence.toString() has been measured with the NetBeans profiler as significant (assuming that the CharSequence is not already a String) when reading large ASCII files. Now if the new String.substring(...) implementation copies the internal array, we may expect a performance cost similar to StringBuilder.toString(). Would it be worth to revisit the Integer.parseInt(String) case - and similar methods in other wrapper classes - for allowing CharSequence input? Martin Le 23/06/12 00:15, Mike Duigou a écrit :

I've made a test implementation of subSequence() utilizing an inner class with offset and count fields to try to understand all the parts that would be impacted. My observations thus far:

- The specification of the subSequence() method is currently too specific. It says that the result is a subString(). This would no longer be true. Hopefully nobody assumed that this meant they could cast the result to String. I know, why would you if you can just call subString() instead? I've learned to assume that somebody somewhere does always does the most unexpected thing. - The CharSequences returned by subSequence would follow only the general CharSequence rules for equals()/hashCode(). Any current usages of the result of subSequence for equals() or hashing, even though it's not advised, would break. We could add equals() and hashCode() implementations to the CharSequence returned but they would probably be expensive. - In general I wonder if parsers will be satisfied with a CharSequence that only implements identity equals(). - I also worry about applications that currently do use subSequence currently and which will fail when the result is not a String instance as String.equals() will return false for all CharSequences that aren't Strings. ie. CharSequence token =ine.subSequence(line, start, end); if (keyword.equals(token)) ... This would now fail.

At this point I wonder if this is a feature worth pursuing.

Mike

Le 26/06/12 20:10, Mike Duigou a écrit :

...

StringBuilder.append(string.substring(lower, upper)); by: StringBuilder.append(string, lower, upper);

This would seem to be a good refactoring regardless of the substring implementation as it avoids creation of a temporary object.

The rational was that the performance advantage of using System.arraycopy(...) instead than a loop over CharSequence.charAt(int) may compensate the cost of creating a temporary object. I would not be surprised if the former was faster than the later for large substrings despite the temporary object creation. However it may not be true anymore now that substring(...) performs a copy. Of course this would need to be verified with benchmark... Martin