Is that a valid example given StringBuilder isn't threadsafe to begin with? If the SB instance is shared among threads that perform writes to it without external synchronization, it's a bug in that code. Am I missing something?
You'd have to ensure that the returned String is stable and effectively immutable though, which means the underlying byte[] has to be released by SB right before it's mutated if it's shared, which I think Ivan's patch is doing (from a quick glance). sent from my phone On Nov 27, 2015 10:16 AM, "Peter Levart" <[email protected]> wrote: > > > On 11/27/2015 01:39 PM, Ivan Gerasimov wrote: > >> Hello! >> >> Prior to Java5, StringBuffer used to be able to share its internal char[] >> buffer with the String, returned with toString(). >> With introducing of the new StringBuilder class this functionality was >> removed. >> >> It seems tempting to reintroduce this feature now in >> AbstractStringBuilder. >> The rationale here is that StringBuilder already provides a way of >> accepting the hint about the result's size. >> The constructor with the explicitly specified capacity is used for >> increasing the efficiency of strings concatenations. >> Optimizing this case by avoiding additional memory allocation and copying >> looks sensible to me. >> >> Here's the draft webrev >> http://cr.openjdk.java.net/~igerasim/XXXXXXX-ASB-shared-value/00/webrev/ >> > > It is tempting, yes. But I think there was a reason that this was dropped > when StringBuilder was introduced and that reason was thread-safety. > StringBuilder doesn't use any synchronization. If a concurrent thread gets > a reference to a StringBuilder and executes mutating operations while some > other thread calls toString() on the same StringBuilder, then returned > String could appear to be changing it's content (be mutable), which can > have security implications: > > 413 public String toString() { > 414 final byte[] value = this.value; > 415 if (isLatin1()) { > 416 if ((count << coder) < value.length) { > 417 return StringLatin1.newString(value, 0, count); > 418 } > 419 shared = true; > 420 return new String(value, String.LATIN1); > 421 } > 422 return StringUTF16.newStringSB(value, 0, count); > 423 } > > > 927 public AbstractStringBuilder replace(int start, int end, String > str) { > 928 if (end > count) { > 929 end = count; > 930 } > 931 checkRangeSIOOBE(start, end, count); > 932 int len = str.length(); > 933 int newCount = count + len - (end - start); > 934 ensureCapacityInternal(newCount); > 935 unshareValue(); > 936 shift(end, newCount - count); > 937 count = newCount; > 938 putStringAt(start, str); > 939 return this; > 940 } > > > For example: > > static final StringBuilder sb = new StringBuilder(3).append("abc"); > > Thread 1: > > String s = sb.toString(); > System.out.println(s); > > Thread 2: > > sb.replace(0, 3, "def"); > > > Question: What can Thread 1 print? > > Thread 1 sets shared = true and shares value array with String, but when > Thread 2 executes replace, it may not yet notice the write from Thread 1 > and may not do anything in unshareValue(), effectively overwriting the > shared array with "def". > > Answer: I think anything of the following: > > abc > > dbc > aec > abf > > dec > dbf > aef > > def > > ...and the answer may change over time. Now imagine handing over such > string to new FileInputStream()... > > > Regards, Peter > > > >> This variant showed a significant speed improvement for the cases, when >> the the capacity is equal to the result's size (up to +25% to throughput). >> For the other cases, the difference isn't very clear based on my >> benchmarks :) >> But is seems to be small enough, as it only adds a few comparisons, >> coupled with already relatively heavy operations, like allocation and >> copying. >> >> Here's the benchmark that I've used: >> >> http://cr.openjdk.java.net/~igerasim/XXXXXXX-ASB-shared-value/MyBenchmark.java >> >> And the corresponding graphs. >> >> http://cr.openjdk.java.net/~igerasim/XXXXXXX-ASB-shared-value/ASB-Shared-bench-test15.png >> >> http://cr.openjdk.java.net/~igerasim/XXXXXXX-ASB-shared-value/ASB-Shared-bench-test17.png >> The blue line here stands for the baseline throughput. >> >> If people agree, this is a useful addition, a test should also be added, >> of course. >> >> Sincerely yours, >> Ivan >> >> >
