Hi, thank you for taking time to read my proposal. Is this a matter of some OpenJDK coding conventions? I myself never use ternary operator and always use if with braces. My experience is that it improves code readability. If I have to apply one of proposed changes I would choose first or third.
Regards, Tomasz On Thu, May 28, 2015 at 12:35 PM, Ulf Zibis <ulf.zi...@cosoco.de> wrote: > Hi, > > I more would like: > > 2199 return (s instanceof String) > 2200 ? indexOf((String) s) >= 0; > 2201 : indexOf(value, 0, value.length, s, 0, s.length(), > 0) >= 0; > > or: > > 2199 return (s instanceof String > 2200 ? indexOf((String) s) > 2201 : indexOf(value, 0, value.length, s, 0, s.length(), > 0)) > 2202 >= 0; > > or: > > 2199 int index = (s instanceof String) > 2200 ? indexOf((String) s) > 2201 : indexOf(value, 0, value.length, s, 0, s.length(), > 0); > 2202return index >= 0; > > -Ulf > > > > Am 28.05.2015 um 12:06 schrieb Tomasz Kowalczewski: > >> Hello all, >> >> encouraged by your responses I did a simple implementation and performed >> some JMH experiments. Details below. >> >> Webrev [1] contains my changes. I deliberately wanted to do minimal code >> changes. The gist of it is implementing indexOf that works on CharSequence >> instead of String's internal char array. Both versions are almost >> identical >> (I did not change existing implementation in place to not impact all other >> String-only paths that use it). >> >> In my first attempt I just inlined (and simplified) indexOf implementation >> into String::contains, but in the end decided to create general purpose >> (package private) version. This might prove useful for others[2] >> >> JMH test project is here [3]. All benchmarks were run using "java -jar >> benchmarks.jar -wi 10 -i 10 -f -prof gc" on Amazon EC2 instance >> (c4.xlarge, >> 4 virtual cores). I have run it against three java builds: >> >> a. stock java 8u40 >> b. my own build from clean jdk9 sources >> c. my own build from modified jdk9 sources >> >> Results with gc profiler enabled are included in benchmark repo. This gist >> contains summary results [4]. >> >> I know that this test covers only very narrow space of possible >> performance >> regressions (ideas form more comprehensive tests are welcome). The results >> show that String only path is not impacted by my changes. When using >> actual >> CharSequence (StringBuilder in this case) we are mostly winning, exception >> is case when CharSequence is of medium size and has many partial matches >> in >> searched string. >> >> I hope this exercise will be useful to someone and that this change might >> be considered for inclusion in OpenJDK. If not then maybe at least tests >> for String::indexOf? >> >> [1] http://openjdk.s3-website-us-east-1.amazonaws.com/ >> [2] Changeset from >> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-May/033678.html >> uses >> indexOf that could accept CharSequence instead of String. >> [3] https://github.com/tkowalcz/openjdk-jmh >> [4] https://gist.github.com/tkowalcz/d8f5b9435e184f65fd2a >> >> Regards, >> Tomasz Kowalczewski >> >> On Sat, Mar 21, 2015 at 9:21 AM, Tomasz Kowalczewski < >> tomasz.kowalczew...@gmail.com> wrote: >> >> There are other places which accept CharSequence and iterate over it >>> not caring about possible concurrent modification (the only sane way >>> IMHO). Examples are String.contentEquals and String.join that uses >>> StringJoiner which iterates its argument char by char. >>> >>> It looks like contains is the exception in this regard. >>> >>> On 20 mar 2015, at 17:05, Vitaly Davidovich <vita...@gmail.com> wrote: >>> >>> If CharSequence is mutable and thread-safe, I would expect toString() >>>>> implementation to provide the atomic snapshot (e.g. do the >>>>> synchronization). Therefore, I find Sherman's argument interesting. >>>>> >>>> >>>> That's my point about "it ought to be up to the caller" -- they're in a >>>> position to make this call, but String.contains() is playing defensive >>>> because it has no clue of the context. >>>> >>>> On Fri, Mar 20, 2015 at 11:55 AM, Aleksey Shipilev < >>>> aleksey.shipi...@oracle.com> wrote: >>>> >>>> If CharSequence is mutable and thread-safe, I would expect toString() >>>>> implementation to provide the atomic snapshot (e.g. do the >>>>> synchronization). Therefore, I find Sherman's argument interesting. >>>>> >>>>> On the other hand, we don't seem to be having any details/requirements >>>>> for contains(CharSequence) w.r.t. it's own argument. What if >>>>> String.contains pulls the values one charAt at a time? The concurrency >>>>> requirements are not enforceable in CharSequence alone then, unless you >>>>> call the supposed-to-be-atomic toString() first. >>>>> >>>>> -Aleksey. >>>>> >>>>> On 03/20/2015 06:48 PM, Vitaly Davidovich wrote: >>>>>> Yes, but that ought to be for the caller to decide. Also, although >>>>>> the >>>>>> resulting String is immutable, toString() itself may observe mutation. >>>>>> >>>>>> On Fri, Mar 20, 2015 at 11:40 AM, Xueming Shen < >>>>>> >>>>> xueming.s...@oracle.com> >>> >>>> wrote: >>>>>> >>>>>> On 03/20/2015 02:34 AM, Tomasz Kowalczewski wrote: >>>>>>>> >>>>>>>> Hello! >>>>>>>> >>>>>>>> Current implementation of String.contains that accepts CharSequence >>>>>>>> >>>>>>> calls >>>>> >>>>>> toString on it and passes resulting string to indexOf(String). This >>>>>>>> >>>>>>> IMO >>> >>>> defeats the purpose of using CharSequences (that is to have a mutable >>>>>>>> character buffer and not allocate unnecessary objects). >>>>>>>> >>>>>>> It is arguable that cs.toString() may serve the purpose of taking a >>>>>>> snapshot of an otherwise >>>>>>> "mutable" character buffer? >>>>>>> >>>>>>> -Sherman >>>>>>> >>>>>>> >>>>>>> Is changing this a desirable development? It seems pretty >>>>>>> >>>>>> straightforward >>>>> >>>>>> to port indexOf(String) to use CharSequence. >>>>>>>> >>>>>>>> If all you need is patch then I can work on it (I have signed OCA) >>>>>>>> >>>>>>> just >>> >>>> wanted to make sure it is not a futile work. >>>>>>>> >>>>>>> >>>>> >>>>> >> >> > -- Tomasz Kowalczewski
