RE: RFR: 7147994 Hashtable rehash() javadoc describes implementation details
Stuart, Thanks for all the details.
All you have said makes sense to me, I have no contention for closing this
issue as Won't Fix (am not related to originator, picked this issue up as
starters to understand the contribution process)
Regards!
Jay
-Stuart Marks wrote: -
To: Jayashree Sk1
From: Stuart Marks
Date: 04/30/2020 09:53AM
Cc: core-libs-dev@openjdk.java.net
Subject: [EXTERNAL] Re: RFR: 7147994 Hashtable rehash() javadoc describes
implementation details
The bug report states that this method specification describes implementation
details, with the implication that implementation details should be avoided and
that abstract specifications (contracts or invariants) should be provided
instead. The alternative wording from the bug report removes the implementation
details and replaces them with some informative text.
However, looking more closely about this change, I think it's wrong.
This is a protected method, so it can be overridden or called by a subclass. As
such, the method specification should provide information necessary for
subclasses to be implemented correctly, in particular, about "self-use" of the
overridable methods from other parts of the superclass implementation. (See
Bloch, Effective Java 3/e, Item 19. See also AbstractCollection.removeAll() for
an example of this in practice.)
I think the bug report is wrong because it suggests removing implementation
details, when in fact this is a place where implementation details ought to be
provided.
(One might argue that more implementation details should be provided here, but
it's not clear that Hashtable was actually designed for subclassing. That said,
it can be subclassed, and there are surely many subclasses out there relying on
all kinds of behavior of Hashtable. It's probably not worth trying to document
all of it though.)
So, I'm inclined to close this issue as Won't Fix.
s'marks
On 4/29/20 3:02 AM, Jayashree Sk1 wrote:
> Hi All,
> Please find the below changes for the issues
> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D7147994=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k=bNY044TQSELJWMafSRxC-62CyHMVnMxboXMG0qqU6CU=V9rN7MCO4rArlV8tRjEn44_Kl_tCH2h-xjqakCvpBxY=
>
> It is a description change, which was already approved by the reviewer.
>
> Thanks!
>
> diff -r 59b5bd9a7168 src/java.base/share/classes/java/util/Hashtable.java
> --- a/src/java.base/share/classes/java/util/Hashtable.java Mon Mar 16
> 02:16:49 2020 -0400
> +++ b/src/java.base/share/classes/java/util/Hashtable.java Mon Mar 30
> 15:45:43 2020 +0530
> @@ -399,9 +399,9 @@
> /**
>* Increases the capacity of and internally reorganizes this
>* hashtable, in order to accommodate and access its entries more
> - * efficiently. This method is called automatically when the
> - * number of keys in the hashtable exceeds this hashtable's capacity
> - * and load factor.
> + * efficiently. This method is called to increase the capacity of and
> + * internally reorganize this hashtable in order to more efficiently
> + * accommodate and access its entries.
>*/
> @SuppressWarnings("unchecked")
> protected void rehash() {
>
RE: RFR: 6415694 Clarification in Javadoc for java.rmi.AlreadyBoundException
Thanks for the review comment Stuart. Yes, I will need a sponsor, this is my first time here in OpenJDK. Regards! Jay -Stuart Marks wrote: - To: Jayashree Sk1 From: Stuart Marks Date: 04/30/2020 09:22AM Cc: core-libs-dev@openjdk.java.net Subject: [EXTERNAL] Re: RFR: 6415694 Clarification in Javadoc for java.rmi.AlreadyBoundException The change looks fine. Although this is in a normative portion of the specification, the nature of the change is purely editorial, so I don't think it needs a CSR. Do you need a sponsor? s'marks On 4/29/20 2:57 AM, Jayashree Sk1 wrote: > Hi All, > Please find the below changes for the issues > https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D6415694=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k=OXEC-w1xTWoJUyw_sRivRwIGmkHIUc3DllMsA_N3qIk=iesFFcE4NSOreLmf8vwdKzcdnFgjGE_SEERICPUFef4= > . > It is a description change, which was already approved by the reviewer. > > Thanks! > > diff -r 59b5bd9a7168 > src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java > --- a/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon > Mar 16 02:16:49 2020 -0400 > +++ b/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon > Mar 30 15:45:43 2020 +0530 > @@ -26,8 +26,8 @@ > > /** >* An AlreadyBoundException is thrown if an attempt > - * is made to bind an object in the registry to a name that already > - * has an associated binding. > + * is made to bind an object to a name that already > + * has an associated binding in the registry. >* >* @since 1.1 >* @author Ann Wollrath >
Re: RFR: 7147994 Hashtable rehash() javadoc describes implementation details
The bug report states that this method specification describes implementation
details, with the implication that implementation details should be avoided and
that abstract specifications (contracts or invariants) should be provided
instead. The alternative wording from the bug report removes the implementation
details and replaces them with some informative text.
However, looking more closely about this change, I think it's wrong.
This is a protected method, so it can be overridden or called by a subclass. As
such, the method specification should provide information necessary for
subclasses to be implemented correctly, in particular, about "self-use" of the
overridable methods from other parts of the superclass implementation. (See
Bloch, Effective Java 3/e, Item 19. See also AbstractCollection.removeAll() for
an example of this in practice.)
I think the bug report is wrong because it suggests removing implementation
details, when in fact this is a place where implementation details ought to be
provided.
(One might argue that more implementation details should be provided here, but
it's not clear that Hashtable was actually designed for subclassing. That said,
it can be subclassed, and there are surely many subclasses out there relying on
all kinds of behavior of Hashtable. It's probably not worth trying to document
all of it though.)
So, I'm inclined to close this issue as Won't Fix.
s'marks
On 4/29/20 3:02 AM, Jayashree Sk1 wrote:
Hi All,
Please find the below changes for the issues
https://bugs.openjdk.java.net/browse/JDK-7147994
It is a description change, which was already approved by the reviewer.
Thanks!
diff -r 59b5bd9a7168 src/java.base/share/classes/java/util/Hashtable.java
--- a/src/java.base/share/classes/java/util/Hashtable.java Mon Mar 16
02:16:49 2020 -0400
+++ b/src/java.base/share/classes/java/util/Hashtable.java Mon Mar 30
15:45:43 2020 +0530
@@ -399,9 +399,9 @@
/**
* Increases the capacity of and internally reorganizes this
* hashtable, in order to accommodate and access its entries more
- * efficiently. This method is called automatically when the
- * number of keys in the hashtable exceeds this hashtable's capacity
- * and load factor.
+ * efficiently. This method is called to increase the capacity of and
+ * internally reorganize this hashtable in order to more efficiently
+ * accommodate and access its entries.
*/
@SuppressWarnings("unchecked")
protected void rehash() {
Re: RFR: 6415694 Clarification in Javadoc for java.rmi.AlreadyBoundException
The change looks fine. Although this is in a normative portion of the specification, the nature of the change is purely editorial, so I don't think it needs a CSR. Do you need a sponsor? s'marks On 4/29/20 2:57 AM, Jayashree Sk1 wrote: Hi All, Please find the below changes for the issues https://bugs.openjdk.java.net/browse/JDK-6415694. It is a description change, which was already approved by the reviewer. Thanks! diff -r 59b5bd9a7168 src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java --- a/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon Mar 16 02:16:49 2020 -0400 +++ b/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon Mar 30 15:45:43 2020 +0530 @@ -26,8 +26,8 @@ /** * An AlreadyBoundException is thrown if an attempt - * is made to bind an object in the registry to a name that already - * has an associated binding. + * is made to bind an object to a name that already + * has an associated binding in the registry. * * @since 1.1 * @author Ann Wollrath
Re: Collections.synchronizedXXX() and internal mutex (aka SyncRoot)
Hi Dmytro,
Callers of an API performing explicit synchronization, along with the
synchronized collections wrappers, have mostly fallen into disuse since the
introduction of the java.util.concurrent collections.
Multiple threads can either interact directly on a concurrent collection, or the
developer can provide an intermediate object (not a collection) that does
internal locking, and that exports the right set of thread-safe APIs to callers.
I'm thus skeptical of the utility of enhancing these wrapper classes with
additional APIs.
Do you have a use case that's difficult to handle any other way?
s'marks
On 4/29/20 12:58 AM, dmytro sheyko wrote:
Hello,
Have you ever discussed to make field mutex in synchronized collections
accessible?
Javadoc for Collections#synchronizedSortedSet suggest to iterate collection
this way:
SortedSet s = Collections.synchronizedSortedSet(new TreeSet());
SortedSet s2 = s.headSet(foo);
...
synchronized (s) { // Note: s, not s2!!!
Iterator i = s2.iterator(); // Must be in the synchronized block
while (i.hasNext())
foo(i.next());
}
I.e. in order to iterate subset, we also need a reference to the whole set,
which is not really convenient. How about to make it possible to write:
SortedSet s2 = s.headSet(foo);
...
synchronized (Collections.getSyncRoot(s2)) { // Note:
Collections.getSyncRoot(s2)
Iterator i = s2.iterator(); // Must be in the synchronized block
while (i.hasNext())
foo(i.next());
}
Also I think it would be convenient to let to provide custom sync root when
synchronized collection is created.
E.g.
Object customSyncRoot = new Object();
SortedSet s = Collections.synchronizedSortedSet(new TreeSet(),
customSyncRoot);
What do you think about this?
Regards,
Dmytro
Re: [15] RFR: 8244152: Remove unnecessary hash map resize in LocaleProviderAdapters
LGTM Vyom On Thu, Apr 30, 2020 at 3:50 AM wrote: > Hello, > > Please review this small fix to the following issue: > > https://bugs.openjdk.java.net/browse/JDK-8244152 > > The proposed changeset is located at: > > https://cr.openjdk.java.net/~naoto/8244152/webrev.00/ > > The hash map used there didn't have initial capacity, even though the > exact numbers are known. > > Naoto > -- Thanks, Vyom
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
On 4/29/20 10:23 PM, Maurizio Cimadamore wrote: Many thanks Peter, my preference would be for adding the benchmark now, and come back to fix this post integration. The MemoryScope code is finicky and getting confidence that the code is race-free takes time and I'd prefer not to change that during the course of this RFR. Your approach seems promising, so let's keep working and thinking on it on the side (and perhaps let's maybe go for it in the panama repo first, to make sure we don't add regressions). Sounds like a plan? I agree. There's still plenty of time until 15 ships... Regards, Peter Cheers Maurizio On 29/04/2020 20:19, Peter Levart wrote: Hi Maurizio, On 4/29/20 2:41 AM, Maurizio Cimadamore wrote: The current implementation has performances that are on par with the previous acquire-based implementation, and also on par with what can be achieved with Unsafe. We do have a micro benchmark in the patch (see ParallelSum (**)) which tests this, and I get _identical_ numbers even if I _comment_ the body of acquire/release - so that no contention can happen; so, I'm a bit skeptical overall that contention on acquire/release is the main factor at play here - but perhaps we need more targeted benchmarks. So I modified your benchmark (just took out the relevant parts) and added some benchmarks that exhibit Stream.findAny() and Stream.findFirst(). As I anticipated, the results for parallel stream variants were slowing the benchmark down, so I had to reduce the number of elements by a factor of 16 to get results in reasonable time: http://cr.openjdk.java.net/~plevart/jdk-dev/8243491_MemoryScope/ParallelSum.java I then swapped-in the alternative implementation of MemoryScope (note that this is not a whole implementation - the dup() method is missing): http://cr.openjdk.java.net/~plevart/jdk-dev/8243491_MemoryScope/MemoryScope.java ...and I got these results: i7 2600K (4 cores / 8 threads) with proposed MemoryScope: Benchmark Mode Cnt Score Error Units ParallelSum.find_any_stream_parallel avgt 10 1332.687 ± 733.535 ms/op ParallelSum.find_any_stream_serial avgt 10 440.260 ± 3.110 ms/op ParallelSum.find_first_loop_serial avgt 10 5.809 ± 0.044 ms/op ParallelSum.find_first_stream_parallel avgt 10 2070.318 ± 41.072 ms/op ParallelSum.find_first_stream_serial avgt 10 440.034 ± 4.672 ms/op ParallelSum.sum_loop_serial avgt 10 5.647 ± 0.055 ms/op ParallelSum.sum_stream_parallel avgt 10 5.314 ± 0.294 ms/op ParallelSum.sum_stream_serial avgt 10 19.179 ± 0.136 ms/op with alternative MemoryScope: Benchmark Mode Cnt Score Error Units ParallelSum.find_any_stream_parallel avgt 10 80.280 ± 13.183 ms/op ParallelSum.find_any_stream_serial avgt 10 317.388 ± 2.787 ms/op ParallelSum.find_first_loop_serial avgt 10 5.790 ± 0.038 ms/op ParallelSum.find_first_stream_parallel avgt 10 117.925 ± 1.747 ms/op ParallelSum.find_first_stream_serial avgt 10 315.076 ± 5.725 ms/op ParallelSum.sum_loop_serial avgt 10 5.652 ± 0.042 ms/op ParallelSum.sum_stream_parallel avgt 10 4.881 ± 0.053 ms/op ParallelSum.sum_stream_serial avgt 10 19.143 ± 0.035 ms/op So here it is. The proof that contention does occur. Regards, Peter
Re: [15] RFR: 8244152: Remove unnecessary hash map resize in LocaleProviderAdapters
+1 -Joe On 4/29/2020 3:18 PM, naoto.s...@oracle.com wrote: Hello, Please review this small fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244152 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8244152/webrev.00/ The hash map used there didn't have initial capacity, even though the exact numbers are known. Naoto
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
On 4/29/20 11:16 PM, Maurizio Cimadamore wrote: On 29/04/2020 21:40, Maurizio Cimadamore wrote: On 29/04/2020 08:10, Peter Levart wrote: Right, as you saw in a private Email, I did exactly that in a revised version (posted below). The spurious ISE may happen but only when close is called prematurely relative to all child scope close(s) that were or are still active. So we could say the other way: if close was not called prematurely, the ISE on acquire would not be spurious - so ordering of close relative to later acquire was wrong anyway and the exception is an "indication" of that wrong ordering Unless we want to use close() to "probe" the scope whether it is still active or not, I don't think this should present a problem. One quick comment: unless I'm missing something, this is starting to make a pretty strong assumption on how acquire()/close() are going to be used. Yes, if acquire() is not exposed to developers (as in the current API) and only hidden behind a spliterator, we might get away with this; but should we at some point re-introduce some kind of explicit acquire mechanism in the API, then such an assumption would not be a fine one to make. Actually, the more I think of it, the less I'm convinced that, even in the restricted case of acquire() that the API has now, the proposed implementation is correct. A client has a segment, and it creates a spliterator for it. Then it gives the spliterator to some thread pool which will happily work away with the segment. But, the client code prematurely closes the segment - this operation should fail with exception, as per javadoc, if there are other actors working on the segment. Your implementation does that, but that failure leaves a sneaky side-effect - in that the threads in the thread-pool might not be able to continue their work on the segment. Think differently: what if the client succeeded in closing the segment, just because it did it in a time window when no thread in the thread pool held an open scope (this is entirely possible with parallel stream for example since threads periodically acquire and close scopes). This would have the same effect on threads in the thread pool - they would not be able to continue their work... What I'm trying to say is that this is just a mechanism to make things safe, not to coordinate work. If program wants to avoid trouble, it must carefully coordinate work of threads. This action-at-a-distance between a failed close and a pending acquire is not documented anywhere, and I also find it very counter-intuitive. So, while I agree there might be ways to have a more scalable scope implementation, I think this is a problem that needs to be addressed. If we were willing to change the spec a bit, I would then be more inclined to say that when you call MemorySegment::close you always close - period; under no circumstances is an exception thrown. If there are pending acquires on the segment, we keep spinning until there aren't, and then we close for good. I think that would be a more stable semantics, rather than one where it seems like the operation failed, but in reality, it succeeded in part ;-) (at least for a period of time) Well, you might not agree, but I don't think of this as a problem. You are trying to define some "correct" behavior to a program that is incorrectly written anyway. I'm just saying that if the program is correctly written, then there is no exceptions. And not defining the behavior for incorrect programs is not that uncommon. Just think of Java memory model. How much of a program behavior is guaranteed when program has data races? Regards, Peter Maurizio Maurizio
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
On 4/29/20 2:30 PM, fo...@univ-mlv.fr wrote: I think the problem with perf might be caused by the fact that while the array is now a constant, the elements are not (the array is mutable after all). For fields you can fix this by using @Stable, but not for CP entries :) I think you're right, Ah, I missed that! I think what could work is; rather than ldc'ing the array, and then looking up the values with 'normal' Java code, we could have another dynamic constant that does the the array lookup as well. Then the resolved value is stored in a separate CP slot as a true constant. We probably want to have a bootstrap method in ConstantBootstraps that can do an arbitrary array lookup given an array and an index for that. FYI. I'm exploring is `classDataAt` to get a specific element/entry from a class data of immutable List or Map. [1] http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8239578/webrev.00/src/java.base/share/classes/java/lang/invoke/MethodHandles.java.frames.html Not going down this road, sorry :-) I've added the changes (see attached patch), and all benchmarks are several order of magnitude slower. I think this is mostly caused by the fact that the addOffset/multiplyOffset handle are no longer cached in static constants. While I understand that there might be better ways to generate the code, I'd strongly prefer to leave the code as per last iteration. I can't honestly see in which way having 3-4 static fields in a synthetic VarHandle class is going to hurt (but I can see how much it hurts by not having said fields). I agree to keep the code per last iteration. We can always improve this in the future with performance measurement. Mandy
Re: Java 11 vs 14 MethodHandle behavior change
On 4/29/20 2:07 AM, Simone Bordet wrote:
Did you get any exception in 14? Is it from findVirtual or from in?
From findVirtual():
Exception in thread "main" java.lang.IllegalAccessException: no such
method: org.module1.MyEndPoint.onMessage(MyString)void/invokeVirtual
at
java.base/java.lang.invoke.MemberName.makeAccessException(MemberName.java:971)
at
java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1114)
at
java.base/java.lang.invoke.MethodHandles$Lookup.resolveOrFail(MethodHandles.java:2785)
at
java.base/java.lang.invoke.MethodHandles$Lookup.findVirtual(MethodHandles.java:1883)
at org.module2.Main.main(Main.java:28)
Caused by: java.lang.LinkageError: loader constraint violation: when
resolving method 'void
org.module1.MyEndPoint.onMessage(org.module1.MyString)' the class
loader 'bootstrap' of the current class, java/lang/Object, and the
class loader java.net.URLClassLoader @7291c18f for the method's
defining class, org/module1/MyEndPoint, have different Class objects
for the type org/module1/MyString used in the signature
(java.lang.Object is in module java.base of loader 'bootstrap';
org.module1.MyEndPoint is in unnamed module of loader
java.net.URLClassLoader @7291c18f, parent loader 'app')
at java.base/java.lang.invoke.MethodHandleNatives.resolve(Native Method)
at
java.base/java.lang.invoke.MemberName$Factory.resolve(MemberName.java:1084)
at
java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:)
... 3 more
Thanks for the reproducer.
`publicLookup().in(endPointClass1)` in JDK 13 produces a Lookup object
on endPointClass1 with PUBLIC access only. UNCONDITIONAL bit was lost.
The resulting Lookup can access classes unconditionally exported from
module M that the module of endPointClass1 can read. The resulting
Lookup is no longer a public lookup, i.e. the lookup context is the
lookup class (i.e. endPointClass1) and the lookup mode (i.e. PUBLIC).
`publicLookup().in(endPointClass1)` in JDK 14 produces a Lookup object
on endPointClass1 with UNCONDITIONAL bit due to the spec change for
JDK-8173978. The resulting Lookup remains to be a public lookup which
can access any unconditionally exported classes from any module.
Although the lookup class of the teleported public Lookup is
endPointClass1, it should not affect the lookup context. Unfortunately,
the workaround for JDK-8228671 causes this bug.
The reproducer shows that the new public Lookup uses Object as the
lookup clsas that violates the loader constraint. lookup1 finds
"org.module1.MyEndPoint" loaded by CL1 and it adds a loader constraint
with the boot loader (rather than CL1) due to the workaround fixed by
JDK-8228671 that uses Object as the lookup class. When lookup2 finds
"org.module1.MyEndPoint" loaded by CL2, the lookup fails with loader
constraint violation.
The current implementation already does that.
private Class lookupClassOrNull() {
if (allowedModes == TRUSTED) {
return null;
}
if (allowedModes == UNCONDITIONAL) {
// use Object as the caller to pass to VM doing resolution
return Object.class;
}
return lookupClass;
}
What exactly have you changed?
if (allowedModes == UNCONDITIONAL) {
return lookupClass;
}
This patch will hit the assertion that JDK-8228671 ran into. We need a
long-term fix (perhaps to look into JDK-8173977)
https://bugs.openjdk.java.net/browse/JDK-8244090
Thanks. I will look into it.
Mandy
[15] RFR: 8244152: Remove unnecessary hash map resize in LocaleProviderAdapters
Hello, Please review this small fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244152 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8244152/webrev.00/ The hash map used there didn't have initial capacity, even though the exact numbers are known. Naoto
Re: Collections.synchronizedXXX() and internal mutex (aka SyncRoot)
Background on this can be found here:
https://bugs.openjdk.java.net/browse/JDK-4335520
Jason
From: core-libs-dev on behalf of
dmytro sheyko
Sent: Wednesday, April 29, 2020 2:58 AM
To: core-libs-dev
Subject: Collections.synchronizedXXX() and internal mutex (aka SyncRoot)
Hello,
Have you ever discussed to make field mutex in synchronized collections
accessible?
Javadoc for Collections#synchronizedSortedSet suggest to iterate collection
this way:
SortedSet s = Collections.synchronizedSortedSet(new TreeSet());
SortedSet s2 = s.headSet(foo);
...
synchronized (s) { // Note: s, not s2!!!
Iterator i = s2.iterator(); // Must be in the synchronized block
while (i.hasNext())
foo(i.next());
}
I.e. in order to iterate subset, we also need a reference to the whole set,
which is not really convenient. How about to make it possible to write:
SortedSet s2 = s.headSet(foo);
...
synchronized (Collections.getSyncRoot(s2)) { // Note:
Collections.getSyncRoot(s2)
Iterator i = s2.iterator(); // Must be in the synchronized block
while (i.hasNext())
foo(i.next());
}
Also I think it would be convenient to let to provide custom sync root when
synchronized collection is created.
E.g.
Object customSyncRoot = new Object();
SortedSet s = Collections.synchronizedSortedSet(new TreeSet(),
customSyncRoot);
What do you think about this?
Regards,
Dmytro
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
- Mail original - > De: "Jorn Vernee" > À: "Maurizio Cimadamore" , "mandy chung" > , "Remi Forax" > > Cc: "core-libs-dev" > Envoyé: Mercredi 29 Avril 2020 22:09:47 > Objet: Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second > Incubator) > Hi, Hi Jorn, > > I think the problem with perf might be caused by the fact that while the > array is now a constant, the elements are not (the array is mutable > after all). For fields you can fix this by using @Stable, but not for CP > entries :) I think you're right, > > I think what could work is; rather than ldc'ing the array, and then > looking up the values with 'normal' Java code, we could have another > dynamic constant that does the the array lookup as well. Then the > resolved value is stored in a separate CP slot as a true constant. We > probably want to have a bootstrap method in ConstantBootstraps that can > do an arbitrary array lookup given an array and an index for that. > > Given the amount of work, I'd say definitely something that should be > saved for another time, also since there doesn't appear to be a major > payoff for doing that at the moment. I think it's either to have a small class instead of an array with all the fields marked as @Stable, this will also avoid the cast because the fields will have the right type. > > Jorn Rémi > > On 29/04/2020 03:13, Maurizio Cimadamore wrote: >> >> On 28/04/2020 21:44, Maurizio Cimadamore wrote: >>> >>> On 28/04/2020 19:09, Mandy Chung wrote: On 4/28/20 12:58 AM, fo...@univ-mlv.fr wrote: > > > > > I don't think you need to store all the values into static > fields, you can directly do a ldc + aaload with the right index > right where you need it, > > > I think this is what you are thinking as reported in JDK-8243492: > http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8239578/webrev.01-accessor/ > > > > if the accessors are declared ACC_STATIC, yes ! > Thanks for catching this and this way will not be hit JDK-824349. Here is the revised patch: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8239578/webrev.02/ Maurizio - do you mind remerge MemoryAccessVarHandleGenerator.java with webrev.02? >>> >>> I'll take care of that >> >> Not going down this road, sorry :-) >> >> I've added the changes (see attached patch), and all benchmarks are >> several order of magnitude slower. I think this is mostly caused by >> the fact that the addOffset/multiplyOffset handle are no longer >> cached in static constants. >> >> While I understand that there might be better ways to generate the >> code, I'd strongly prefer to leave the code as per last iteration. I >> can't honestly see in which way having 3-4 static fields in a >> synthetic VarHandle class is going to hurt (but I can see how much it >> hurts by not having said fields). >> >> Cheers >> Maurizio >> >>> >>> Maurizio >>> thanks > >>> Mandy
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
- Mail original - > De: "Maurizio Cimadamore" > À: "mandy chung" , "Remi Forax" > Cc: "core-libs-dev" > Envoyé: Mercredi 29 Avril 2020 03:13:35 > Objet: Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second > Incubator) > On 28/04/2020 21:44, Maurizio Cimadamore wrote: >> >> On 28/04/2020 19:09, Mandy Chung wrote: >>> On 4/28/20 12:58 AM, fo...@univ-mlv.fr wrote: I don't think you need to store all the values into static fields, you can directly do a ldc + aaload with the right index right where you need it, I think this is what you are thinking as reported in JDK-8243492: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8239578/webrev.01-accessor/ if the accessors are declared ACC_STATIC, yes ! >>> >>> Thanks for catching this and this way will not be hit JDK-824349. >>> >>> Here is the revised patch: >>> http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8239578/webrev.02/ >>> >>> Maurizio - do you mind remerge MemoryAccessVarHandleGenerator.java >>> with webrev.02? >> >> I'll take care of that > > Not going down this road, sorry :-) > > I've added the changes (see attached patch), and all benchmarks are > several order of magnitude slower. I think this is mostly caused by the > fact that the addOffset/multiplyOffset handle are no longer cached in > static constants. > > While I understand that there might be better ways to generate the code, > I'd strongly prefer to leave the code as per last iteration. I can't > honestly see in which way having 3-4 static fields in a synthetic > VarHandle class is going to hurt (but I can see how much it hurts by not > having said fields). I somehow miss that email, ok, let's back to the black board :) Rémi > > Cheers > Maurizio > >> >> Maurizio >> >>> >>> thanks > >> Mandy
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
On 29/04/2020 21:40, Maurizio Cimadamore wrote: On 29/04/2020 08:10, Peter Levart wrote: Right, as you saw in a private Email, I did exactly that in a revised version (posted below). The spurious ISE may happen but only when close is called prematurely relative to all child scope close(s) that were or are still active. So we could say the other way: if close was not called prematurely, the ISE on acquire would not be spurious - so ordering of close relative to later acquire was wrong anyway and the exception is an "indication" of that wrong ordering Unless we want to use close() to "probe" the scope whether it is still active or not, I don't think this should present a problem. One quick comment: unless I'm missing something, this is starting to make a pretty strong assumption on how acquire()/close() are going to be used. Yes, if acquire() is not exposed to developers (as in the current API) and only hidden behind a spliterator, we might get away with this; but should we at some point re-introduce some kind of explicit acquire mechanism in the API, then such an assumption would not be a fine one to make. Actually, the more I think of it, the less I'm convinced that, even in the restricted case of acquire() that the API has now, the proposed implementation is correct. A client has a segment, and it creates a spliterator for it. Then it gives the spliterator to some thread pool which will happily work away with the segment. But, the client code prematurely closes the segment - this operation should fail with exception, as per javadoc, if there are other actors working on the segment. Your implementation does that, but that failure leaves a sneaky side-effect - in that the threads in the thread-pool might not be able to continue their work on the segment. This action-at-a-distance between a failed close and a pending acquire is not documented anywhere, and I also find it very counter-intuitive. So, while I agree there might be ways to have a more scalable scope implementation, I think this is a problem that needs to be addressed. If we were willing to change the spec a bit, I would then be more inclined to say that when you call MemorySegment::close you always close - period; under no circumstances is an exception thrown. If there are pending acquires on the segment, we keep spinning until there aren't, and then we close for good. I think that would be a more stable semantics, rather than one where it seems like the operation failed, but in reality, it succeeded in part ;-) (at least for a period of time) Maurizio Maurizio
Re: RFR: JDK-8219536: Add Option for user defined jlink options
Looks good.
- Alexey
On 4/29/2020 2:36 PM, Andy Herrick wrote:
I don't think I sent out webrev.5 [6] fixing Alexander's points below.
Please Review:
[6] http://cr.openjdk.java.net/~herrick/8219536/webrev.05/index.html
/Andy
On 4/23/2020 7:59 PM, Alexander Matveev wrote:
Hi Andy,
http://cr.openjdk.java.net/~herrick/8219536/webrev.04/src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/internal/Arguments.java.frames.html
1) Copyright year needs to be updated. Other files also needs
copyright year to be updated.
2) Line 778: Not sure why it was moved to single line. I think it is
better to revert it back.
Otherwise looks fine.
Thanks,
Alexander
On 4/23/20 1:48 PM, Andy Herrick wrote:
Please review updated webrev at [5] to address comments below from
Alexey.
[5] http://cr.openjdk.java.net/~herrick/8219536/webrev.04
/Andy
On 4/23/2020 11:17 AM, Alexey Semenyuk wrote:
http://cr.openjdk.java.net/~herrick/8219536/webrev.03/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JPackageCommand.java.sdiff.html:731
- 'launcherName' parameter of readRuntimeReleaseFile() function
seems to be not used.
http://cr.openjdk.java.net/~herrick/8219536/webrev.03/test/jdk/tools/jpackage/share/jdk/jpackage/tests/JLinkOptionsTest.java.html:
copyright year should be 2020.
http://cr.openjdk.java.net/~herrick/8219536/webrev.03/test/jdk/tools/jpackage/share/jdk/jpackage/tests/JLinkOptionsTest.java.html:
128 - There is no point to assert if 'cfgfile' is not null. It
should be always non null for application packaging or
readLaunherCfgFile.
For testing if string contains some values, I'd suggest to use
TKit.assertTextStream():
---
List mods = List.of(release.get(1));
if (required != null) {
for (String s : required) {
TKit.assertTextStream(s).label("mods").apply(mods.stream());
}
}
if (prohibited != null) {
for (String s : prohibited) {
TKit.assertTextStream(s).label("mods").negate().apply(mods.stream());
}
}
---
Will save you from maintaining explicit log messages.
- Alexey
On 4/23/2020 10:08 AM, Andy Herrick wrote:
Please review webrev at [1] to address issue [2].
This is the new feature to add the jpackage option --jlink-options
as specified in CSR at [3]
/Andy
[1] http://cr.openjdk.java.net/~herrick/8219536/webrev.03
[2] https://bugs.openjdk.java.net/browse/JDK-8219536
[3] https://bugs.openjdk.java.net/browse/JDK-8243272
Re: RFR: JDK-8219536: Add Option for user defined jlink options
Hi Andy,
Looks fine.
Thanks,
Alexander
On 4/29/20 11:36 AM, Andy Herrick wrote:
I don't think I sent out webrev.5 [6] fixing Alexander's points below.
Please Review:
[6] http://cr.openjdk.java.net/~herrick/8219536/webrev.05/index.html
/Andy
On 4/23/2020 7:59 PM, Alexander Matveev wrote:
Hi Andy,
http://cr.openjdk.java.net/~herrick/8219536/webrev.04/src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/internal/Arguments.java.frames.html
1) Copyright year needs to be updated. Other files also needs
copyright year to be updated.
2) Line 778: Not sure why it was moved to single line. I think it is
better to revert it back.
Otherwise looks fine.
Thanks,
Alexander
On 4/23/20 1:48 PM, Andy Herrick wrote:
Please review updated webrev at [5] to address comments below from
Alexey.
[5] http://cr.openjdk.java.net/~herrick/8219536/webrev.04
/Andy
On 4/23/2020 11:17 AM, Alexey Semenyuk wrote:
http://cr.openjdk.java.net/~herrick/8219536/webrev.03/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JPackageCommand.java.sdiff.html:731
- 'launcherName' parameter of readRuntimeReleaseFile() function
seems to be not used.
http://cr.openjdk.java.net/~herrick/8219536/webrev.03/test/jdk/tools/jpackage/share/jdk/jpackage/tests/JLinkOptionsTest.java.html:
copyright year should be 2020.
http://cr.openjdk.java.net/~herrick/8219536/webrev.03/test/jdk/tools/jpackage/share/jdk/jpackage/tests/JLinkOptionsTest.java.html:
128 - There is no point to assert if 'cfgfile' is not null. It
should be always non null for application packaging or
readLaunherCfgFile.
For testing if string contains some values, I'd suggest to use
TKit.assertTextStream():
---
List mods = List.of(release.get(1));
if (required != null) {
for (String s : required) {
TKit.assertTextStream(s).label("mods").apply(mods.stream());
}
}
if (prohibited != null) {
for (String s : prohibited) {
TKit.assertTextStream(s).label("mods").negate().apply(mods.stream());
}
}
---
Will save you from maintaining explicit log messages.
- Alexey
On 4/23/2020 10:08 AM, Andy Herrick wrote:
Please review webrev at [1] to address issue [2].
This is the new feature to add the jpackage option --jlink-options
as specified in CSR at [3]
/Andy
[1] http://cr.openjdk.java.net/~herrick/8219536/webrev.03
[2] https://bugs.openjdk.java.net/browse/JDK-8219536
[3] https://bugs.openjdk.java.net/browse/JDK-8243272
Re: RFR: JDK-8244018: No error message for non-existent icon path
Hi Andy, http://cr.openjdk.java.net/~herrick/8244018/webrev.01/src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/internal/resources/MainResources_ja.properties.frames.html http://cr.openjdk.java.net/~herrick/8244018/webrev.01/src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/internal/resources/MainResources_zh_CN.properties.frames.html Typo in year: 20120 -> 2020 Otherwise looks fine. Thanks, Alexander On 4/29/20 7:31 AM, Andy Herrick wrote: Please review fix at [1] for issue [2] The change just adds error when specified icon is not found, and a test for that. /Andy [1] - http://cr.openjdk.java.net/~herrick/8244018/webrev.01/ [2] - https://bugs.openjdk.java.net/browse/JDK-8244018
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
On 29/04/2020 08:10, Peter Levart wrote: Right, as you saw in a private Email, I did exactly that in a revised version (posted below). The spurious ISE may happen but only when close is called prematurely relative to all child scope close(s) that were or are still active. So we could say the other way: if close was not called prematurely, the ISE on acquire would not be spurious - so ordering of close relative to later acquire was wrong anyway and the exception is an "indication" of that wrong ordering Unless we want to use close() to "probe" the scope whether it is still active or not, I don't think this should present a problem. One quick comment: unless I'm missing something, this is starting to make a pretty strong assumption on how acquire()/close() are going to be used. Yes, if acquire() is not exposed to developers (as in the current API) and only hidden behind a spliterator, we might get away with this; but should we at some point re-introduce some kind of explicit acquire mechanism in the API, then such an assumption would not be a fine one to make. Maurizio
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
Many thanks Peter, my preference would be for adding the benchmark now, and come back to fix this post integration. The MemoryScope code is finicky and getting confidence that the code is race-free takes time and I'd prefer not to change that during the course of this RFR. Your approach seems promising, so let's keep working and thinking on it on the side (and perhaps let's maybe go for it in the panama repo first, to make sure we don't add regressions). Sounds like a plan? Cheers Maurizio On 29/04/2020 20:19, Peter Levart wrote: Hi Maurizio, On 4/29/20 2:41 AM, Maurizio Cimadamore wrote: The current implementation has performances that are on par with the previous acquire-based implementation, and also on par with what can be achieved with Unsafe. We do have a micro benchmark in the patch (see ParallelSum (**)) which tests this, and I get _identical_ numbers even if I _comment_ the body of acquire/release - so that no contention can happen; so, I'm a bit skeptical overall that contention on acquire/release is the main factor at play here - but perhaps we need more targeted benchmarks. So I modified your benchmark (just took out the relevant parts) and added some benchmarks that exhibit Stream.findAny() and Stream.findFirst(). As I anticipated, the results for parallel stream variants were slowing the benchmark down, so I had to reduce the number of elements by a factor of 16 to get results in reasonable time: http://cr.openjdk.java.net/~plevart/jdk-dev/8243491_MemoryScope/ParallelSum.java I then swapped-in the alternative implementation of MemoryScope (note that this is not a whole implementation - the dup() method is missing): http://cr.openjdk.java.net/~plevart/jdk-dev/8243491_MemoryScope/MemoryScope.java ...and I got these results: i7 2600K (4 cores / 8 threads) with proposed MemoryScope: Benchmark Mode Cnt Score Error Units ParallelSum.find_any_stream_parallel avgt 10 1332.687 ± 733.535 ms/op ParallelSum.find_any_stream_serial avgt 10 440.260 ± 3.110 ms/op ParallelSum.find_first_loop_serial avgt 10 5.809 ± 0.044 ms/op ParallelSum.find_first_stream_parallel avgt 10 2070.318 ± 41.072 ms/op ParallelSum.find_first_stream_serial avgt 10 440.034 ± 4.672 ms/op ParallelSum.sum_loop_serial avgt 10 5.647 ± 0.055 ms/op ParallelSum.sum_stream_parallel avgt 10 5.314 ± 0.294 ms/op ParallelSum.sum_stream_serial avgt 10 19.179 ± 0.136 ms/op with alternative MemoryScope: Benchmark Mode Cnt Score Error Units ParallelSum.find_any_stream_parallel avgt 10 80.280 ± 13.183 ms/op ParallelSum.find_any_stream_serial avgt 10 317.388 ± 2.787 ms/op ParallelSum.find_first_loop_serial avgt 10 5.790 ± 0.038 ms/op ParallelSum.find_first_stream_parallel avgt 10 117.925 ± 1.747 ms/op ParallelSum.find_first_stream_serial avgt 10 315.076 ± 5.725 ms/op ParallelSum.sum_loop_serial avgt 10 5.652 ± 0.042 ms/op ParallelSum.sum_stream_parallel avgt 10 4.881 ± 0.053 ms/op ParallelSum.sum_stream_serial avgt 10 19.143 ± 0.035 ms/op So here it is. The proof that contention does occur. Regards, Peter
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
Hi, I think the problem with perf might be caused by the fact that while the array is now a constant, the elements are not (the array is mutable after all). For fields you can fix this by using @Stable, but not for CP entries :) I think what could work is; rather than ldc'ing the array, and then looking up the values with 'normal' Java code, we could have another dynamic constant that does the the array lookup as well. Then the resolved value is stored in a separate CP slot as a true constant. We probably want to have a bootstrap method in ConstantBootstraps that can do an arbitrary array lookup given an array and an index for that. Given the amount of work, I'd say definitely something that should be saved for another time, also since there doesn't appear to be a major payoff for doing that at the moment. Jorn On 29/04/2020 03:13, Maurizio Cimadamore wrote: On 28/04/2020 21:44, Maurizio Cimadamore wrote: On 28/04/2020 19:09, Mandy Chung wrote: On 4/28/20 12:58 AM, fo...@univ-mlv.fr wrote: I don't think you need to store all the values into static fields, you can directly do a ldc + aaload with the right index right where you need it, I think this is what you are thinking as reported in JDK-8243492: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8239578/webrev.01-accessor/ if the accessors are declared ACC_STATIC, yes ! Thanks for catching this and this way will not be hit JDK-824349. Here is the revised patch: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8239578/webrev.02/ Maurizio - do you mind remerge MemoryAccessVarHandleGenerator.java with webrev.02? I'll take care of that Not going down this road, sorry :-) I've added the changes (see attached patch), and all benchmarks are several order of magnitude slower. I think this is mostly caused by the fact that the addOffset/multiplyOffset handle are no longer cached in static constants. While I understand that there might be better ways to generate the code, I'd strongly prefer to leave the code as per last iteration. I can't honestly see in which way having 3-4 static fields in a synthetic VarHandle class is going to hurt (but I can see how much it hurts by not having said fields). Cheers Maurizio Maurizio thanks Mandy
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
Hi Maurizio, On 4/29/20 2:41 AM, Maurizio Cimadamore wrote: The current implementation has performances that are on par with the previous acquire-based implementation, and also on par with what can be achieved with Unsafe. We do have a micro benchmark in the patch (see ParallelSum (**)) which tests this, and I get _identical_ numbers even if I _comment_ the body of acquire/release - so that no contention can happen; so, I'm a bit skeptical overall that contention on acquire/release is the main factor at play here - but perhaps we need more targeted benchmarks. So I modified your benchmark (just took out the relevant parts) and added some benchmarks that exhibit Stream.findAny() and Stream.findFirst(). As I anticipated, the results for parallel stream variants were slowing the benchmark down, so I had to reduce the number of elements by a factor of 16 to get results in reasonable time: http://cr.openjdk.java.net/~plevart/jdk-dev/8243491_MemoryScope/ParallelSum.java I then swapped-in the alternative implementation of MemoryScope (note that this is not a whole implementation - the dup() method is missing): http://cr.openjdk.java.net/~plevart/jdk-dev/8243491_MemoryScope/MemoryScope.java ...and I got these results: i7 2600K (4 cores / 8 threads) with proposed MemoryScope: Benchmark Mode Cnt Score Error Units ParallelSum.find_any_stream_parallel avgt 10 1332.687 ± 733.535 ms/op ParallelSum.find_any_stream_serial avgt 10 440.260 ± 3.110 ms/op ParallelSum.find_first_loop_serial avgt 10 5.809 ± 0.044 ms/op ParallelSum.find_first_stream_parallel avgt 10 2070.318 ± 41.072 ms/op ParallelSum.find_first_stream_serial avgt 10 440.034 ± 4.672 ms/op ParallelSum.sum_loop_serial avgt 10 5.647 ± 0.055 ms/op ParallelSum.sum_stream_parallel avgt 10 5.314 ± 0.294 ms/op ParallelSum.sum_stream_serial avgt 10 19.179 ± 0.136 ms/op with alternative MemoryScope: Benchmark Mode Cnt Score Error Units ParallelSum.find_any_stream_parallel avgt 10 80.280 ± 13.183 ms/op ParallelSum.find_any_stream_serial avgt 10 317.388 ± 2.787 ms/op ParallelSum.find_first_loop_serial avgt 10 5.790 ± 0.038 ms/op ParallelSum.find_first_stream_parallel avgt 10 117.925 ± 1.747 ms/op ParallelSum.find_first_stream_serial avgt 10 315.076 ± 5.725 ms/op ParallelSum.sum_loop_serial avgt 10 5.652 ± 0.042 ms/op ParallelSum.sum_stream_parallel avgt 10 4.881 ± 0.053 ms/op ParallelSum.sum_stream_serial avgt 10 19.143 ± 0.035 ms/op So here it is. The proof that contention does occur. Regards, Peter
Re: LinkedHashMap/LinkedHashSet enhancement: OrderedMap/OrderedSet
On 4/28/20 9:48 PM, Jason Mehrens wrote: Looks like It is intentional that unmodifiable queues are not present. See: https://bugs.openjdk.java.net/browse/JDK-5030930. The same logic would have been used for when Deque was added in the following release. Good find. Looking at the Queue interface, it adds four mutator methods and two access methods: element() and peek(). These latter two provide only a tiny bit of convenience over an iterator, so an unmodifiable Queue provides hardly any value over Collection. Thus the rationale in JDK-5030930 for not providing an unmodifiable Queue makes sense. Deque is considerably richer than Queue, not only in mutator methods. It also adds access methods for both ends (null-returning and throwing), with better names, plus a descending iterator. That might make it worthwhile reconsidering an unmodifiable Deque. s'marks
Re: RFR: JDK-8219536: Add Option for user defined jlink options
I don't think I sent out webrev.5 [6] fixing Alexander's points below.
Please Review:
[6] http://cr.openjdk.java.net/~herrick/8219536/webrev.05/index.html
/Andy
On 4/23/2020 7:59 PM, Alexander Matveev wrote:
Hi Andy,
http://cr.openjdk.java.net/~herrick/8219536/webrev.04/src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/internal/Arguments.java.frames.html
1) Copyright year needs to be updated. Other files also needs
copyright year to be updated.
2) Line 778: Not sure why it was moved to single line. I think it is
better to revert it back.
Otherwise looks fine.
Thanks,
Alexander
On 4/23/20 1:48 PM, Andy Herrick wrote:
Please review updated webrev at [5] to address comments below from
Alexey.
[5] http://cr.openjdk.java.net/~herrick/8219536/webrev.04
/Andy
On 4/23/2020 11:17 AM, Alexey Semenyuk wrote:
http://cr.openjdk.java.net/~herrick/8219536/webrev.03/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JPackageCommand.java.sdiff.html:731
- 'launcherName' parameter of readRuntimeReleaseFile() function
seems to be not used.
http://cr.openjdk.java.net/~herrick/8219536/webrev.03/test/jdk/tools/jpackage/share/jdk/jpackage/tests/JLinkOptionsTest.java.html:
copyright year should be 2020.
http://cr.openjdk.java.net/~herrick/8219536/webrev.03/test/jdk/tools/jpackage/share/jdk/jpackage/tests/JLinkOptionsTest.java.html:
128 - There is no point to assert if 'cfgfile' is not null. It
should be always non null for application packaging or
readLaunherCfgFile.
For testing if string contains some values, I'd suggest to use
TKit.assertTextStream():
---
List mods = List.of(release.get(1));
if (required != null) {
for (String s : required) {
TKit.assertTextStream(s).label("mods").apply(mods.stream());
}
}
if (prohibited != null) {
for (String s : prohibited) {
TKit.assertTextStream(s).label("mods").negate().apply(mods.stream());
}
}
---
Will save you from maintaining explicit log messages.
- Alexey
On 4/23/2020 10:08 AM, Andy Herrick wrote:
Please review webrev at [1] to address issue [2].
This is the new feature to add the jpackage option --jlink-options
as specified in CSR at [3]
/Andy
[1] http://cr.openjdk.java.net/~herrick/8219536/webrev.03
[2] https://bugs.openjdk.java.net/browse/JDK-8219536
[3] https://bugs.openjdk.java.net/browse/JDK-8243272
Re: RFR[8183266] - [TESTBUG]Add test to cover XPathEvaluationResult.XPathResultType.getQNameType method
Hi Fernando, Thanks for adding coverage to this API. The change looks good overall. A couple of comments to the test: Line 39 instead of LocalPart, it may be better to verify the QName themselves, I mean, assert the QNames are equal. Line 19-21: the comment block can be moved to a @summary tag with a message sth. like "Verifies the specification of the XPathEvaluationResult API" to allow potential future additions. Best, Joe On 4/29/2020 4:50 AM, Fernando Guallini wrote: Hi all, Please, review the following change: webrev: http://cr.openjdk.java.net/~joehw/jdk15/8183266/webrev/ bug: https://bugs.openjdk.java.net/browse/JDK-8183266 Change details: - Added test coverage for XPathEvaluationResult.XPathResultType.getQNameType method - Added type check for the getQNameType flow restricting the Number class subtypes to satisfy the spec (Integer, Double and Long) - Updated equalsClassType method to be null safe. Kind regards, Fernando
Re: RFR [15] 8243541: (tz) Upgrade time-zone data to tzdata2020a
On 27/04/2020 19:19, Kiran Ravikumar wrote: > Hi Martin and Andrew, > > > Thanks for the review and providing a direction towards updating the > translations. > > > Here is an updated webrev with the changes: > > http://cr.openjdk.java.net/~kravikumar/8243541/webrev/ > > > All associated tests pass. Please let me know if they look alright. > > > Thanks, > > Kiran > Looks good to me. Now we have a translation for Nuuk in all the languages for which we had Godthab translations, and the zones are tested too. Thumbs up from me! Thanks, -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222 https://keybase.io/gnu_andrew
RFR: JDK-8244018: No error message for non-existent icon path
Please review fix at [1] for issue [2] The change just adds error when specified icon is not found, and a test for that. /Andy [1] - http://cr.openjdk.java.net/~herrick/8244018/webrev.01/ [2] - https://bugs.openjdk.java.net/browse/JDK-8244018
Re: RFR(T) : 8244052 : remove copying of s.h.WB$WhiteBoxPermission in test/jdk
Serguei, David, thanks for your reviews. pushed. -- Igor > On Apr 28, 2020, at 11:39 PM, serguei.spit...@oracle.com wrote: > > LGTM++ > > Thanks, > Serguei > > > On 4/28/20 23:28, David Holmes wrote: >> Looks good! >> >> Thanks, >> David >> >> On 29/04/2020 1:20 pm, Igor Ignatyev wrote: >>> http://cr.openjdk.java.net/~iignatyev//8244052/webrev.00/ 34 lines changed: 0 ins; 11 del; 23 mod; >>> >>> Hi all, >>> >>> could you please review this trivial patch? >>> from JBS: JDK-8199290 made it unnecessary to explicitly pass sun.hotspot.WhiteBox$WhiteBoxPermission as an argument to ClassFileInstaller b/c the former now copies it every time it copies sun.hotspot.WhiteBox. >>> >>> besides removing WhiteBoxPermission from the list of ClassFileInstaller's >>> arguments, the patch also makes sure ClassFileInstaller is run in a driver >>> mode in all the touched tests. >>> >>> testing: the changed tests >>> webrev: http://cr.openjdk.java.net/~iignatyev//8244052/webrev.00/ >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244052 >>> >>> Thanks, >>> -- Igor >>> >
Re: RFR(T) : 8244066 : ClassFileInstaller should be run in driver mode
Hi Stefan, I've already pushed it (right after Ioi reviewed it), nevertheless I appreciate you reviewing it. -- Igor > On Apr 28, 2020, at 11:11 PM, Stefan Karlsson > wrote: > > Looks good. > > StefanK > > On 2020-04-29 06:41, Igor Ignatyev wrote: >> http://cr.openjdk.java.net/~iignatyev//8244066/webrev.00 >>> 16 lines changed: 0 ins; 0 del; 16 mod; >> Hi all, >> >> could you please review this trivial cleanup in tests? >> from JBS: >>> ClassFileInstaller is a test utility class which copies class files to the >>> current directory, there is no point in running it w/ external flags, hence >>> it should be run in a driver mode. >> testing: updated tests >> webrev: http://cr.openjdk.java.net/~iignatyev//8244066/webrev.00 >> JBS: https://bugs.openjdk.java.net/browse/JDK-8244066 >> >> Thanks, >> -- Igor >
RFR[8183266] - [TESTBUG]Add test to cover XPathEvaluationResult.XPathResultType.getQNameType method
Hi all, Please, review the following change: webrev: http://cr.openjdk.java.net/~joehw/jdk15/8183266/webrev/ bug: https://bugs.openjdk.java.net/browse/JDK-8183266 Change details: - Added test coverage for XPathEvaluationResult.XPathResultType.getQNameType method - Added type check for the getQNameType flow restricting the Number class subtypes to satisfy the spec (Integer, Double and Long) - Updated equalsClassType method to be null safe. Kind regards, Fernando
RFR: 6415694 Clarification in Javadoc for java.rmi.AlreadyBoundException
Hi All, Please find the below changes for the issues https://bugs.openjdk.java.net/browse/JDK-6415694. It is a description change, which was already approved by the reviewer. Thanks! diff -r 59b5bd9a7168 src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java --- a/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon Mar 16 02:16:49 2020 -0400 +++ b/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon Mar 30 15:45:43 2020 +0530 @@ -26,8 +26,8 @@ /** * An AlreadyBoundException is thrown if an attempt - * is made to bind an object in the registry to a name that already - * has an associated binding. + * is made to bind an object to a name that already + * has an associated binding in the registry. * * @since 1.1 * @author Ann Wollrath
RFR: 7147994 Hashtable rehash() javadoc describes implementation details
Hi All,
Please find the below changes for the issues
https://bugs.openjdk.java.net/browse/JDK-7147994
It is a description change, which was already approved by the reviewer.
Thanks!
diff -r 59b5bd9a7168 src/java.base/share/classes/java/util/Hashtable.java
--- a/src/java.base/share/classes/java/util/Hashtable.java Mon Mar 16
02:16:49 2020 -0400
+++ b/src/java.base/share/classes/java/util/Hashtable.java Mon Mar 30
15:45:43 2020 +0530
@@ -399,9 +399,9 @@
/**
* Increases the capacity of and internally reorganizes this
* hashtable, in order to accommodate and access its entries more
- * efficiently. This method is called automatically when the
- * number of keys in the hashtable exceeds this hashtable's capacity
- * and load factor.
+ * efficiently. This method is called to increase the capacity of and
+ * internally reorganize this hashtable in order to more efficiently
+ * accommodate and access its entries.
*/
@SuppressWarnings("unchecked")
protected void rehash() {
Re: Java 11 vs 14 MethodHandle behavior change
Hi,
On Tue, Apr 28, 2020 at 7:34 PM Mandy Chung wrote:
> endPointClass is in unnamed module and so it's unconditionally exported. The
> public lookup should be able to find public members from it.One thing to
> double check if endPointClass is publicly accessible?
It is.
> Did you get any exception in 14? Is it from findVirtual or from in?
>From findVirtual():
Exception in thread "main" java.lang.IllegalAccessException: no such
method: org.module1.MyEndPoint.onMessage(MyString)void/invokeVirtual
at
java.base/java.lang.invoke.MemberName.makeAccessException(MemberName.java:971)
at
java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1114)
at
java.base/java.lang.invoke.MethodHandles$Lookup.resolveOrFail(MethodHandles.java:2785)
at
java.base/java.lang.invoke.MethodHandles$Lookup.findVirtual(MethodHandles.java:1883)
at org.module2.Main.main(Main.java:28)
Caused by: java.lang.LinkageError: loader constraint violation: when
resolving method 'void
org.module1.MyEndPoint.onMessage(org.module1.MyString)' the class
loader 'bootstrap' of the current class, java/lang/Object, and the
class loader java.net.URLClassLoader @7291c18f for the method's
defining class, org/module1/MyEndPoint, have different Class objects
for the type org/module1/MyString used in the signature
(java.lang.Object is in module java.base of loader 'bootstrap';
org.module1.MyEndPoint is in unnamed module of loader
java.net.URLClassLoader @7291c18f, parent loader 'app')
at java.base/java.lang.invoke.MethodHandleNatives.resolve(Native Method)
at
java.base/java.lang.invoke.MemberName$Factory.resolve(MemberName.java:1084)
at
java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:)
... 3 more
> The current implementation already does that.
>
> private Class lookupClassOrNull() {
> if (allowedModes == TRUSTED) {
> return null;
> }
> if (allowedModes == UNCONDITIONAL) {
> // use Object as the caller to pass to VM doing resolution
> return Object.class;
> }
> return lookupClass;
> }
>
> What exactly have you changed?
if (allowedModes == UNCONDITIONAL) {
return lookupClass;
}
> Yes, please file a JBS issue and I will look into it. If the requested
> target class to be accessed through Lookup::in is exported, it should work
> because the set of classes that public lookups can access should not change.
https://bugs.openjdk.java.net/browse/JDK-8244090
Thanks!
--
Simone Bordet
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless. Victoria Livschitz
Collections.synchronizedXXX() and internal mutex (aka SyncRoot)
Hello,
Have you ever discussed to make field mutex in synchronized collections
accessible?
Javadoc for Collections#synchronizedSortedSet suggest to iterate collection
this way:
SortedSet s = Collections.synchronizedSortedSet(new TreeSet());
SortedSet s2 = s.headSet(foo);
...
synchronized (s) { // Note: s, not s2!!!
Iterator i = s2.iterator(); // Must be in the synchronized block
while (i.hasNext())
foo(i.next());
}
I.e. in order to iterate subset, we also need a reference to the whole set,
which is not really convenient. How about to make it possible to write:
SortedSet s2 = s.headSet(foo);
...
synchronized (Collections.getSyncRoot(s2)) { // Note:
Collections.getSyncRoot(s2)
Iterator i = s2.iterator(); // Must be in the synchronized block
while (i.hasNext())
foo(i.next());
}
Also I think it would be convenient to let to provide custom sync root when
synchronized collection is created.
E.g.
Object customSyncRoot = new Object();
SortedSet s = Collections.synchronizedSortedSet(new TreeSet(),
customSyncRoot);
What do you think about this?
Regards,
Dmytro
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
> De: "mandy chung" > À: "Remi Forax" > Cc: "Maurizio Cimadamore" , "core-libs-dev" > > Envoyé: Mardi 28 Avril 2020 20:09:07 > Objet: Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second > Incubator) > On 4/28/20 12:58 AM, [ mailto:fo...@univ-mlv.fr | fo...@univ-mlv.fr ] wrote: I don't think you need to store all the values into static fields, you can directly do a ldc + aaload with the right index right where you need it, >>> I think this is what you are thinking as reported in JDK-8243492: >>> [ >>> http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8239578/webrev.01-accessor/ >>> | >>> http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8239578/webrev.01-accessor/ >>> ] >> if the accessors are declared ACC_STATIC, yes ! > Thanks for catching this and this way will not be hit JDK-824349. > Here is the revised patch: > [ http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8239578/webrev.02/ | > http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8239578/webrev.02/ ] Looks good to me ! > Maurizio - do you mind remerge MemoryAccessVarHandleGenerator.java with > webrev.02? > thanks > Mandy cheers, Rémi
Re: RFR 8243491: Implementation of Foreign-Memory Access API (Second Incubator)
On 4/29/20 2:41 AM, Maurizio Cimadamore wrote:
On 28/04/2020 21:27, Peter Levart wrote:
Hi,
The problem with current implementation of MemoryScope is that if a
child scope is frequently acquired and closed (which increments and
then decrements the parent scope counter atomically using CAS), and
that is performed from multiple concurrent threads, contention might
become prohibitive. And I think that is precisely what happens when a
parallel pipeline is such that it might short-circuit the stream:
final boolean forEachWithCancel(Spliterator spliterator,
Sink sink) {
boolean cancelled;
do { } while (!(cancelled = sink.cancellationRequested()) &&
spliterator.tryAdvance(sink));
return cancelled;
}
1st spliterators are created by trySplit (all of them inherit the
same MemoryScope) and then FJPool threads are busy concurrently
executing above method which calls tryAdvance for each element of the
particular spliterator which does the following:
public boolean tryAdvance(Consumer
action) {
Objects.requireNonNull(action);
if (currentIndex < elemCount) {
AbstractMemorySegmentImpl acquired = segment.acquire();
try {
action.accept(acquired.asSliceNoCheck(currentIndex * elementSize,
elementSize));
} finally {
acquired.closeNoCheck();
currentIndex++;
if (currentIndex == elemCount) {
segment = null;
}
}
return true;
} else {
return false;
}
}
... acquire/close at each call. If the Stream is played to the end
(i.e. it can't short-circuit), then forEachRemaining is used which
performs just one acquire/close for the whole remaining spliterator.
So for short-circuiting streams it might be important to have a
MemoryScope that is scalable. Here's one such attempt using a pair of
scalable counters (just one pair per root memory scope):
The current implementation has performances that are on par with the
previous acquire-based implementation, and also on par with what can
be achieved with Unsafe. We do have a micro benchmark in the patch
(see ParallelSum (**)) which tests this, and I get _identical_ numbers
even if I _comment_ the body of acquire/release - so that no
contention can happen; so, I'm a bit skeptical overall that contention
on acquire/release is the main factor at play here - but perhaps we
need more targeted benchmarks.
Right, summing is typically not a short-circuiting operation, so I bet
forEachRemaining is used in the leaf spliterators. FindAny or findFirst
might be the ones to test. I'll prepare a test and see what
experimenting with alternative MemoryScope can do...
(**) - your email caused me to look deeper at the ParallelSum
benchmark which, as currently written seems to favor Unsafe over the
MemorySegment API - but in reality, as I discovered, that is down to
an issue in the implementation of the unsafe spliterator, which
doesn't sum all the elements; I will fix the benchmark in an upcoming
iteration
So, while I'm open to suggestion as to how to reduce contention on the
acquire counter, I think we need more evidence that this is indeed an
issue (or the _main_ issue, when it comes to parallel computation).
That said, your implementation looks interesting - some questions
inline and also below:
answers inline...
import java.util.concurrent.atomic.LongAdder;
/**
* @author Peter Levart
*/
public abstract class MemoryScope {
public static MemoryScope create(Object ref, Runnable
cleanupAction) {
return new Root(ref, cleanupAction);
}
MemoryScope() {}
public abstract MemoryScope acquire();
public abstract void close();
private static class Root extends MemoryScope {
private final LongAdder enters = new LongAdder();
private final LongAdder exits = new LongAdder();
private volatile boolean closed;
private final Object ref;
private final Runnable cleanupAction;
Root(Object ref, Runnable cleanupAction) {
this.ref = ref;
this.cleanupAction = cleanupAction;
}
@Override
public MemoryScope acquire() {
// increment enters 1st
enters.increment();
// check closed flag 2nd
if (closed) {
exits.increment();
throw new IllegalStateException("This scope is
already closed");
}
return new MemoryScope() {
@Override
public MemoryScope acquire() {
return Root.this.acquire();
}
@Override
public void close() {
exits.increment();
Here -- don't you mean Root.this.exits? Otherwise Root.exists is gonna
remain !=
Re: RFR(T) : 8244052 : remove copying of s.h.WB$WhiteBoxPermission in test/jdk
LGTM++ Thanks, Serguei On 4/28/20 23:28, David Holmes wrote: Looks good! Thanks, David On 29/04/2020 1:20 pm, Igor Ignatyev wrote: http://cr.openjdk.java.net/~iignatyev//8244052/webrev.00/ 34 lines changed: 0 ins; 11 del; 23 mod; Hi all, could you please review this trivial patch? from JBS: JDK-8199290 made it unnecessary to explicitly pass sun.hotspot.WhiteBox$WhiteBoxPermission as an argument to ClassFileInstaller b/c the former now copies it every time it copies sun.hotspot.WhiteBox. besides removing WhiteBoxPermission from the list of ClassFileInstaller's arguments, the patch also makes sure ClassFileInstaller is run in a driver mode in all the touched tests. testing: the changed tests webrev: http://cr.openjdk.java.net/~iignatyev//8244052/webrev.00/ JBS: https://bugs.openjdk.java.net/browse/JDK-8244052 Thanks, -- Igor
Re: RFR(T) : 8244052 : remove copying of s.h.WB$WhiteBoxPermission in test/jdk
Looks good! Thanks, David On 29/04/2020 1:20 pm, Igor Ignatyev wrote: http://cr.openjdk.java.net/~iignatyev//8244052/webrev.00/ 34 lines changed: 0 ins; 11 del; 23 mod; Hi all, could you please review this trivial patch? from JBS: JDK-8199290 made it unnecessary to explicitly pass sun.hotspot.WhiteBox$WhiteBoxPermission as an argument to ClassFileInstaller b/c the former now copies it every time it copies sun.hotspot.WhiteBox. besides removing WhiteBoxPermission from the list of ClassFileInstaller's arguments, the patch also makes sure ClassFileInstaller is run in a driver mode in all the touched tests. testing: the changed tests webrev: http://cr.openjdk.java.net/~iignatyev//8244052/webrev.00/ JBS: https://bugs.openjdk.java.net/browse/JDK-8244052 Thanks, -- Igor
Re: RFR(T) : 8244066 : ClassFileInstaller should be run in driver mode
Looks good. StefanK On 2020-04-29 06:41, Igor Ignatyev wrote: http://cr.openjdk.java.net/~iignatyev//8244066/webrev.00 16 lines changed: 0 ins; 0 del; 16 mod; Hi all, could you please review this trivial cleanup in tests? from JBS: ClassFileInstaller is a test utility class which copies class files to the current directory, there is no point in running it w/ external flags, hence it should be run in a driver mode. testing: updated tests webrev: http://cr.openjdk.java.net/~iignatyev//8244066/webrev.00 JBS: https://bugs.openjdk.java.net/browse/JDK-8244066 Thanks, -- Igor
