Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Hi Brent,
This looks good. It might also be a good idea to add a test that checks
this new implementation detail (the unsynchronized get) that new code
will become dependent upon, for example:
/*
* @test
* @bug 8029891
* @summary Test that the Properties.get() method does not synchronize
any more
* @run main CheckUnsynchronizedGet
*/
public class CheckUnsynchronizedGet {
public static void main(String[] args) {
Properties props = new Properties();
synchronized (props) {
props.setProperty("key", "value");
String value =
CompletableFuture.supplyAsync(() ->
props.getProperty("key")).join();
assert "value".equals(value);
}
}
}
What do you think?
Regards, Peter
On 05/13/2016 01:55 AM, Brent Christian wrote:
Update to the test, and additional comments:
http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/
Thanks,
-Brent
On 5/12/16 4:15 PM, Mandy Chung wrote:
On May 12, 2016, at 2:44 PM, Brent Christian
wrote:
On 5/12/16 11:44 AM, Mandy Chung wrote:
This patch looks good.
To help future readers to understand this, it may be better to move:
1152 private transient ConcurrentHashMap map =
1153 new ConcurrentHashMap<>(8);
to the beginning and add a comment describing what are lock-free
and what are synchronized (basically some part of your summary below).
Also a comment in Hashtable::defaultWriteHashtable and
readHashtable methods to mention that they are overridden by
Properties.
Good ideas.
In the GetResource.java test, what is the reason taking out:
73 URL u2 =
Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
It was coming back null and failing the test, I think because
Jigsaw moved CalendarData into a different module (jdk.localedata, I
believe?).
src/java.base/share/classes/sun/util/resources/CalendarData.properties
is still in java.base. This is due to resource encapsulation.
Unnamed module calling ClassLoader::getResource of a resource in a
named module returns null.
I fiddled with @modules a bit, but stopped when I discovered that
when the test deadlocks, it hangs on the first call to getResource()
and doesn't get to this line.
I can look into getting it to load CalendarData again, if you like.
The launcher and builtin class loader implementation has changed so
much that the code path exercised JDK-6977738 no longer exists. I
think dropping line 73 is okay.
Mandy
Re: RFR[9]:Fix java/lang/invoke/MethodHandleImpl's use of Unsafe.defineAnonymousClass()
Thank you Vladimir for comments. Please review updated webrev http://cr.openjdk.java.net/~srastogi/8149574/webrev.09/ Regards, Shilpi On 5/12/2016 6:12 PM, Vladimir Ivanov wrote: rankly speaking, I'd prefer [2] to be co
Re: MethodHandle for array length
Hi Uwe,
I was planning to add a bug for this feature but it seems that Michael was
faster than me,
https://bugs.openjdk.java.net/browse/JDK-8156915
regards,
Rémi
- Mail original -
> De: "Uwe Schindler"
> À: "Michael Haupt" , "Core-Libs-Dev"
>
> Envoyé: Jeudi 12 Mai 2016 14:34:34
> Objet: RE: MethodHandle for array length
>
> Hi Michael,
>
> > > Am 11.05.2016 um 21:35 schrieb Uwe Schindler :
> > > With Java 9 this gets a bit worse: There is no "easy way" with the
> > MethodHanldes class to generate a MethodHandles.countedLoop() on all
> > elements of an array:
> > >
> > > public static MethodHandle countedLoop(MethodHandle iterations,
> > MethodHandle init, MethodHandle body) [new in Java 9]
> >
> > this isn't a remedy when you need the index in each iteration, but you can
> > generate a loop over all elements of an array using iteratedLoop(), as
> > illustrated by this test from LoopCombinatorTest:
> >
> > @Test
> > public static void testIterateSum() throws Throwable {
> > // Integer[] a = new Integer[]{1,2,3,4,5,6}; int sum = 0; for (int e :
> > a) { sum
> > += e; } return sum; => 21
> > MethodHandle loop =
> > MethodHandles.iteratedLoop(Iterate.MH_sumIterator, Iterate.MH_sumInit,
> > Iterate.MH_sumStep);
> > assertEquals(Iterate.MT_sum, loop.type());
> > assertEquals(21, loop.invoke(new Integer[]{1, 2, 3, 4, 5, 6}));
> > }
> >
> > ... where MH_sumIterator is a handle to this method:
> >
> > static Iterator sumIterator(Integer[] a) {
> > return Arrays.asList(a).iterator();
> > }
>
> Of course this works, too.
>
> My proposal or question here was more about the API inconsistency in general.
> You can do a lot here and there, you can implement effective or
> non-effective loops with MethodHandles, but still the following holds true:
> java.lang.invoke.MethodHandles is missing an accessor for "array.length",
> which is not understandable. The "countedLoop" here was just an example use
> case (if that one is great or not does not matter), it was just posted to
> actually show the API and how it *could* be used.
>
> The real example from our script engine was not related to loops, it was an
> actual invokedynamic callsite implementation where it was not possible to
> figure out how to get a method handle to the very special arraylength byte
> code, aka "array.length javac sugar". Everything else to handle arrays is
> there at one single place, but the array length is missing and you get very
> annoyed. If you are not so familiar to bytecode and the mechanics behind,
> you have a very hard time to find out how to do this: the intuitive
> solutions does not work: "array.length" -> aha it’s a field, so let's try
> Lookup.findGetter(long[].class, "length", int.class) -> does not work!
>
> What is so hard to accept the proposal to have
> MethodHandles.arrayLengthGetter, implemented in the most effective way (as a
> methodhandle that solely calls the special bytecode "arraylength")? In fact
> thais is like a 10 liner to implement + Javadocs. No need to delay a
> release, there are still changes in JDK 9 going on that are far more risky
> than this.
>
> Best,
> Uwe
>
> > Best,
> >
> > Michael
> >
> > --
> >
> > Oracle is committed to
> > developing practices and products that help protect the environment
>
>
Re: RFR[9]:Fix java/lang/invoke/MethodHandleImpl's use of Unsafe.defineAnonymousClass()
> On 13 May 2016, at 09:13, shilpi.rast...@oracle.com wrote:
>
> Thank you Vladimir for comments.
>
> Please review updated webrev
> http://cr.openjdk.java.net/~srastogi/8149574/webrev.09/
>
Just one minor comment (last one i promise!):
1140 MethodHandle vamh = prepareForInvoker(MH_checkCallerClass);
1141 Object ok = bccInvoker.invokeExact(vamh, new
Object[]{hostClass, bcc});
1142 assert((boolean)ok);
If you wanna keep that assert I think it would be better to state:
assert Boolean.TRUE.equals(ok) : ok;
since the cast could theoretically result in an NPE rather than an AE.
Paul.
Re: RFR[9]:Fix java/lang/invoke/MethodHandleImpl's use of Unsafe.defineAnonymousClass()
Thanks Paul! Please review http://cr.openjdk.java.net/~srastogi/8149574/webrev10.0/ Regards, Shilpi On 5/13/2016 2:09 PM, Paul Sandoz wrote: assert Boolean.TRUE.equals(ok) : ok;
Re: RFR[9]:Fix java/lang/invoke/MethodHandleImpl's use of Unsafe.defineAnonymousClass()
> On 13 May 2016, at 10:56, shilpi.rast...@oracle.com wrote: > > Thanks Paul! > > Please review http://cr.openjdk.java.net/~srastogi/8149574/webrev10.0/ > +1 Paul.
RE: MethodHandle for array length
Hi,
OK, thanks for creating an issue!
Uwe
-
Uwe Schindler
uschind...@apache.org
ASF Member, Apache Lucene PMC / Committer
Bremen, Germany
http://lucene.apache.org/
> -Original Message-
> From: Remi Forax [mailto:fo...@univ-mlv.fr]
> Sent: Friday, May 13, 2016 10:37 AM
> To: Uwe Schindler
> Cc: Michael Haupt ; Core-Libs-Dev d...@openjdk.java.net>
> Subject: Re: MethodHandle for array length
>
> Hi Uwe,
> I was planning to add a bug for this feature but it seems that Michael was
> faster than me,
> https://bugs.openjdk.java.net/browse/JDK-8156915
>
> regards,
> Rémi
>
> - Mail original -
> > De: "Uwe Schindler"
> > À: "Michael Haupt" , "Core-Libs-Dev" libs-...@openjdk.java.net>
> > Envoyé: Jeudi 12 Mai 2016 14:34:34
> > Objet: RE: MethodHandle for array length
> >
> > Hi Michael,
> >
> > > > Am 11.05.2016 um 21:35 schrieb Uwe Schindler
> :
> > > > With Java 9 this gets a bit worse: There is no "easy way" with the
> > > MethodHanldes class to generate a MethodHandles.countedLoop() on all
> > > elements of an array:
> > > >
> > > > public static MethodHandle countedLoop(MethodHandle iterations,
> > > MethodHandle init, MethodHandle body) [new in Java 9]
> > >
> > > this isn't a remedy when you need the index in each iteration, but you
> can
> > > generate a loop over all elements of an array using iteratedLoop(), as
> > > illustrated by this test from LoopCombinatorTest:
> > >
> > > @Test
> > > public static void testIterateSum() throws Throwable {
> > > // Integer[] a = new Integer[]{1,2,3,4,5,6}; int sum = 0; for (int e :
> > > a) { sum
> > > += e; } return sum; => 21
> > > MethodHandle loop =
> > > MethodHandles.iteratedLoop(Iterate.MH_sumIterator,
> Iterate.MH_sumInit,
> > > Iterate.MH_sumStep);
> > > assertEquals(Iterate.MT_sum, loop.type());
> > > assertEquals(21, loop.invoke(new Integer[]{1, 2, 3, 4, 5, 6}));
> > > }
> > >
> > > ... where MH_sumIterator is a handle to this method:
> > >
> > > static Iterator sumIterator(Integer[] a) {
> > > return Arrays.asList(a).iterator();
> > > }
> >
> > Of course this works, too.
> >
> > My proposal or question here was more about the API inconsistency in
> general.
> > You can do a lot here and there, you can implement effective or
> > non-effective loops with MethodHandles, but still the following holds true:
> > java.lang.invoke.MethodHandles is missing an accessor for "array.length",
> > which is not understandable. The "countedLoop" here was just an example
> use
> > case (if that one is great or not does not matter), it was just posted to
> > actually show the API and how it *could* be used.
> >
> > The real example from our script engine was not related to loops, it was an
> > actual invokedynamic callsite implementation where it was not possible to
> > figure out how to get a method handle to the very special arraylength byte
> > code, aka "array.length javac sugar". Everything else to handle arrays is
> > there at one single place, but the array length is missing and you get very
> > annoyed. If you are not so familiar to bytecode and the mechanics behind,
> > you have a very hard time to find out how to do this: the intuitive
> > solutions does not work: "array.length" -> aha it’s a field, so let's try
> > Lookup.findGetter(long[].class, "length", int.class) -> does not work!
> >
> > What is so hard to accept the proposal to have
> > MethodHandles.arrayLengthGetter, implemented in the most effective
> way (as a
> > methodhandle that solely calls the special bytecode "arraylength")? In fact
> > thais is like a 10 liner to implement + Javadocs. No need to delay a
> > release, there are still changes in JDK 9 going on that are far more risky
> > than this.
> >
> > Best,
> > Uwe
> >
> > > Best,
> > >
> > > Michael
> > >
> > > --
> > >
> > > Oracle is committed to
> > > developing practices and products that help protect the environment
> >
> >
RE: MethodHandle for array length
Hi,
One addition, maybe add to issue:
If this was added, jdk.dynalink module could use it, too - this was mentioned
by Attila already:
http://hg.openjdk.java.net/jdk9/dev/nashorn/file/4b118e012ac4/src/jdk.dynalink/share/classes/jdk/dynalink/beans/BeanLinker.java
Uwe
-
Uwe Schindler
uschind...@apache.org
ASF Member, Apache Lucene PMC / Committer
Bremen, Germany
http://lucene.apache.org/
> -Original Message-
> From: Uwe Schindler [mailto:uschind...@apache.org]
> Sent: Friday, May 13, 2016 11:55 AM
> To: 'Remi Forax'
> Cc: 'Michael Haupt' ; 'Core-Libs-Dev' d...@openjdk.java.net>
> Subject: RE: MethodHandle for array length
>
> Hi,
>
> OK, thanks for creating an issue!
>
> Uwe
>
> -
> Uwe Schindler
> uschind...@apache.org
> ASF Member, Apache Lucene PMC / Committer
> Bremen, Germany
> http://lucene.apache.org/
>
> > -Original Message-
> > From: Remi Forax [mailto:fo...@univ-mlv.fr]
> > Sent: Friday, May 13, 2016 10:37 AM
> > To: Uwe Schindler
> > Cc: Michael Haupt ; Core-Libs-Dev > d...@openjdk.java.net>
> > Subject: Re: MethodHandle for array length
> >
> > Hi Uwe,
> > I was planning to add a bug for this feature but it seems that Michael was
> > faster than me,
> > https://bugs.openjdk.java.net/browse/JDK-8156915
> >
> > regards,
> > Rémi
> >
> > - Mail original -
> > > De: "Uwe Schindler"
> > > À: "Michael Haupt" , "Core-Libs-Dev"
> > libs-...@openjdk.java.net>
> > > Envoyé: Jeudi 12 Mai 2016 14:34:34
> > > Objet: RE: MethodHandle for array length
> > >
> > > Hi Michael,
> > >
> > > > > Am 11.05.2016 um 21:35 schrieb Uwe Schindler
> > :
> > > > > With Java 9 this gets a bit worse: There is no "easy way" with the
> > > > MethodHanldes class to generate a MethodHandles.countedLoop() on
> all
> > > > elements of an array:
> > > > >
> > > > > public static MethodHandle countedLoop(MethodHandle iterations,
> > > > MethodHandle init, MethodHandle body) [new in Java 9]
> > > >
> > > > this isn't a remedy when you need the index in each iteration, but you
> > can
> > > > generate a loop over all elements of an array using iteratedLoop(), as
> > > > illustrated by this test from LoopCombinatorTest:
> > > >
> > > > @Test
> > > > public static void testIterateSum() throws Throwable {
> > > > // Integer[] a = new Integer[]{1,2,3,4,5,6}; int sum = 0; for (int
> > > > e :
> > > > a) { sum
> > > > += e; } return sum; => 21
> > > > MethodHandle loop =
> > > > MethodHandles.iteratedLoop(Iterate.MH_sumIterator,
> > Iterate.MH_sumInit,
> > > > Iterate.MH_sumStep);
> > > > assertEquals(Iterate.MT_sum, loop.type());
> > > > assertEquals(21, loop.invoke(new Integer[]{1, 2, 3, 4, 5, 6}));
> > > > }
> > > >
> > > > ... where MH_sumIterator is a handle to this method:
> > > >
> > > > static Iterator sumIterator(Integer[] a) {
> > > > return Arrays.asList(a).iterator();
> > > > }
> > >
> > > Of course this works, too.
> > >
> > > My proposal or question here was more about the API inconsistency in
> > general.
> > > You can do a lot here and there, you can implement effective or
> > > non-effective loops with MethodHandles, but still the following holds
> true:
> > > java.lang.invoke.MethodHandles is missing an accessor for
> "array.length",
> > > which is not understandable. The "countedLoop" here was just an
> example
> > use
> > > case (if that one is great or not does not matter), it was just posted to
> > > actually show the API and how it *could* be used.
> > >
> > > The real example from our script engine was not related to loops, it was
> an
> > > actual invokedynamic callsite implementation where it was not possible
> to
> > > figure out how to get a method handle to the very special arraylength
> byte
> > > code, aka "array.length javac sugar". Everything else to handle arrays is
> > > there at one single place, but the array length is missing and you get
> > > very
> > > annoyed. If you are not so familiar to bytecode and the mechanics
> behind,
> > > you have a very hard time to find out how to do this: the intuitive
> > > solutions does not work: "array.length" -> aha it’s a field, so let's try
> > > Lookup.findGetter(long[].class, "length", int.class) -> does not work!
> > >
> > > What is so hard to accept the proposal to have
> > > MethodHandles.arrayLengthGetter, implemented in the most effective
> > way (as a
> > > methodhandle that solely calls the special bytecode "arraylength")? In
> fact
> > > thais is like a 10 liner to implement + Javadocs. No need to delay a
> > > release, there are still changes in JDK 9 going on that are far more risky
> > > than this.
> > >
> > > Best,
> > > Uwe
> > >
> > > > Best,
> > > >
> > > > Michael
> > > >
> > > > --
> > > >
> > > >
Re: RFR: 8147588: Jar file and Zip file not removed in spite of the OPEN_DELETE flag
On 12/05/2016 20:16, Xueming Shen wrote: Hi, Please help review the proposed change for #8147588 issue: https://bugs.openjdk.java.net/browse/JDK-8147588 webrev: http://cr.openjdk.java.net/~sherman/8147588/webrev This is a regression on Windows platform triggered by the change for https://bugs.openjdk.java.net/browse/JDK-8145260, in which we brought the ZipFile native natively code to java for better performance (both memory and access). As showed the from ln#107-114 at webrev below http://cr.openjdk.java.net/~sherman/8145260/webrev.push/src/java.base/share/native/libzip/ZipFile.c-.html The original native implementation uses a special "Windows-only" option O_TEMPORARY to open the zip/jar file on Windows to support this OPEN_DELETE functionality. There is no corresponding public open/delete functionality at java.io API right now. The proposed the change here is to add a private constructor for RandomAccessFile and access from ZipFile via SharedSecrets. This O_TEMPORARY is only supported on Windows platform. I assume O_TEMPORARY maps to Windows FILE_FLAG_DELETE_ON_CLOSE flag. In the new file system API then this gets exposed as the DELETE_ON_CLOSE open option but it isn't exposed via RAF. I assume it would be too much of a change to move ZipFile, particularly with behavior changes related to interruption. I've looked through the patch but I just wondering if there are cleaner alternatives. What would you think about not pushing O_TEMPORARY to RAF but instead have ZipFile open the zip file with ZFILE_Open, wrap it with a FileDescriptor and then create a RAF to that FileDescriptor. I realize RAF doesn't have a public constructor for this so it needs plumbing but the nice this is that it keeps this specific mode issue out of RAF. -Alan
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Note that there is a possible initialization circularity introduced by
this patch, which I think is harmless because:
- ThreadLocalRandom has just recently been enhanced to cope with such
situations - CHM needs ThreadLocalRandom in put() for example, when new
entry is being inserted, TLR invokes
Boolean.getBoolean("java.util.secureRandomSeed") as the last thing in
its static initializer but still works correctly even before this last
part of initializer is finished).
- When an invocation of Boolean.getBoolean ends up in the same
Properties instance (the System.getProperties()) it might be asking for
an entry in the same CHM instance (CHM.get()) which is just being
modified by put(). This amounts to re-entrance of CHM instance, but is
harmless as Boolean.getBoolean is only possibly invoked as the last
thing of various CHM modifications methods when the CHM state is already
consistent (see addCount() invocations in CHM).
Regards, Peter
On 05/13/2016 01:55 AM, Brent Christian wrote:
Update to the test, and additional comments:
http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/
Thanks,
-Brent
On 5/12/16 4:15 PM, Mandy Chung wrote:
On May 12, 2016, at 2:44 PM, Brent Christian
wrote:
On 5/12/16 11:44 AM, Mandy Chung wrote:
This patch looks good.
To help future readers to understand this, it may be better to move:
1152 private transient ConcurrentHashMap map =
1153 new ConcurrentHashMap<>(8);
to the beginning and add a comment describing what are lock-free
and what are synchronized (basically some part of your summary below).
Also a comment in Hashtable::defaultWriteHashtable and
readHashtable methods to mention that they are overridden by
Properties.
Good ideas.
In the GetResource.java test, what is the reason taking out:
73 URL u2 =
Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
It was coming back null and failing the test, I think because
Jigsaw moved CalendarData into a different module (jdk.localedata, I
believe?).
src/java.base/share/classes/sun/util/resources/CalendarData.properties
is still in java.base. This is due to resource encapsulation.
Unnamed module calling ClassLoader::getResource of a resource in a
named module returns null.
I fiddled with @modules a bit, but stopped when I discovered that
when the test deadlocks, it hangs on the first call to getResource()
and doesn't get to this line.
I can look into getting it to load CalendarData again, if you like.
The launcher and builtin class loader implementation has changed so
much that the code path exercised JDK-6977738 no longer exists. I
think dropping line 73 is okay.
Mandy
Re: RFR[9]:Fix java/lang/invoke/MethodHandleImpl's use of Unsafe.defineAnonymousClass()
MethodHandle vamh = prepareForInvoker(MH_checkCallerClass);
Object ok = bccInvoker.invokeExact(vamh, new Object[]{hostClass, bcc});
+ assert Boolean.TRUE.equals(ok) : ok;
What I meant is to convert the whole test (inside try-catch block) into
an assert.
+UNSAFE.ensureClassInitialized(bcc);
Do people see any reason to force invoker class init? I'd prefer to see
it goes away.
Also, as a second thought, generateInvokerTemplate() looks clearer than
invokerTemplateGenerator().
Something like the following:
http://cr.openjdk.java.net/~vlivanov/8149574/webrev.00/
Additional cleanup: checkCallerClass() should return injected invoker
class when invoked using a method handle, so no need in 2nd argument.
Best regards,
Vladimir Ivanov
On 5/13/16 11:56 AM, shilpi.rast...@oracle.com wrote:
Thanks Paul!
Please review http://cr.openjdk.java.net/~srastogi/8149574/webrev10.0/
Regards,
Shilpi
On 5/13/2016 2:09 PM, Paul Sandoz wrote:
assert Boolean.TRUE.equals(ok) : ok;
Re: RFR[9]:Fix java/lang/invoke/MethodHandleImpl's use of Unsafe.defineAnonymousClass()
Also instead of
MethodVisitor mv = cw.visitMethod(ACC_PRIVATE | ACC_STATIC, "invoke_V",
"(Ljava/lang/invoke/MethodHandle;[Ljava/lang/Object;)Ljava/lang/Object;",
null, new String[] { "java/lang/Throwable" });
because the code is never read by a java compiler (as javac), you don't need to
specify the exception
(checked exceptions are a Java the language artifact)
MethodVisitor mv = cw.visitMethod(ACC_PRIVATE | ACC_STATIC, "invoke_V",
"(Ljava/lang/invoke/MethodHandle;[Ljava/lang/Object;)Ljava/lang/Object;",
null, null);
my 2 cents,
Rémi
- Mail original -
> De: "Vladimir Ivanov"
> À: "shilpi rastogi"
> Cc: core-libs-dev@openjdk.java.net
> Envoyé: Vendredi 13 Mai 2016 15:41:33
> Objet: Re: RFR[9]:Fix java/lang/invoke/MethodHandleImpl's use of
> Unsafe.defineAnonymousClass()
>
> MethodHandle vamh = prepareForInvoker(MH_checkCallerClass);
> Object ok = bccInvoker.invokeExact(vamh, new Object[]{hostClass, bcc});
> + assert Boolean.TRUE.equals(ok) : ok;
>
> What I meant is to convert the whole test (inside try-catch block) into
> an assert.
>
>
> +UNSAFE.ensureClassInitialized(bcc);
>
> Do people see any reason to force invoker class init? I'd prefer to see
> it goes away.
>
> Also, as a second thought, generateInvokerTemplate() looks clearer than
> invokerTemplateGenerator().
>
> Something like the following:
>http://cr.openjdk.java.net/~vlivanov/8149574/webrev.00/
>
> Additional cleanup: checkCallerClass() should return injected invoker
> class when invoked using a method handle, so no need in 2nd argument.
>
> Best regards,
> Vladimir Ivanov
>
> On 5/13/16 11:56 AM, shilpi.rast...@oracle.com wrote:
> > Thanks Paul!
> >
> > Please review http://cr.openjdk.java.net/~srastogi/8149574/webrev10.0/
> >
> > Regards,
> > Shilpi
> >
> > On 5/13/2016 2:09 PM, Paul Sandoz wrote:
> >> assert Boolean.TRUE.equals(ok) : ok;
> >
>
Re: RFR[9]:Fix java/lang/invoke/MethodHandleImpl's use of Unsafe.defineAnonymousClass()
Good point, Remi. I agree that it's redundant.
Best regards,
Vladimir Ivanov
On 5/13/16 4:51 PM, Remi Forax wrote:
Also instead of
MethodVisitor mv = cw.visitMethod(ACC_PRIVATE | ACC_STATIC, "invoke_V",
"(Ljava/lang/invoke/MethodHandle;[Ljava/lang/Object;)Ljava/lang/Object;",
null, new String[] { "java/lang/Throwable" });
because the code is never read by a java compiler (as javac), you don't need to
specify the exception
(checked exceptions are a Java the language artifact)
MethodVisitor mv = cw.visitMethod(ACC_PRIVATE | ACC_STATIC, "invoke_V",
"(Ljava/lang/invoke/MethodHandle;[Ljava/lang/Object;)Ljava/lang/Object;",
null, null);
my 2 cents,
Rémi
- Mail original -
De: "Vladimir Ivanov"
À: "shilpi rastogi"
Cc: core-libs-dev@openjdk.java.net
Envoyé: Vendredi 13 Mai 2016 15:41:33
Objet: Re: RFR[9]:Fix java/lang/invoke/MethodHandleImpl's use of
Unsafe.defineAnonymousClass()
MethodHandle vamh = prepareForInvoker(MH_checkCallerClass);
Object ok = bccInvoker.invokeExact(vamh, new Object[]{hostClass, bcc});
+ assert Boolean.TRUE.equals(ok) : ok;
What I meant is to convert the whole test (inside try-catch block) into
an assert.
+UNSAFE.ensureClassInitialized(bcc);
Do people see any reason to force invoker class init? I'd prefer to see
it goes away.
Also, as a second thought, generateInvokerTemplate() looks clearer than
invokerTemplateGenerator().
Something like the following:
http://cr.openjdk.java.net/~vlivanov/8149574/webrev.00/
Additional cleanup: checkCallerClass() should return injected invoker
class when invoked using a method handle, so no need in 2nd argument.
Best regards,
Vladimir Ivanov
On 5/13/16 11:56 AM, shilpi.rast...@oracle.com wrote:
Thanks Paul!
Please review http://cr.openjdk.java.net/~srastogi/8149574/webrev10.0/
Regards,
Shilpi
On 5/13/2016 2:09 PM, Paul Sandoz wrote:
assert Boolean.TRUE.equals(ok) : ok;
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Good idea.
Mandy
> On May 13, 2016, at 12:10 AM, Peter Levart wrote:
>
> Hi Brent,
>
>
> This looks good. It might also be a good idea to add a test that checks this
> new implementation detail (the unsynchronized get) that new code will become
> dependent upon, for example:
>
>
> /*
> * @test
> * @bug 8029891
> * @summary Test that the Properties.get() method does not synchronize any more
> * @run main CheckUnsynchronizedGet
> */
> public class CheckUnsynchronizedGet {
>
>public static void main(String[] args) {
>Properties props = new Properties();
>synchronized (props) {
>props.setProperty("key", "value");
>String value =
>CompletableFuture.supplyAsync(() ->
> props.getProperty("key")).join();
>assert "value".equals(value);
>}
>}
> }
>
> What do you think?
>
> Regards, Peter
>
> On 05/13/2016 01:55 AM, Brent Christian wrote:
>> Update to the test, and additional comments:
>>
>> http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/
>>
>> Thanks,
>> -Brent
>>
>> On 5/12/16 4:15 PM, Mandy Chung wrote:
>>>
On May 12, 2016, at 2:44 PM, Brent Christian
wrote:
On 5/12/16 11:44 AM, Mandy Chung wrote:
>
> This patch looks good.
>
> To help future readers to understand this, it may be better to move:
> 1152 private transient ConcurrentHashMap map =
> 1153 new ConcurrentHashMap<>(8);
>
> to the beginning and add a comment describing what are lock-free and what
> are synchronized (basically some part of your summary below).
>
> Also a comment in Hashtable::defaultWriteHashtable and readHashtable
> methods to mention that they are overridden by Properties.
Good ideas.
> In the GetResource.java test, what is the reason taking out:
> 73 URL u2 =
> Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
>
It was coming back null and failing the test, I think because
Jigsaw moved CalendarData into a different module (jdk.localedata, I
believe?).
>>>
>>> src/java.base/share/classes/sun/util/resources/CalendarData.properties is
>>> still in java.base. This is due to resource encapsulation. Unnamed module
>>> calling ClassLoader::getResource of a resource in a named module returns
>>> null.
>>>
>>>
I fiddled with @modules a bit, but stopped when I discovered that when
the test deadlocks, it hangs on the first call to getResource() and
doesn't get to this line.
I can look into getting it to load CalendarData again, if you like.
>>>
>>> The launcher and builtin class loader implementation has changed so much
>>> that the code path exercised JDK-6977738 no longer exists. I think
>>> dropping line 73 is okay.
>>>
>>> Mandy
>>>
>
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
> On May 12, 2016, at 4:55 PM, Brent Christian > wrote: > > Update to the test, and additional comments: > > http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/ > +1 Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
> On May 12, 2016, at 5:58 PM, David Holmes wrote: >> >> With all of the inherited methods @hidden, it looks like that section >> is left out altogether. > > Okay, so I have to say @hidden seems to me to be seriously flawed! If a class > has a method that can be called then IMHO that has to be documented in that > class as either being first defined, overridden, or inherited - it can't just > say nothing as-if the method were not there! > > If we are overriding in a trivial way that does not change the specification > at all then there should be a simple way to show that - perhaps the "Methods > inherited from ..." should be split into two parts: method implementations > inherited from ..., and method specifications inherited from ... ?? > I agree for this case these methods should not be hidden as if it’s not there (that’s probably carried from the original @treatAsPrivate request). > I guess I need to raise this with the javadoc folk :( Is there a mailing list > for that? Can you file a JBS issue? Kumar is on vacation and will talk with him when he’s back. Mandy
Re: JDK 9 RFR of 8130679: Writer/StringWriter.write methods do not specify index out bounds
Hello,
I have a third version of the patch here:
http://cr.openjdk.java.net/~bpb/8130679/webrev.00/
This one I believe addresses all the inconsistencies including the one in
https://bugs.openjdk.java.net/browse/JDK-8029804 at the expense of a slight
weakening of the specification of the Writer.write({char[],String},int,int)
methods, and matches the behavior of all the classes affected. I have tested it
and inspected the code extensively.
Thanks,
Brian
On May 10, 2016, at 8:11 AM, Brian Burkhalter
wrote:
> On May 10, 2016, at 8:03 AM, Roger Riggs wrote:
>
>> Good catch, the javadoc needs to match the code and in this case the new
>> language
>> does not match the code. Some cases are harder to follow because they
>> delegate
>> to other classes for copying.
>
> I think I’ll drop BufferedWriter from this RFR and let Pavel handle it in
> 8029804.
>
>> It may also be the case that since Writer.write(char[], off, len) is
>> abstract, it cannot enforce the contract
>> consistently on all subclasses and something special will be needed for that
>> javadoc.
>
> I’ll see whether this needs changing before revving.
Re: JDK 9 RFR of 8130679: Writer/StringWriter.write methods do not specify index out bounds
Correction: wrong link provided below:
http://cr.openjdk.java.net/~bpb/8130679/webrev.02/
Sorry for the confusion.
Brian
On May 13, 2016, at 10:42 AM, Brian Burkhalter
wrote:
> I have a third version of the patch here:
>
> http://cr.openjdk.java.net/~bpb/8130679/webrev.00/
>
> This one I believe addresses all the inconsistencies including the one in
> https://bugs.openjdk.java.net/browse/JDK-8029804 at the expense of a slight
> weakening of the specification of the Writer.write({char[],String},int,int)
> methods, and matches the behavior of all the classes affected. I have tested
> it and inspected the code extensively.
Re: JDK 9 RFR of 8130679: Writer/StringWriter.write methods do not specify index out bounds
Brian,
This looks good!
Could you please fix this tiny typo in-place?
145 * @throws IOException if the pipe is
146 * broken},
^^^
And one more thing. You've removed this `@see` indentation in Writer.java.
(Surprisingly enough, some views generated by webrev, e.g. sdiff, do not show
it!)
36 * @see BufferedWriter
37 * @see CharArrayWriter
38 * @see FilterWriter
39 * @see OutputStreamWriter
> 40 * @see FileWriter
41 * @see PipedWriter
42 * @see PrintWriter
43 * @see StringWriter
44 * @see Reader
I believe it was intentional. It's a cute way to show the tree of inheritance.
Compare it with java.io.Reader:
* @see BufferedReader
* @see LineNumberReader
* @see CharArrayReader
* @see InputStreamReader
* @see FileReader
* @see FilterReader
* @see PushbackReader
* @see PipedReader
* @see StringReader
* @see Writer
Thanks.
> On 13 May 2016, at 19:04, Brian Burkhalter
> wrote:
>
> Correction: wrong link provided below:
>
> http://cr.openjdk.java.net/~bpb/8130679/webrev.02/
>
> Sorry for the confusion.
>
> Brian
>
> On May 13, 2016, at 10:42 AM, Brian Burkhalter
> wrote:
>
>> I have a third version of the patch here:
>>
>> http://cr.openjdk.java.net/~bpb/8130679/webrev.00/
>>
>> This one I believe addresses all the inconsistencies including the one in
>> https://bugs.openjdk.java.net/browse/JDK-8029804 at the expense of a slight
>> weakening of the specification of the Writer.write({char[],String},int,int)
>> methods, and matches the behavior of all the classes affected. I have tested
>> it and inspected the code extensively.
>
Re: JDK 9 RFR of 8130679: Writer/StringWriter.write methods do not specify index out bounds
Hi Pavel, On May 13, 2016, at 11:25 AM, Pavel Rappo wrote: > This looks good! Good good! Thanks! > > Could you please fix this tiny typo in-place? > > 145 * @throws IOException if the pipe is > 146 * href=PipedOutputStream.html#BROKEN>broken}, > > ^^^ OK > And one more thing. You've removed this `@see` indentation in Writer.java. > (Surprisingly enough, some views generated by webrev, e.g. sdiff, do not show > it!) > > 36 * @see BufferedWriter > 37 * @see CharArrayWriter > 38 * @see FilterWriter > 39 * @see OutputStreamWriter >> 40 * @see FileWriter > 41 * @see PipedWriter > 42 * @see PrintWriter > 43 * @see StringWriter > 44 * @see Reader > > I believe it was intentional. It's a cute way to show the tree of inheritance. > Compare it with java.io.Reader: > > * @see BufferedReader > * @see LineNumberReader > * @see CharArrayReader > * @see InputStreamReader > * @see FileReader > * @see FilterReader > * @see PushbackReader > * @see PipedReader > * @see StringReader > * @see Writer That cuteness was lost on me; I’ll reinstate it. Brian
Re: JDK 9 RFR of 8130679: Writer/StringWriter.write methods do not specify index out bounds
Hi Brian, Looks fine. Thanks for digging through all the cases. The @see indenting is ok, but not really a thing. White space is should not be used/relied upon for formatting. Roger On 5/13/2016 2:34 PM, Brian Burkhalter wrote: Hi Pavel, On May 13, 2016, at 11:25 AM, Pavel Rappo wrote: This looks good! Good good! Thanks! Could you please fix this tiny typo in-place? 145 * @throws IOException if the pipe is 146 * broken}, ^^^ OK And one more thing. You've removed this `@see` indentation in Writer.java. (Surprisingly enough, some views generated by webrev, e.g. sdiff, do not show it!) 36 * @see BufferedWriter 37 * @see CharArrayWriter 38 * @see FilterWriter 39 * @see OutputStreamWriter 40 * @see FileWriter 41 * @see PipedWriter 42 * @see PrintWriter 43 * @see StringWriter 44 * @see Reader I believe it was intentional. It's a cute way to show the tree of inheritance. Compare it with java.io.Reader: * @see BufferedReader * @see LineNumberReader * @see CharArrayReader * @see InputStreamReader * @see FileReader * @see FilterReader * @see PushbackReader * @see PipedReader * @see StringReader * @see Writer That cuteness was lost on me; I’ll reinstate it. Brian
Re: RFR: 8147588: Jar file and Zip file not removed in spite of the OPEN_DELETE flag
On 5/13/16 3:27 AM, Alan Bateman wrote: On 12/05/2016 20:16, Xueming Shen wrote: Hi, Please help review the proposed change for #8147588 issue: https://bugs.openjdk.java.net/browse/JDK-8147588 webrev: http://cr.openjdk.java.net/~sherman/8147588/webrev This is a regression on Windows platform triggered by the change for https://bugs.openjdk.java.net/browse/JDK-8145260, in which we brought the ZipFile native natively code to java for better performance (both memory and access). As showed the from ln#107-114 at webrev below http://cr.openjdk.java.net/~sherman/8145260/webrev.push/src/java.base/share/native/libzip/ZipFile.c-.html The original native implementation uses a special "Windows-only" option O_TEMPORARY to open the zip/jar file on Windows to support this OPEN_DELETE functionality. There is no corresponding public open/delete functionality at java.io API right now. The proposed the change here is to add a private constructor for RandomAccessFile and access from ZipFile via SharedSecrets. This O_TEMPORARY is only supported on Windows platform. I assume O_TEMPORARY maps to Windows FILE_FLAG_DELETE_ON_CLOSE flag. In the new file system API then this gets exposed as the DELETE_ON_CLOSE open option but it isn't exposed via RAF. I assume it would be too much of a change to move ZipFile, particularly with behavior changes related to interruption. I've looked through the patch but I just wondering if there are cleaner alternatives. What would you think about not pushing O_TEMPORARY to RAF but instead have ZipFile open the zip file with ZFILE_Open, wrap it with a FileDescriptor and then create a RAF to that FileDescriptor. I realize RAF doesn't have a public constructor for this so it needs plumbing but the nice this is that it keeps this specific mode issue out of RAF. something like this? http://cr.openjdk.java.net/~sherman/8147588/webrev/ -sherman
Re: JDK 9 RFR of 8130679: Writer/StringWriter.write methods do not specify index out bounds
Hi Roger, On May 13, 2016, at 11:44 AM, Roger Riggs wrote: > Looks fine. Thanks for digging through all the cases. Thanks & you’re welcome. > The @see indenting is ok, but not really a thing. White space is should not > be used/relied upon for formatting. The public javadoc http://download.java.net/java/jdk9/docs/api/java/io/Writer.html just shows the @see contents as one wrapped line. It seemed to me that to have “@see Writer” in the Writer class itself was unnecessary given the lack of formatting as it merely points back to the same class. Brian
Re: JDK 9 RFR of 8130679: Writer/StringWriter.write methods do not specify index out bounds
On 13 May 2016, at 19:04, Brian Burkhalter wrote:
> Correction: wrong link provided below:
>
> http://cr.openjdk.java.net/~bpb/8130679/webrev.02/
Looks good to me Brian.
-Chris.
> Sorry for the confusion.
>
> Brian
>
> On May 13, 2016, at 10:42 AM, Brian Burkhalter
> wrote:
>
>> I have a third version of the patch here:
>>
>> http://cr.openjdk.java.net/~bpb/8130679/webrev.00/
>>
>> This one I believe addresses all the inconsistencies including the one in
>> https://bugs.openjdk.java.net/browse/JDK-8029804 at the expense of a slight
>> weakening of the specification of the Writer.write({char[],String},int,int)
>> methods, and matches the behavior of all the classes affected. I have tested
>> it and inspected the code extensively.
>
Re: RFR: 8147588: Jar file and Zip file not removed in spite of the OPEN_DELETE flag
On 13/05/2016 19:49, Xueming Shen wrote: something like this? http://cr.openjdk.java.net/~sherman/8147588/webrev/ Along these lines, yes. Would it be cleaner if ZipFile_openAndDelete were rename to ZipFile_open and used ZFILE_Open in libzip instead of winFileHandleOpen in libjava? Also given the platforms and modes then would it be simpler to use use ZFILE_Open for all platforms/cases. Should JavaIORandomAccessFileAccess.open be renamed to wrap as it does not open the file? -Alan
Re: RFR: 8147588: Jar file and Zip file not removed in spite of the OPEN_DELETE flag
On 5/13/16 1:24 PM, Alan Bateman wrote: On 13/05/2016 19:49, Xueming Shen wrote: something like this? http://cr.openjdk.java.net/~sherman/8147588/webrev/ Along these lines, yes. Would it be cleaner if ZipFile_openAndDelete were rename to ZipFile_open and used ZFILE_Open in libzip instead of winFileHandleOpen in libjava? Also given the platforms and modes then would it be simpler to use use ZFILE_Open for all platforms/cases. My memory suggests the reason we went to winFileHandleOpen() is for that windows' long path issue we dealt with years ago. ZFILE_Open() does not appear to deal with that. Sure I can bring ZFILE_Open in for the non-windows platform, though my original wish is to reduce the native exposure of the ZipFile. I don't have a strong feeling for that though. Should JavaIORandomAccessFileAccess.open be renamed to wrap as it does not open the file? It means to "open" an RandomAccessFile object, how about "create"? "wrap" a little indicates the RandomAccessFile does not nothing but only a wrapper around the "fd". -Sherman
RFR: 8144062: Move jdk.Version to java.lang.Runtime.Version
Hi. Reviving this work from a few months back. Please review the following changes to move jdk.Version to jdk.lang.Runtime.Version. Bug 8144062: Move jdk.Version to java.lang.Runtime.Version https://bugs.openjdk.java.net/browse/JDK-8144062 webrev http://cr.openjdk.java.net/~iris/verona/8144062/webrev.1/ When jdk.Version was initially pushed in jdk-9+1-5, it was Improperly exported by java.base. After exploring a few options, the best choice was to move jdk.Version to java.lang.Runtime.Version (a nested class of Runtime). By making Version an SE API, it may be exported by the java.base module. As part of the move, a limited number of chnages were made to the Version class: - Change package name and class declaration (to static) - Eliminate use of "JDK" when describing a Java SE API - Initial clarifications related to zeros (trailing vs. Internal components) - Small typographical and grammatical enhancements - Indentation The complete Runtime.Version specification is available here: http://cr.openjdk.java.net/~iris/verona/8144062/doc.1/java/lang/Runtime.Version.html The old jdk.Version.current() was replaced with Runtime.version(). In System.getProperties(), we indicate which version-related system properties may be supported by Runtime.Version. The remaining jdk and langtools file changes are all side-effects of changing jdk.Version.current() to Runtime.version(). Thanks, Iris
Re: RFR: 8144062: Move jdk.Version to java.lang.Runtime.Version
Hi Iris, is there a way to avoid to use regex when parsing the version ? java.lang.Runtime.Version is used during the boot process, so now every Java programs loads a bunch of classes related to java.util.regex even if they do not use any regex or use another regex engine (like Nashorn or JRuby). regards, Rémi - Mail original - > De: "Iris Clark" > À: "Java Core Libs" , > compiler-...@openjdk.java.net > Cc: verona-...@openjdk.java.net > Envoyé: Samedi 14 Mai 2016 01:20:23 > Objet: RFR: 8144062: Move jdk.Version to java.lang.Runtime.Version > > Hi. > > Reviving this work from a few months back. > > Please review the following changes to move jdk.Version to > java.lang.Runtime.Version. > > Bug > > 8144062: Move jdk.Version to java.lang.Runtime.Version > https://bugs.openjdk.java.net/browse/JDK-8144062 > > webrev > > http://cr.openjdk.java.net/~iris/verona/8144062/webrev.1/ > > When jdk.Version was initially pushed in jdk-9+1-5, it was > Improperly exported by java.base. After exploring a few > options, the best choice was to move jdk.Version to > java.lang.Runtime.Version (a nested class of Runtime). By > making Version an SE API, it may be exported by the java.base > module. > > As part of the move, a limited number of chnages were > made to the Version class: > > - Change package name and class declaration (to static) > - Eliminate use of "JDK" when describing a Java SE API > - Initial clarifications related to zeros (trailing vs. > Internal components) > - Small typographical and grammatical enhancements > - Indentation > > The complete Runtime.Version specification is available here: > > > http://cr.openjdk.java.net/~iris/verona/8144062/doc.1/java/lang/Runtime.Version.html > > The old jdk.Version.current() was replaced with > Runtime.version(). > > In System.getProperties(), we indicate which version-related > system properties may be supported by Runtime.Version. > > The remaining jdk and langtools file changes are all > side-effects of changing jdk.Version.current() to > Runtime.version(). > > Thanks, > Iris >
Re: Review request for JDK-8156575: Add jdeps -addmods, -system, -inverse options
Daniel, I have added more tests and fixed a few issues http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8156575/webrev.02/ I also created a test case similar to yours. This patch applies on top of: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8156680/webrev.02/ for JDK-8156680: jdeps implementation refresh Thanks Mandy
