Re: RFR: 8259842: Remove Result cache from StringCoding [v11]

2021-01-22 Thread Claes Redestad
On Thu, 21 Jan 2021 22:43:50 GMT, Naoto Sato wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Reduce code duplication in getBytes/getBytesNoRepl > > Marked as reviewed by naoto (Reviewer). Passed testing tiers 1-

Re: RFR: 8259842: Remove Result cache from StringCoding [v12]

2021-01-21 Thread Claes Redestad
> The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak

Re: RFR: 8259842: Remove Result cache from StringCoding [v11]

2021-01-21 Thread Naoto Sato
On Thu, 21 Jan 2021 20:48:33 GMT, Claes Redestad wrote: >> The `StringCoding.resultCached` mechanism is used to remove the allocation >> of a `StringCoding.Result` object on potentially hot paths used in some >> `String` constructors. Using a `ThreadLocal` has overheads though, and the >> over

Re: RFR: 8259842: Remove Result cache from StringCoding [v11]

2021-01-21 Thread Roger Riggs
On Thu, 21 Jan 2021 20:48:33 GMT, Claes Redestad wrote: >> The `StringCoding.resultCached` mechanism is used to remove the allocation >> of a `StringCoding.Result` object on potentially hot paths used in some >> `String` constructors. Using a `ThreadLocal` has overheads though, and the >> over

Re: RFR: 8259842: Remove Result cache from StringCoding [v11]

2021-01-21 Thread Claes Redestad
> The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak

Re: RFR: 8259842: Remove Result cache from StringCoding [v10]

2021-01-21 Thread Claes Redestad
> The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak

Re: RFR: 8259842: Remove Result cache from StringCoding [v9]

2021-01-21 Thread Claes Redestad
> The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak

Re: RFR: 8259842: Remove Result cache from StringCoding [v8]

2021-01-21 Thread Claes Redestad
> The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak

Re: RFR: 8259842: Remove Result cache from StringCoding [v7]

2021-01-19 Thread Roger Riggs
On Mon, 18 Jan 2021 09:26:07 GMT, Claes Redestad wrote: >> The `StringCoding.resultCached` mechanism is used to remove the allocation >> of a `StringCoding.Result` object on potentially hot paths used in some >> `String` constructors. Using a `ThreadLocal` has overheads though, and the >> over

Re: RFR: 8259842: Remove Result cache from StringCoding [v7]

2021-01-18 Thread Peter Levart
On Mon, 18 Jan 2021 09:26:07 GMT, Claes Redestad wrote: >> The `StringCoding.resultCached` mechanism is used to remove the allocation >> of a `StringCoding.Result` object on potentially hot paths used in some >> `String` constructors. Using a `ThreadLocal` has overheads though, and the >> over

Re: RFR: 8259842: Remove Result cache from StringCoding [v7]

2021-01-18 Thread Claes Redestad
> The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak

Re: RFR: 8259842: Remove Result cache from StringCoding [v6]

2021-01-17 Thread Claes Redestad
> The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak

Re: RFR: 8259842: Remove Result cache from StringCoding [v5]

2021-01-17 Thread Claes Redestad
On Sun, 17 Jan 2021 15:51:58 GMT, Peter Levart wrote: > `newStringUTF8NoRepl()` does not contain this optimization. Good catch: this optimization was in the original code for `newStringNoRepl` but not `newStringUTF8NoRepl`. I'll put it back to avoid a regression, but this time into `newStr

Re: RFR: 8259842: Remove Result cache from StringCoding [v5]

2021-01-17 Thread Peter Levart
On Sun, 17 Jan 2021 14:56:40 GMT, Peter Levart wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Simplify lookupCharset > > This looks good. > Are you planning to do similar things for encoding too? I already appr

Re: RFR: 8259842: Remove Result cache from StringCoding [v5]

2021-01-17 Thread Peter Levart
On Sun, 17 Jan 2021 12:07:03 GMT, Claes Redestad wrote: >> The `StringCoding.resultCached` mechanism is used to remove the allocation >> of a `StringCoding.Result` object on potentially hot paths used in some >> `String` constructors. Using a `ThreadLocal` has overheads though, and the >> over

Re: RFR: 8259842: Remove Result cache from StringCoding [v5]

2021-01-17 Thread Claes Redestad
On Sun, 17 Jan 2021 09:21:27 GMT, Peter Levart wrote: >> This looks much better now. Maybe just one small suggestion: >> `java.lang.StringCoding#lookupCharset` is used in two places and in both >> places the same exception handling/rethrowing logic is wrapped around the >> invocation. So you c

Re: RFR: 8259842: Remove Result cache from StringCoding [v5]

2021-01-17 Thread Claes Redestad
> The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak

Re: RFR: 8259842: Remove Result cache from StringCoding [v4]

2021-01-17 Thread Peter Levart
On Sun, 17 Jan 2021 09:03:30 GMT, Peter Levart wrote: >>> Do you think this code belongs more to String than to StringCoding? >> >> I consider StringCoding an implementation detail of String, so I'm not sure >> there's (much) value in maintaining the separation of concern if it is cause >> for

Re: RFR: 8259842: Remove Result cache from StringCoding [v4]

2021-01-17 Thread Peter Levart
On Sat, 16 Jan 2021 01:02:20 GMT, Claes Redestad wrote: >> Some common logic is now extracted into methods. That's good. But much of >> the decoding logic still remains in String. I still think all of static >> methods can be moved to StringCoding directly as they are now while the >> private

Re: RFR: 8259842: Remove Result cache from StringCoding [v4]

2021-01-15 Thread Claes Redestad
On Sat, 16 Jan 2021 00:25:24 GMT, Peter Levart wrote: > Do you think this code belongs more to String than to StringCoding? I consider StringCoding an implementation detail of String, so I'm not sure there's (much) value in maintaining the separation of concern if it is cause for a performance

Re: RFR: 8259842: Remove Result cache from StringCoding [v4]

2021-01-15 Thread Claes Redestad
> The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak

Re: RFR: 8259842: Remove Result cache from StringCoding [v3]

2021-01-15 Thread Peter Levart
On Fri, 15 Jan 2021 22:03:31 GMT, Claes Redestad wrote: >> Hi Claes, >> This is quite an undertaking in re-factoring! >> I think that decoding logic that you produced can still be kept in >> StringCoding class though. The private constructor that you created for >> UTF-8 decoding is unnecessary

Re: RFR: 8259842: Remove Result cache from StringCoding [v3]

2021-01-15 Thread Claes Redestad
> The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak

Re: RFR: 8259842: Remove Result cache from StringCoding [v2]

2021-01-15 Thread Claes Redestad
On Fri, 15 Jan 2021 21:39:00 GMT, Peter Levart wrote: > WDYT? I get that the approach I took got a bit messy, but I've just spent some time cleaning it up. Please have a look at the latest, which outlines much of the logic and consolidates the replace/throw logic in the UTF8 decode paths. I've

Re: RFR: 8259842: Remove Result cache from StringCoding [v2]

2021-01-15 Thread Claes Redestad
> The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak

Re: RFR: 8259842: Remove Result cache from StringCoding

2021-01-15 Thread Peter Levart
On Fri, 15 Jan 2021 19:14:06 GMT, Naoto Sato wrote: >> The `StringCoding.resultCached` mechanism is used to remove the allocation >> of a `StringCoding.Result` object on potentially hot paths used in some >> `String` constructors. Using a `ThreadLocal` has overheads though, and the >> overhead

Re: RFR: 8259842: Remove Result cache from StringCoding

2021-01-15 Thread Roger Riggs
On Fri, 15 Jan 2021 20:05:17 GMT, Claes Redestad wrote: >> src/java.base/share/classes/java/lang/String.java line 544: >> >>> 542: return; >>> 543: } >>> 544: if (charset == UTF_8) { >> >> The constructor is getting big. Might be better to keep the original private

Re: RFR: 8259842: Remove Result cache from StringCoding

2021-01-15 Thread Roger Riggs
On Fri, 15 Jan 2021 20:14:48 GMT, Roger Riggs wrote: >> Since we're calculating two final values, that split was what necessitated a >> `Result` object. Until valhalla I don't think there's a way to get rid of >> the performance cost here without putting the bulk of the logic into the >> const

Re: RFR: 8259842: Remove Result cache from StringCoding

2021-01-15 Thread Claes Redestad
On Fri, 15 Jan 2021 19:11:38 GMT, Naoto Sato wrote: >> The `StringCoding.resultCached` mechanism is used to remove the allocation >> of a `StringCoding.Result` object on potentially hot paths used in some >> `String` constructors. Using a `ThreadLocal` has overheads though, and the >> overhead

Re: RFR: 8259842: Remove Result cache from StringCoding

2021-01-15 Thread Naoto Sato
On Fri, 15 Jan 2021 14:33:19 GMT, Claes Redestad wrote: > The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead >

Re: RFR: 8259842: Remove Result cache from StringCoding

2021-01-15 Thread Claes Redestad
On Fri, 15 Jan 2021 16:49:57 GMT, Roger Riggs wrote: > Interesting, I was/am in the middle of converting Result to be a Valhalla > primitive class to reduce the memory pressure and had written some new jmh > tests too. > And to reduce the dependency on ThreadLocals. Ok, I expect that would end

Re: RFR: 8259842: Remove Result cache from StringCoding

2021-01-15 Thread Roger Riggs
On Fri, 15 Jan 2021 14:33:19 GMT, Claes Redestad wrote: > The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead >