RFR [8054221] StringJoiner imlementation optimization
Hello! Here's the proposal by Martin Buchholz to improve implementation of StringJoiner class: http://cr.openjdk.java.net/~martin/webrevs/openjdk9/StringJoiner-optimizatio... I think that result of concatenation in the merge() method can be saved to be reused later. This would be useful, for example, if a StringJoiner is merged into other StringJoiners several times. So, here's my slightly modified version of the Martin's webrev: http://cr.openjdk.java.net/~igerasim/8054221/0/webrev/ I've also modified the test to regularly test cases with empty prefixes/suffixes. Sincerely yours, Ivan
Hi, - is there a sufficiently good reason not to use an ArrayList<String> over a manually resized String[] here? It'd simplify and use overflow-conscious code at a negligible performance cost - nit: elt(s) -> element(s) - nit: always use curly braces /Claes On 08/04/2014 06:16 PM, Ivan Gerasimov wrote:
Hello!
Here's the proposal by Martin Buchholz to improve implementation of StringJoiner class: http://cr.openjdk.java.net/~martin/webrevs/openjdk9/StringJoiner-optimizatio...
I think that result of concatenation in the merge() method can be saved to be reused later. This would be useful, for example, if a StringJoiner is merged into other StringJoiners several times.
So, here's my slightly modified version of the Martin's webrev: http://cr.openjdk.java.net/~igerasim/8054221/0/webrev/
I've also modified the test to regularly test cases with empty prefixes/suffixes.
Sincerely yours, Ivan
On Mon, Aug 4, 2014 at 10:04 AM, Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
- is there a sufficiently good reason not to use an ArrayList<String> over a manually resized String[] here? It'd simplify and use overflow-conscious code at a negligible performance cost
This is a core library - it should be fanatically devoted to high performance. Should HashMap have an internal ArrayList instead of an array?
- nit: elt(s) -> element(s) - nit: always use curly braces
Nah.
Thanks, Ivan. On Mon, Aug 4, 2014 at 9:16 AM, Ivan Gerasimov <ivan.gerasimov@oracle.com> wrote:
Hello!
Here's the proposal by Martin Buchholz to improve implementation of StringJoiner class: http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ StringJoiner-optimization/
I think that result of concatenation in the merge() method can be saved to be reused later. This would be useful, for example, if a StringJoiner is merged into other StringJoiners several times.
Looks good - saving the result of compaction is a good idea. --- I'm truly unsure whether it's worth optimizing for size < 2. But if we do that, I think we should optimize size == 0 as well, thus: if (addLen == 0 && size <= 1) return (size == 0) ? "" : elts[0]; --- 240 if (other == this) { I doubt that optimizing specially for a self-merge is worthwhile. When you're thinking of doing that, you probably really want Strings.repeat(String, int copies) https://code.google.com/p/guava-libraries/source/browse/guava/src/com/google... I suggest adding repeat to String! So, here's my slightly modified version of the Martin's webrev:
http://cr.openjdk.java.net/~igerasim/8054221/0/webrev/
I've also modified the test to regularly test cases with empty prefixes/suffixes.
Sincerely yours, Ivan
Thanks you Martin! On 05.08.2014 9:52, Martin Buchholz wrote:
I'm truly unsure whether it's worth optimizing for size < 2.
With saving results of compaction in merge, this case will become a little more common. This way X.merge(Y); Y.toString(); can work a bit faster.
But if we do that, I think we should optimize size == 0 as well, thus:
if (addLen == 0 && size <= 1) return (size == 0) ? "" : elts[0]; Yes, done.
--- 240 if (other == this) {
I doubt that optimizing specially for a self-merge is worthwhile.
You're probably correct, it's not worth coding that. I just was upset that combining with itself doesn't work as well as in other cases, that's why I implemented it. I'll remove it. Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8054221/1/webrev/
When you're thinking of doing that, you probably really want Strings.repeat(String, int copies) https://code.google.com/p/guava-libraries/source/browse/guava/src/com/google...
I suggest adding repeat to String!
Yes, it would be a useful addition. In addition, there might be overloads with a delimiter, prefix and suffix :-) Sincerely yours, Ivan
Hi, Don't get too carried away on adding convience methods, last year when StringJoiner was proposed, there was quite a bit of tension between enough and too much. Don't add to the size and maintenance of the JDK without a clear use case and knowledge that the APIs will be used frequently. Roger On 8/5/2014 7:55 AM, Ivan Gerasimov wrote:
Thanks you Martin!
On 05.08.2014 9:52, Martin Buchholz wrote:
I'm truly unsure whether it's worth optimizing for size < 2.
With saving results of compaction in merge, this case will become a little more common. This way X.merge(Y); Y.toString(); can work a bit faster.
But if we do that, I think we should optimize size == 0 as well, thus:
if (addLen == 0 && size <= 1) return (size == 0) ? "" : elts[0]; Yes, done.
--- 240 if (other == this) {
I doubt that optimizing specially for a self-merge is worthwhile.
You're probably correct, it's not worth coding that. I just was upset that combining with itself doesn't work as well as in other cases, that's why I implemented it. I'll remove it.
Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8054221/1/webrev/
When you're thinking of doing that, you probably really want Strings.repeat(String, int copies) https://code.google.com/p/guava-libraries/source/browse/guava/src/com/google...
I suggest adding repeat to String!
Yes, it would be a useful addition. In addition, there might be overloads with a delimiter, prefix and suffix :-)
Sincerely yours, Ivan
On Tue, Aug 5, 2014 at 6:45 AM, roger riggs <roger.riggs@oracle.com> wrote:
Hi,
Don't get too carried away on adding convience methods, last year when StringJoiner was proposed, there was quite a bit of tension between enough and too much.
StringJoiner is certainly funky. I'm not sure what the perfect string joining API is, but StringJoiner probably isn't it.
Don't add to the size and maintenance of the JDK without a clear use case and knowledge that the APIs will be used frequently.
String repetition is quite popular - in perl, it has its own operator ("x") ! I see thousands of uses of Strings.repeat here at Google.
But if we do that, I think we should optimize size == 0 as well, thus:
if (addLen == 0 && size <= 1) return (size == 0) ? "" : elts[0]; Yes, done.
Or we can just call compactElts() if addLenn == 0, so it will work for any size. Updated the webrev at the same location: http://cr.openjdk.java.net/~igerasim/8054221/1/webrev/ Sincerely yours, Ivan
39 private final static String PREFIXES[] = {"", "{", "@#$%"}; This C style syntax is not good Java style - use String[] instead. 177 if (addLen == 0) { 178 compactElts(); 179 return size == 0 ? "" : elts[0]; 180 } I'm concerned about the extra String[8] created by compactElts. We assume that StringJoiners are all temporary objects, so don't bother creating that shorter String[8] to hold the one sole element on compaction - just reuse the original array (but should we null out the entries? probably) On Tue, Aug 5, 2014 at 12:19 PM, Ivan Gerasimov <ivan.gerasimov@oracle.com> wrote:
But if we do that, I think we should optimize size == 0 as well, thus:
if (addLen == 0 && size <= 1) return (size == 0) ? "" : elts[0];
Yes, done.
Or we can just call compactElts() if addLenn == 0, so it will work for any size.
Updated the webrev at the same location: http://cr.openjdk.java.net/~igerasim/8054221/1/webrev/
Sincerely yours, Ivan
On 06.08.2014 10:14, Martin Buchholz wrote:
39 private final static String PREFIXES[] = {"", "{", "@#$%"}; This C style syntax is not good Java style - use String[] instead.
Thanks! fixed
177 if (addLen == 0) { 178 compactElts(); 179 return size == 0 ? "" : elts[0]; 180 } I'm concerned about the extra String[8] created by compactElts. We assume that StringJoiners are all temporary objects, so don't bother creating that shorter String[8] to hold the one sole element on compaction - just reuse the original array (but should we null out the entries? probably)
The benchmark showed that allocating a new array is faster than nullifying the existing one. But if we set the entries to null in the loop that is already there, it will probably be no slower than allocation. Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8054221/2/webrev/ Sincerely yours, Ivan
On Tue, Aug 5, 2014 at 12:19 PM, Ivan Gerasimov <ivan.gerasimov@oracle.com <mailto:ivan.gerasimov@oracle.com>> wrote:
But if we do that, I think we should optimize size == 0 as well, thus:
if (addLen == 0 && size <= 1) return (size == 0) ? "" : elts[0];
Yes, done.
Or we can just call compactElts() if addLenn == 0, so it will work for any size.
Updated the webrev at the same location: http://cr.openjdk.java.net/~igerasim/8054221/1/webrev/ <http://cr.openjdk.java.net/%7Eigerasim/8054221/1/webrev/>
Sincerely yours, Ivan
I have more comments, but this is good to go. 69 @Test(expectedExceptions = {NullPointerException.class}) Controversial - many people, including myself, think this is too magical without adding much value. My own preference is assertThrows e.g. in http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/JSR166TestC... Microbenchmarks undermeasure the impact of allocation, which increases with heap size. Real world java applications struggle with allocation/gc, not cpu. So don't measure allocation against cpu directly in microbenchmarks. On Wed, Aug 6, 2014 at 2:47 AM, Ivan Gerasimov <ivan.gerasimov@oracle.com> wrote:
On 06.08.2014 10:14, Martin Buchholz wrote:
39 private final static String PREFIXES[] = {"", "{", "@#$%"};
This C style syntax is not good Java style - use String[] instead.
Thanks! fixed
177 if (addLen == 0) { 178 compactElts(); 179 return size == 0 ? "" : elts[0]; 180 }
I'm concerned about the extra String[8] created by compactElts. We assume that StringJoiners are all temporary objects, so don't bother creating that shorter String[8] to hold the one sole element on compaction - just reuse the original array (but should we null out the entries? probably)
The benchmark showed that allocating a new array is faster than nullifying the existing one. But if we set the entries to null in the loop that is already there, it will probably be no slower than allocation.
Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8054221/2/webrev/
Sincerely yours, Ivan
On Tue, Aug 5, 2014 at 12:19 PM, Ivan Gerasimov <ivan.gerasimov@oracle.com
wrote:
But if we do that, I think we should optimize size == 0 as well, thus:
if (addLen == 0 && size <= 1) return (size == 0) ? "" : elts[0];
Yes, done.
Or we can just call compactElts() if addLenn == 0, so it will work for any size.
Updated the webrev at the same location: http://cr.openjdk.java.net/~igerasim/8054221/1/webrev/
Sincerely yours, Ivan
Hi, If a function like assertThrows was included in TestNG, it would make sense to use it. The TestNG expectedException mechanism is adequate and creating a one-off extension to TestNG would raise the maintenance costs without adding much value. Roger On 8/6/2014 10:13 AM, Martin Buchholz wrote:
I have more comments, but this is good to go.
69 @Test(expectedExceptions = {NullPointerException.class})
Controversial - many people, including myself, think this is too magical without adding much value. My own preference is assertThrows e.g. in http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/JSR166TestC...
Microbenchmarks undermeasure the impact of allocation, which increases with heap size. Real world java applications struggle with allocation/gc, not cpu. So don't measure allocation against cpu directly in microbenchmarks.
On Wed, Aug 6, 2014 at 2:47 AM, Ivan Gerasimov <ivan.gerasimov@oracle.com> wrote:
On 06.08.2014 10:14, Martin Buchholz wrote:
39 private final static String PREFIXES[] = {"", "{", "@#$%"};
This C style syntax is not good Java style - use String[] instead.
Thanks! fixed
177 if (addLen == 0) { 178 compactElts(); 179 return size == 0 ? "" : elts[0]; 180 }
I'm concerned about the extra String[8] created by compactElts. We assume that StringJoiners are all temporary objects, so don't bother creating that shorter String[8] to hold the one sole element on compaction - just reuse the original array (but should we null out the entries? probably)
The benchmark showed that allocating a new array is faster than nullifying the existing one. But if we set the entries to null in the loop that is already there, it will probably be no slower than allocation.
Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8054221/2/webrev/
Sincerely yours, Ivan
On Tue, Aug 5, 2014 at 12:19 PM, Ivan Gerasimov <ivan.gerasimov@oracle.com
wrote: But if we do that, I think we should optimize size == 0 as well, thus:
if (addLen == 0 && size <= 1) return (size == 0) ? "" : elts[0];
Yes, done.
Or we can just call compactElts() if addLenn == 0, so it will work for any size.
Updated the webrev at the same location: http://cr.openjdk.java.net/~igerasim/8054221/1/webrev/
Sincerely yours, Ivan
I agree that assertions like assertThrows belongs in your test framework. For this change my preference would be to revert to the previous code without expectedExceptions but author preference wins. On Wed, Aug 6, 2014 at 9:16 AM, roger riggs <roger.riggs@oracle.com> wrote:
Hi,
If a function like assertThrows was included in TestNG, it would make sense to use it. The TestNG expectedException mechanism is adequate and creating a one-off extension to TestNG would raise the maintenance costs without adding much value.
Roger
On 8/6/2014 10:13 AM, Martin Buchholz wrote:
I have more comments, but this is good to go.
69 @Test(expectedExceptions = {NullPointerException.class})
Controversial - many people, including myself, think this is too magical without adding much value. My own preference is assertThrows e.g. in http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/ test/tck/JSR166TestCase.java?revision=1.121&view=markup
Microbenchmarks undermeasure the impact of allocation, which increases with heap size. Real world java applications struggle with allocation/gc, not cpu. So don't measure allocation against cpu directly in microbenchmarks.
On Wed, Aug 6, 2014 at 2:47 AM, Ivan Gerasimov <ivan.gerasimov@oracle.com
wrote:
On 06.08.2014 10:14, Martin Buchholz wrote:
39 private final static String PREFIXES[] = {"", "{", "@#$%"};
This C style syntax is not good Java style - use String[] instead.
Thanks! fixed
177 if (addLen == 0) { 178 compactElts(); 179 return size == 0 ? "" : elts[0]; 180 }
I'm concerned about the extra String[8] created by compactElts. We assume that StringJoiners are all temporary objects, so don't bother creating that shorter String[8] to hold the one sole element on compaction - just reuse the original array (but should we null out the entries? probably)
The benchmark showed that allocating a new array is faster than nullifying the existing one. But if we set the entries to null in the loop that is already there, it will probably be no slower than allocation.
Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8054221/2/webrev/
Sincerely yours, Ivan
On Tue, Aug 5, 2014 at 12:19 PM, Ivan Gerasimov < ivan.gerasimov@oracle.com
wrote: But if we do that, I think we should optimize size == 0 as well, thus:
if (addLen == 0 && size <= 1)
return (size == 0) ? "" : elts[0];
Yes, done.
Or we can just call compactElts() if addLenn == 0, so it will work for
any size.
Updated the webrev at the same location: http://cr.openjdk.java.net/~igerasim/8054221/1/webrev/
Sincerely yours, Ivan
participants (4)
-
Claes Redestad
-
Ivan Gerasimov
-
Martin Buchholz
-
roger riggs