Re: RFR: JDK-8267936: PreserveAllAnnotations option doesn't expose the annotation to Java code

2021-05-31 Thread Peter Levart
On Mon, 31 May 2021 22:36:52 GMT, David Holmes wrote: > > OTOH when CLASS retention is changing to RUNTIME, such as in this case, the > > ways of looking up are widening (there is some new desire to lookup the > > annotation at runtime via reflection). To facilitate that, the annotation > >

Re: RFR: JDK-8267936: PreserveAllAnnotations option doesn't expose the annotation to Java code

2021-05-31 Thread Peter Levart
On Mon, 31 May 2021 12:37:55 GMT, David Holmes wrote: > That reads backwards to me. +PreserveAllAnnotations means that CLASS > retention annotations are retained by the VM not just RUNTIME ones. So > in that sense it might be a migration aid if moving from RUNTIME to > CLASS, not CLASS to

Re: RFR: JDK-8267936: PreserveAllAnnotations option doesn't expose the annotation to Java code

2021-05-31 Thread Peter Levart
On Mon, 31 May 2021 12:37:55 GMT, David Holmes wrote: > > Hi Jaroslav, > > If you change `@JavaScriptBody` retention to `RUNTIME`, you don't need to > > re-compile any code that uses this annotation. You just have to use > > `-XX:+PreserveAllAnnotations` to expose all anotations to JVM and >

Re: RFR: JDK-8267936: PreserveAllAnnotations option doesn't expose the annotation to Java code [v4]

2021-05-31 Thread Peter Levart
On Mon, 31 May 2021 06:06:37 GMT, Jaroslav Tulach wrote: >> This PR exposes runtime invisible annotations via `Class.getAnnotation` when >> `-XX:+PreserveAllAnnotations` option is passed to the JVM. >> >> Existing `-XX:+PreserveAllAnnotations` option can be very useful for code >> that needs

Re: RFR: JDK-8267936: PreserveAllAnnotations option doesn't expose the annotation to Java code [v4]

2021-05-31 Thread Peter Levart
On Mon, 31 May 2021 06:06:37 GMT, Jaroslav Tulach wrote: >> This PR exposes runtime invisible annotations via `Class.getAnnotation` when >> `-XX:+PreserveAllAnnotations` option is passed to the JVM. >> >> Existing `-XX:+PreserveAllAnnotations` option can be very useful for code >> that needs

Re: RFR: JDK-8267936: PreserveAllAnnotations option doesn't expose the annotation to Java code [v2]

2021-05-31 Thread Peter Levart
On Sun, 30 May 2021 20:06:49 GMT, Jaroslav Tulach wrote: > My use-case relates to > [@JavaScriptBody](https://bits.netbeans.org/html+java/1.7.1/net/java/html/js/package-summary.html) > annotation used for Java/JavaScript interop. Originally all existing usages > (Post Processing Classes,

Re: RFR: 8266310: deadlock while loading the JNI code [v5]

2021-05-31 Thread Peter Levart
On Thu, 27 May 2021 14:28:09 GMT, Aleksei Voitylov wrote: >> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line >> 481: >> >>> 479: throw new Error("Maximum lock count exceeded"); >>> 480: } >>> 481: >> >> Hi Aleksei, >> I know in practice this

Re: RFR: 8266310: deadlock while loading the JNI code [v6]

2021-05-31 Thread Peter Levart
On Thu, 27 May 2021 14:31:59 GMT, Aleksei Voitylov wrote: >> Please review this PR which fixes the deadlock in ClassLoader between the >> two lock objects - a lock object associated with the class being loaded, and >> the ClassLoader.loadedLibraryNames hash map, locked during the native >>

Re: RFR: 8266310: deadlock while loading the JNI code [v5]

2021-05-26 Thread Peter Levart
On Fri, 21 May 2021 15:49:09 GMT, Aleksei Voitylov wrote: >> Please review this PR which fixes the deadlock in ClassLoader between the >> two lock objects - a lock object associated with the class being loaded, and >> the ClassLoader.loadedLibraryNames hash map, locked during the native >>

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v9]

2021-05-26 Thread Peter Levart
On Wed, 26 May 2021 06:21:12 GMT, Peter Levart wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Editorial updates >> Updated java.security properties to include jdk.serialFilte

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v9]

2021-05-26 Thread Peter Levart
On Tue, 25 May 2021 21:45:33 GMT, Roger Riggs wrote: >> JEP 415: Context-specific Deserialization Filters extends the >> deserialization filtering mechanisms with more flexible and customizable >> protections against malicious deserialization. See JEP 415: >>

Re: RFR: 8266310: deadlock while loading the JNI code [v2]

2021-05-21 Thread Peter Levart
On 21/05/2021 10:29, Peter Levart wrote: I still haven't found a scenario of a possible deadlock when Sergei's initial patch is combined with... Sorry Aleksei, I renamed you to Sergei without intention. Please excuse me. Peter

Re: RFR: 8266310: deadlock while loading the JNI code [v2]

2021-05-21 Thread Peter Levart
On 21/05/2021 01:11, David Holmes wrote: Hi Peter, On 21/05/2021 12:42 am, Peter Levart wrote: Hi Aleksei, Are you trying to solve this in principle or do you have a concrete problem at hand which triggers this deadlock? If it is the later, then some rearrangement of code might do

Re: RFR: 8266310: deadlock while loading the JNI code [v2]

2021-05-20 Thread Peter Levart
Hi Aleksei, Are you trying to solve this in principle or do you have a concrete problem at hand which triggers this deadlock? If it is the later, then some rearrangement of code might do the trick... For example, native libraries are typically loaded by a class initializer of some class that

Re: [External] : Re: ReversibleCollection proposal

2021-05-20 Thread Peter Levart
On 20/05/2021 08:35, dfranken@gmail.com wrote: I also think the proposal of Stuart is reasonable though, but it seemed to me that we had reached some sort of impasse in this discussion. As Remi points out, we have (default) methods which sometimes throw exceptions when they are implemented

Re: JEP draft: Scope Locals

2021-05-19 Thread Peter Levart
On 15/05/2021 19:50, Peter Levart wrote: Another question: Suppose there are two inheritable ScopeLocal variables with bound values in scope (A and B) when I take a snapshot: var snapshot = ScopeLocal.snapshot(); now I pass that snapshot to a thread which does the following: ScopeLocal

Re: RFR: 8266846: Add java.time.InstantSource [v3]

2021-05-19 Thread Peter Levart
On Tue, 18 May 2021 23:14:53 GMT, Stephen Colebourne wrote: >> src/java.base/share/classes/java/time/Clock.java line 487: >> >>> 485: // it more unlikely to hit the 1ns in the future condition. >>> 486: localOffset = System.currentTimeMillis()/1000 - 1024; >>> 487: >>

Re: RFR: 8266846: Add java.time.InstantSource [v2]

2021-05-18 Thread Peter Levart
On Mon, 17 May 2021 17:19:19 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource Changes requested by plevart

Re: JEP draft: Scope Locals

2021-05-15 Thread Peter Levart
Hi, So if scopeLocal.get() is called in a thread outside any .run() scope in that thread, it will throw exception and scopeLocal.isBound() will return false. Unless it was called on an inheritableScopeLocal which inherited binding from parent thread. What if I wanted to create and start a

Re: RFR: 8266310: deadlock while loading the JNI code

2021-05-12 Thread Peter Levart
On Tue, 11 May 2021 13:10:30 GMT, Aleksei Voitylov wrote: > Please review this PR which fixes the deadlock in ClassLoader between the two > lock objects - a lock object associated with the class being loaded, and the > ClassLoader.loadedLibraryNames hash map, locked during the native library

Re: [External] : Re: ReversibleCollection proposal

2021-05-12 Thread Peter Levart
On 5/12/21 2:55 AM, Stuart Marks wrote: As you point out, add() is kind of like addLast(), except without the reordering semantics for LinkedHashSet. And reversed().add() is a roundabout way of saying addFirst() -- also without the reordering semantics for LinkedHashSet. I think most

Re: [External] : Re: ReversibleCollection proposal

2021-05-11 Thread Peter Levart
Hi Remi, I see you avoided addFirst/addLast methods. While getFirst/removeFirst could be specified to be consistent with iterator().next(), so the "first" part of the name would be softer, addFirst is cursed two times: it has no equivalent in current API and it clashes with void returning

Re: RFR: 8266622: Optimize Class.descriptorString() and Class.getCanonicalName0()

2021-05-07 Thread Peter Levart
On Thu, 6 May 2021 16:34:04 GMT, Peter Levart wrote: >> Hello, from discussion in https://github.com/openjdk/jdk/pull/3464 and >> https://github.com/openjdk/jdk/pull/2212 it appears, that in `j.l.Class` >> expressions like >> >> String str = baseName.

Re: RFR: 8266622: Optimize Class.descriptorString() and Class.getCanonicalName0()

2021-05-06 Thread Peter Levart
On Thu, 6 May 2021 15:20:23 GMT, Сергей Цыпанов wrote: > Hello, from discussion in https://github.com/openjdk/jdk/pull/3464 and > https://github.com/openjdk/jdk/pull/2212 it appears, that in `j.l.Class` > expressions like > > String str = baseName.replace('.', '/') + '/' + name; > > are not

Re: [External] : Re: ReversibleCollection proposal

2021-05-01 Thread Peter Levart
On 4/30/21 9:25 PM, Stuart Marks wrote: So I think we're stuck with void-returning add{First,Last} methods. Have you thought of perhaps not adding them at all? Collection.add() for: List(s) - behaves the same as addLast() LinkedHashSet - behaves the same as addLast() Deque - behaves

Re: [External] : Re: ReversibleCollection proposal

2021-04-28 Thread Peter Levart
On 4/28/21 7:19 AM, Stuart Marks wrote: On 4/27/21 9:17 AM, Anthony Vanelverdinghe wrote: On Tuesday, April 27, 2021 11:25 CEST, Peter Levart wrote: Now just some of my thoughts about the proposal: - SortedSet.addFirst/.addLast - I think an operation that would be used in situations like

Re: ReversibleCollection proposal

2021-04-28 Thread Peter Levart
On 4/28/21 2:04 AM, Stuart Marks wrote: Binary compatibility is important. And I guess there is a variant of the proposal that includes ReversibleCollection and ReversibleSet and is binary compatible. Yes, the source incompatibilities with the types seem to involve type inference (e.g.,

Re: RFR: 8265418: Clean-up redundant null-checks of Class.getPackageName()

2021-04-27 Thread Peter Levart
On Tue, 27 Apr 2021 09:17:43 GMT, Сергей Цыпанов wrote: > > Also be careful not to return a string which > > is not interned (which would happen if you did what you are proposing > > above). > > Ok, I'm probably missing something, but when we move `String.intern()` call > to `this.packageName

Re: ReversibleCollection proposal

2021-04-27 Thread Peter Levart
On 4/27/21 5:50 AM, Stuart Marks wrote: On 4/25/21 11:09 AM, Peter Levart wrote: When making this proposition, I was only thinking of how to enable new yet-nonexistent operations on collections that have order / are reversible and that the code calling these methods would already knows

Re: RFR: 8265418: Clean-up redundant null-checks of Class.getPackageName()

2021-04-26 Thread Peter Levart
On 4/26/21 1:27 PM, Сергей Цыпанов wrote: On Mon, 26 Apr 2021 08:45:52 GMT, Alan Bateman wrote: @AlanBateman if DL is not responding, will it be ok to just revert the changes related to `java.util.concurrent`? That should be fine, the null check in Objects.equals is benign with these

Re: ReversibleCollection proposal

2021-04-25 Thread Peter Levart
On 4/23/21 8:33 AM, Stuart Marks wrote: This is what I intended anyway, ie that its OK for "first" to work on an unordered collection, just that addFirst() has very weak semantics wrt first-ness. "Ensures that this collection contains the specified element(optional operation). This has the

Re: ReversibleCollection proposal

2021-04-21 Thread Peter Levart
On 4/22/21 12:14 AM, Stephen Colebourne wrote: public interface Collection { default E getFirst() { return iterator().next(); } Searching for ".iterator().next()" yields 74 matches in JDK sources... Peter

Integrated: 8265237: String.join and StringJoiner can be improved further

2021-04-21 Thread Peter Levart
On Wed, 14 Apr 2021 18:58:57 GMT, Peter Levart wrote: > While JDK-8148937 improved StringJoiner class by replacing internal use of > getChars that copies out characters from String elements into a char[] array > with StringBuilder which is somehow more optimal, the improvement was &

Re: RFR: 8265237: String.join and StringJoiner can be improved further [v4]

2021-04-20 Thread Peter Levart
gs), while creation of garbage has been further reduced to an almost > garbage-free operation. > > So WDYT? Peter Levart has updated the pull request incrementally with one additional commit since the last revision: Fork JVM 3 times in JMH test to smooth out variance in a particular

Re: RFR: 8265237: String.join and StringJoiner can be improved further [v3]

2021-04-19 Thread Peter Levart
On Mon, 19 Apr 2021 10:44:04 GMT, Claes Redestad wrote: >> Peter Levart has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix overflow checking logic, add test for it, avoid branch in loop, add >> 1-cha

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]

2021-04-19 Thread Peter Levart
On Mon, 19 Apr 2021 09:34:56 GMT, Alan Bateman wrote: > an application or library can use the attach mechanism (directly or via the > attach API in a child VM) to load an agent and leak the Instrumentation > object. This is the genie that somehow needs to be put back in its bottle. > One

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]

2021-04-19 Thread Peter Levart
contains one new commit since the last revision: > > 8200559: Java agents doing instrumentation need a means to define auxiliary > classes > This won't work as agents are loaded by the system class loader, not the boot > loader. Peter Levart ***@***.***> schrieb am Mo., 19. Ap

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]

2021-04-19 Thread Peter Levart
On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter wrote: >> To allow agents the definition of auxiliary classes, an API is needed to >> allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or >> `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed

Re: RFR: 8265237: String.join and StringJoiner can be improved further [v2]

2021-04-18 Thread Peter Levart
On Fri, 16 Apr 2021 23:02:33 GMT, Peter Levart wrote: >> src/java.base/share/classes/java/lang/String.java line 3254: >> >>> 3252: >>> 3253: byte[] value = StringConcatHelper.newArray(((long) icoder << >>> 32) | llen); >>> 3254:

Re: RFR: 8265237: String.join and StringJoiner can be improved further [v3]

2021-04-18 Thread Peter Levart
On Sun, 18 Apr 2021 19:55:20 GMT, Peter Levart wrote: >> While JDK-8148937 improved StringJoiner class by replacing internal use of >> getChars that copies out characters from String elements into a char[] array >> with StringBuilder which is somehow more optima

Re: RFR: 8265237: String.join and StringJoiner can be improved further [v3]

2021-04-18 Thread Peter Levart
gs), while creation of garbage has been further reduced to an almost > garbage-free operation. > > So WDYT? Peter Levart has updated the pull request incrementally with one additional commit since the last revision: Fix overflow checking logic, add test for it, avoid branch in loop, add

Re: ReversibleCollection proposal

2021-04-18 Thread Peter Levart
On 4/18/21 9:51 AM, Peter Levart wrote: public interface Collection {     default Collection reversed() { return this; }     default void addFirst(E e) { throw new UnsupportedOperationException(); }     default void addLast(E e) { throw new UnsupportedOperationException

Re: ReversibleCollection proposal

2021-04-18 Thread Peter Levart
On 4/18/21 9:51 AM, Peter Levart wrote: public interface Collection {     default Collection reversed() { return this; }     default void addFirst(E e) { throw new UnsupportedOperationException(); }     default void addLast(E e) { throw new UnsupportedOperationException

Re: ReversibleCollection proposal

2021-04-18 Thread Peter Levart
A word for Remi's concern... While it is good that List (and SortedSet and Deque) extend ReversibleCollection in the proposal, it is a pity the same is not true for Set and Queue. If Set and Queue could also be ReversibleCollection(s), then we would not need ReversibleCollection at all and

Re: RFR: 8265237: String.join and StringJoiner can be improved further [v2]

2021-04-16 Thread Peter Levart
On Fri, 16 Apr 2021 19:05:25 GMT, Roger Riggs wrote: >> Peter Levart has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add String.join benchmark method to StringJoinerBenchmark and adjust some >> parameters

Re: RFR: 8265237: String.join and StringJoiner can be improved further

2021-04-15 Thread Peter Levart
On Wed, 14 Apr 2021 20:03:27 GMT, Claes Redestad wrote: > There's a StringJoinerBenchmark micro added by JDK-8148937 which could > perhaps be expanded with the scenarios you've experimented with here? I modified that micro benchmark and added a method to also measure String.join static method

Re: RFR: 8265237: String.join and StringJoiner can be improved further [v2]

2021-04-15 Thread Peter Levart
gs), while creation of garbage has been further reduced to an almost > garbage-free operation. > > So WDYT? Peter Levart has updated the pull request incrementally with one additional commit since the last revision: Add String.join benchmark method to StringJoinerBenchmark and adjust so

Re: RFR: 8265237: String.join and StringJoiner can be improved further

2021-04-14 Thread Peter Levart
On Wed, 14 Apr 2021 19:54:26 GMT, Florent Guillaume wrote: >> While JDK-8148937 improved StringJoiner class by replacing internal use of >> getChars that copies out characters from String elements into a char[] array >> with StringBuilder which is somehow more optimal, the improvement was >>

Re: RFR: 8265237: String.join and StringJoiner can be improved further

2021-04-14 Thread Peter Levart
On Wed, 14 Apr 2021 18:58:57 GMT, Peter Levart wrote: > While JDK-8148937 improved StringJoiner class by replacing internal use of > getChars that copies out characters from String elements into a char[] array > with StringBuilder which is somehow more optimal, the improvement was &

RFR: 8265237: String.join and StringJoiner can be improved further

2021-04-14 Thread Peter Levart
While JDK-8148937 improved StringJoiner class by replacing internal use of getChars that copies out characters from String elements into a char[] array with StringBuilder which is somehow more optimal, the improvement was marginal in speed (0% ... 10%) and mainly for smaller strings, while GC

Re: RFR: 8265075: Improve and simplify Class.resolveName()

2021-04-13 Thread Peter Levart
Hi, Sergey! Have you measured the code change in the java.lang.Class itself or just equivalent code in the JMH test as you show us? The JMH test may show better results as it is compiled to bytecode that uses special invokedynamic-based string concatenation with optimal MH based

Re: Garbage Free Check

2021-04-06 Thread Peter Levart
Hi Ralph, Which version of JDK did you try running the code. I tried the following benchmark: @BenchmarkMode(Mode.AverageTime) @Fork(value = 1) @Warmup(iterations = 5, time = 1) @Measurement(iterations = 10, time = 1) @OutputTimeUnit(TimeUnit.NANOSECONDS) @State(Scope.Benchmark) public

Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test

2021-03-18 Thread Peter Levart
On Thu, 18 Mar 2021 14:30:21 GMT, Thomas Stuefe wrote: >> The test expects there to be zero output from the child (and it doesn't >> matter what state the child is in). >> Can the logging from the VM be disabled or re-directed? > >> The test expects there to be zero output from the child (and

Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test

2021-03-17 Thread Peter Levart
On Wed, 17 Mar 2021 14:16:36 GMT, Roger Riggs wrote: > Intermittent failures on Windows in a test of destroying the child warrant > extending the time the parent waits after starting the child before > destroying the child. test/jdk/java/lang/ProcessBuilder/Basic.java line 2183: > 2181:

Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v5]

2021-03-17 Thread Peter Levart
On Tue, 16 Mar 2021 14:47:29 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed patch for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8263108? >> >> As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and >>

Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v4]

2021-03-16 Thread Peter Levart
On Tue, 16 Mar 2021 07:29:34 GMT, Vyom Tewari wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Use Chris' suggested solution of avoiding deadlock > > Looks good to me. Holder pattern is easier to understand, I

Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]

2021-03-12 Thread Peter Levart
On Wed, 10 Mar 2021 02:15:28 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed patch for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8263108? >> >> As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and >>

Re: RFR: 8263108: Class initialization deadlock in java.lang.constant

2021-03-09 Thread Peter Levart
On Tue, 9 Mar 2021 13:50:05 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed patch for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8263108? >> >> As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and >>

Re: RFR: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now" [v4]

2021-03-01 Thread Peter Levart
On Sun, 28 Feb 2021 10:28:56 GMT, Attila Szegedi wrote: >> 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with >> "AssertionError: Should have GCd a method handle by now" > > Attila Szegedi has updated the pull request with a new target base due to a > merge or a rebase.

Re: RFR: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now" [v2]

2021-02-26 Thread Peter Levart
On Thu, 25 Feb 2021 15:37:39 GMT, Aleksey Shipilev wrote: >> Attila Szegedi has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. > > The good test would be trying

Re: RFR: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now"

2021-02-19 Thread Peter Levart
On Thu, 18 Feb 2021 19:41:21 GMT, Attila Szegedi wrote: >> 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with >> "AssertionError: Should have GCd a method handle by now" > > @plevart can I bother you for a follow-up review of my original issue? > (Alternatively,

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v7]

2021-02-07 Thread Peter Levart
On Sun, 10 Jan 2021 17:04:19 GMT, Attila Szegedi wrote: >> _(NB: For this leak to occur, an application needs to be either creating and >> discarding linkers frequently, or creating and discarding class loaders >> frequently while creating Dynalink type converters for classes in them. >>

Re: RFR: 8132984: incorrect type for Reference.discovered [v3]

2021-01-19 Thread Peter Levart
On Mon, 18 Jan 2021 23:42:08 GMT, Kim Barrett wrote: >> Please review this change which fixes the type of the private >> Reference.discovered field. It was Reference, but that's wrong because >> it can be any Reference object. >> >> I've changed it to Reference and let that flow through,

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 >>

Re: RFR: 8132984: incorrect type for Reference.discovered

2021-01-18 Thread Peter Levart
On Sat, 26 Dec 2020 03:25:51 GMT, Kim Barrett wrote: > Please review this change which fixes the type of the private > Reference.discovered field. It was Reference, but that's wrong because > it can be any Reference object. > > I've changed it to Reference and let that flow through, updating

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]

2021-01-18 Thread Peter Levart
On Sat, 9 Jan 2021 23:06:22 GMT, Philippe Marschall wrote: >> Implement three optimiztations for Reader.read(CharBuffer) >> >> * Add a code path for heap buffers in Reader#read to use the backing array >> instead of allocating a new one. >> * Change the code path for direct buffers in

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 th

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 >>

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 separatio

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 [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

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 >>

Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll [v7]

2021-01-15 Thread Peter Levart
On Thu, 14 Jan 2021 22:38:53 GMT, Peter Levart wrote: >> Сергей Цыпанов has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request cont

Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll [v7]

2021-01-14 Thread Peter Levart
On Thu, 14 Jan 2021 15:25:25 GMT, Сергей Цыпанов wrote: >> Hello, I feel like this was previously discussed in >> https://mail.openjdk.java.net/pipermail/core-libs-dev/ but since I cannot >> find original mail I post this here. >> >> Currently `Collections.addAll()` is implemented and

Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll

2021-01-14 Thread Peter Levart
On Fri, 8 Jan 2021 11:39:17 GMT, Peter Levart wrote: >> Hi @stsypanov, >> >>> The **behavior** of this convenience method is **identical** to that of >>> c.addAll(Arrays.asList(elements)) >> >> What about: >> >>>

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v7]

2021-01-10 Thread Peter Levart
On Sun, 10 Jan 2021 19:38:11 GMT, Attila Szegedi wrote: >> Hello Attila, >> This looks good to me. Just a question: How frequent are situations where >> the two classloaders are unrelated? > >> How frequent are situations where the two classloaders are unrelated? > > I think they'd be

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v7]

2021-01-10 Thread Peter Levart
On Sun, 10 Jan 2021 17:01:31 GMT, Attila Szegedi wrote: >> Yes, the pre-initialized fields to Map.of() are safe regardless of whether >> they are volatile or not (so I would keep them non-volatile to optimize >> fast-path). Because the BiClassValues instance is published safely to other >>

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

2021-01-08 Thread Peter Levart
On Fri, 8 Jan 2021 16:50:24 GMT, Attila Szegedi wrote: >> I checked the code of ClassValue and it can be assumed that it publishes >> associated values safely. The proof is that it keeps values that it >> publishes assigned to the final field `java.lang.ClassValue.Entry#value`. > > So, are you

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

2021-01-08 Thread Peter Levart
On Fri, 8 Jan 2021 13:00:16 GMT, Peter Levart wrote: >>> So what do you think of this variant: >> >> I like it. I originally kept the fields volatile so that we don't observe >> stale values on `getForward`/`getReverse`, but you're right in that even if

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

2021-01-08 Thread Peter Levart
On 1/8/21 2:03 PM, Peter Levart wrote: On Fri, 8 Jan 2021 12:23:36 GMT, Attila Szegedi wrote: IIUC, your changes mostly all flow from the decision to declare the fields as non-volatile; if they were still declared as volatile then it'd be impossible to observe null in them, I think

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

2021-01-08 Thread Peter Levart
On Fri, 8 Jan 2021 12:47:17 GMT, Attila Szegedi wrote: >> _(NB: For this leak to occur, an application needs to be either creating and >> discarding linkers frequently, or creating and discarding class loaders >> frequently while creating Dynalink type converters for classes in them. >>

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

2021-01-08 Thread Peter Levart
On Fri, 8 Jan 2021 12:23:36 GMT, Attila Szegedi wrote: > IIUC, your changes mostly all flow from the decision to declare the fields as > non-volatile; if they were still declared as volatile then it'd be impossible > to observe null in them, I think (correct me if I'm wrong; it seems like you

Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll

2021-01-08 Thread Peter Levart
On Fri, 8 Jan 2021 11:27:57 GMT, Peter Levart wrote: >> @forax, @plevart could I ask for review once again? > > Hi @stsypanov, > >> The **behavior** of this convenience method is **identical** to that of >> c.addAll(Arrays.asList(elements)) > >

Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll

2021-01-08 Thread Peter Levart
On Fri, 8 Jan 2021 09:37:23 GMT, Сергей Цыпанов wrote: >> Apart from the @SuppressWarnings, this looks good to me. >> And i like the irony of this. > > @forax, @plevart could I ask for review once again? Hi @stsypanov, > The **behavior** of this convenience method is **identical** to that of

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

2021-01-08 Thread Peter Levart
On Fri, 8 Jan 2021 08:49:44 GMT, Attila Szegedi wrote: >> Hi Attila, >> >> This looks good. >> >> If I understand correctly, your `BiClassValue` is a holder for a >> `BiFunction` and a `BiClassValueRoot` which is a >> `ClassValue`. The `BiClassValues` contains two lazily >> constructed

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

2021-01-08 Thread Peter Levart
On Fri, 8 Jan 2021 08:53:19 GMT, Attila Szegedi wrote: >> _(NB: For this leak to occur, an application needs to be either creating and >> discarding linkers frequently, or creating and discarding class loaders >> frequently while creating Dynalink type converters for classes in them. >>

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

2021-01-08 Thread Peter Levart
On Fri, 8 Jan 2021 08:53:19 GMT, Attila Szegedi wrote: >> _(NB: For this leak to occur, an application needs to be either creating and >> discarding linkers frequently, or creating and discarding class loaders >> frequently while creating Dynalink type converters for classes in them. >>

Re: RFR: 8258588: MD5 MessageDigest in java.util.UUID should be cached

2021-01-07 Thread Peter Levart
On Sun, 20 Dec 2020 22:41:33 GMT, PROgrm_JARvis wrote: >>> I have to say that introducing a ThreadLocal here seems like a step in the >>> wrong direction. With a ThreadLocal, if I read this correctly, a >>> MessageDigest will be cached with each thread that ever calls this API, and >>> it

Integrated: 8259021: SharedSecrets should avoid double racy reads from non-volatile fields

2021-01-05 Thread Peter Levart
On Thu, 31 Dec 2020 10:02:01 GMT, Peter Levart wrote: > See: https://bugs.openjdk.java.net/browse/JDK-8259021 > See also discussion in thread: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-December/072798.html This pull request has now been integrated. Changeset:

Re: Is SharedSecrets thread-safe?

2021-01-04 Thread Peter Levart
ss is loading `Introspector` (which sets access object) anyways it might be saner to have this initialization logic in `SharedSecrets` instead. Kind regards Peter Levart hat am 31. Dezember 2020 um 11:07 geschrieben: On 12/31/20 2:30 AM, Hans Boehm wrote: It sounds as though this wou

Re: RFR: 8259021: SharedSecrets should avoid double racy reads from non-volatile fields [v2]

2021-01-04 Thread Peter Levart
On Mon, 4 Jan 2021 15:57:33 GMT, Richard Reingruber wrote: >> The bug title and the PR title need to be the same. >> Editing either one is fine. > > But wouldn't it be legal for a compiler (java to bytecode or bytecode to > machinecode) to replace references of my_local_copy with references to >

Re: RFR: 8259021 avoid double racy reads from non-volatile fields of SharedSecrets [v2]

2021-01-04 Thread Peter Levart
> See: https://bugs.openjdk.java.net/browse/JDK-8259021 > See also discussion in thread: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-December/072798.html Peter Levart has updated the pull request incrementally with one additional commit since the last revision:

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v3]

2021-01-04 Thread Peter Levart
On Sat, 2 Jan 2021 16:28:19 GMT, Johannes Kuhn wrote: >> Attila Szegedi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> jtreg runs with assertions on, so using the assert keyword is sufficient > > Just a small suggestion using

Re: Is SharedSecrets thread-safe?

2020-12-31 Thread Peter Levart
On 12/31/20 2:30 AM, Hans Boehm wrote: It sounds as though this would be correct if if (static_field == null) { initialize(); } return static_field; ``` were rewritten as Foo my_local_copy = static_field; if (my_copy == null) { initialize(); my_local_copy = static_field; } return

RFR: 8259021 avoid double racy reads from non-volatile fields of SharedSecrets

2020-12-31 Thread Peter Levart
See: https://bugs.openjdk.java.net/browse/JDK-8259021 See also discussion in thread: https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-December/072798.html - Commit messages: - 8259021 avoid double racy reads from non-volatile fields of SharedSecrets Changes:

Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll

2020-12-29 Thread Peter Levart
On Tue, 29 Dec 2020 10:42:18 GMT, Peter Levart wrote: >> Hi, >> Sorry for joining late to this discussion, but I think that changing this >> method to delegate to c.addAll(Arrays.asList(...)) might not be the best >> thing to do here. Why? >> - This migh

Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll

2020-12-29 Thread Peter Levart
On Tue, 29 Dec 2020 10:21:07 GMT, Peter Levart wrote: >> "This message" referred to the entirety of that very comment of mine >> https://github.com/openjdk/jdk/pull/1764#issuecomment-748926986 >> >> I prepended that message with that clause (that is, the wo

Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll

2020-12-29 Thread Peter Levart
On Mon, 21 Dec 2020 15:23:06 GMT, Pavel Rappo wrote: >> @pavelrappo also see this not very old comment: >> https://github.com/spring-projects/spring-framework/pull/24636#pullrequestreview-370684078 > > "This message" referred to the entirety of that very comment of mine >

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

2020-12-04 Thread Peter Levart
On Fri, 4 Dec 2020 20:16:14 GMT, Mandy Chung wrote: >> src/java.management/share/classes/com/sun/jmx/mbeanserver/WeakIdentityHashMap.java >> line 127: >> >>> 125: T got = get(); >>> 126: return got != null && wr.refersTo(got); >>> 127: } >> >> Here you could

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

2020-12-04 Thread Peter Levart
On Fri, 4 Dec 2020 20:09:01 GMT, Mandy Chung wrote: >> src/java.base/share/classes/java/util/WeakHashMap.java line 293: >> >>> 291: // then checks for equality >>> 292: Object k = e.get(); >>> 293: return key == k || key.equals(k); >> >> I think `key == k` is already

<    1   2   3   4   5   6   7   8   9   10   >