JDK 9 RFR of JDK-8157006/8157011: Problem list java/io/pathNames/GeneralWin32.java and tools/pack200/TestNormal.java
java/io/pathNames/GeneralWin32.java has been seen to fail with high frequency on windows. Until JDK-8156595 is addressed, the test should be problem listed. bug: https://bugs.openjdk.java.net/browse/JDK-8157006 tools/pack200/TestNormal.java has been seen to fail with some frequency on windows. Until JDK-8156807 is addressed, the test should be problem listed. bug: https://bugs.openjdk.java.net/browse/JDK-8157011 webrev: http://cr.openjdk.java.net/~mli/8157006/webrev.00/ Thank you -Hamlin
Re: Cleaner cleanup
Hey Peter,
First of all, I really love this Cleanup API!
Anyhow I think it would make sense to have Cleanable extend AutoCloseable for
exactly the use case you rendered in your JavaDoc of the Cleaner class.
Made up example:
public T execute(BufferAction bufferAction) {
Buffer buffer = allocateBuffer();
try (Cleanable wrapper = cleaner.register(buffer, Buffer::deallocate) {
bufferAction.execute(buffer);
}
}
I agree that you probably want to encapsulate the cleaner registration and
cleaning action but for frameworks it might be beneficial to have a common
utility method implementing a pattern such as the one above.
Chris
> On 16 May 2016, at 00:08, Peter Levart wrote:
>
> Hi Roger and others,
>
> When the new Cleaner API was created the implementation of Cleanable(s) was
> split into the low-level abstract [Soft|Weak|Phantom]Cleanable classes to be
> used internally for purposes where the footprint matters and their
> corresponding CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses used as
> implementations that take a Runnable cleanup action and are exposed via the
> public Cleaner API.
>
> When thinking of possible JDK internal use cases for the low-level API, I
> came to the conclusion that [Soft|Weak|Phantom]Cleanable classes are not
> suitable as is, because in cases where footprint matters, it is usually also
> the case that the number of [Soft|Weak|Phantom]Cleanable instances created is
> larger and that construction performance also matters. Especially
> multi-threaded construction. I'm thinking of the use cases of auto-cleanable
> concurrent data structures. In such use cases, the present features of
> [Soft|Weak|Phantom]Cleanable classes, namely the guaranteed just-once cleanup
> action invocation and keeping the Cleanable instance reachable until the
> cleanup action is performed, are actually not needed and just present
> footprint and performance (contention) overhead. They also present an
> overhead as they don't allow GC to automatically collect the Cleanable
> instances if the data structure containing them becomes unreachable and
> corresponding registered cleanup actions obsolete.
>
> The mentioned features are important for public Cleaner.Cleanable instances
> as they are usually used for cleanup of native resources where the
> performance of their creation is not so drastically important and where there
> is no intrinsic data structure to hold them reachable.
>
> I propose to move those features from the [Soft|Weak|Phantom]Cleanable
> classes down the hierarchy to the CleanerImpl.[Soft|Weak|Phantom]CleanableRef
> subclasses:
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Cleaner.cleanup/webrev.01/
>
>
> In this change I also removed the CleanerImpl.[Soft|Weak]CleanableRef
> subclasses as they are not needed and I believe will never be. I also renamed
> the CleanerImpl.PhantomCleanableRef subclass to
> CleanerImpl.PhantomCleanableImpl.
>
> Changes to the implementation are straightforward. The most work was put into
> the corresponding test. I did some clean-up to it and also changed it to
> accommodate for the new behavior of [Soft|Weak|Phantom]Cleanable classes. The
> changes speak for itself. One of the not-so obvious changes was to replace
> the CleanableCase.clearRef() action with the CleanableCase.releaseReferent()
> action. The old clearRef() action did not serve any purpose. Whether this
> method was called or not, the behavior of the corresponding Cleanable was
> unchanged as the Reference instance (referenced from the 'ref' field) was
> always of the same strength as the Cleanable itself. So clearing it could not
> affect the behavior of the Cleanable.
>
> I changed 'ref' to hold a direct reference to the referent and renamed the
> field to 'referent'. I changed the EV_XXX int constants to Event enum
> constants with helper methods used in CleanableCase.expectedCleanups() method
> that now returns the number of expected cleanup invocations - in the
> PhantomCleanableImpl case this is the number of expected cleanup action
> invocations while in the plain XxxCleanable subclass cases it is the number
> of Cleanable.clean() method invocations. I added the no-actions case to both
> PhantomCleanableImpl and XxxCleanable cases and extended the number and
> combinations of XxxCleanable cases.
>
> The checkCleaned() method was extended to verify that the number of cleanup
> invocations is *no more* and no less then the expected.
>
> See how WeakKey test is now simplified. This is the typical use-case for
> WeakCleanable I was talking about.
>
>
> So, what do you think of this cleanup?
>
>
> Regards, Peter
>
Re: Cleaner cleanup
Peter,
Just as a quick correction of the previous example, obviously
Buffer::deallocate wouldn’t work, but I guess you get the point.
Chris
> On 16 May 2016, at 10:37, Christoph Engelbert wrote:
>
> Hey Peter,
>
> First of all, I really love this Cleanup API!
> Anyhow I think it would make sense to have Cleanable extend AutoCloseable for
> exactly the use case you rendered in your JavaDoc of the Cleaner class.
>
> Made up example:
> public T execute(BufferAction bufferAction) {
> Buffer buffer = allocateBuffer();
> try (Cleanable wrapper = cleaner.register(buffer, Buffer::deallocate) {
>bufferAction.execute(buffer);
> }
> }
>
> I agree that you probably want to encapsulate the cleaner registration and
> cleaning action but for frameworks it might be beneficial to have a common
> utility method implementing a pattern such as the one above.
>
> Chris
>
>> On 16 May 2016, at 00:08, Peter Levart wrote:
>>
>> Hi Roger and others,
>>
>> When the new Cleaner API was created the implementation of Cleanable(s) was
>> split into the low-level abstract [Soft|Weak|Phantom]Cleanable classes to be
>> used internally for purposes where the footprint matters and their
>> corresponding CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses used as
>> implementations that take a Runnable cleanup action and are exposed via the
>> public Cleaner API.
>>
>> When thinking of possible JDK internal use cases for the low-level API, I
>> came to the conclusion that [Soft|Weak|Phantom]Cleanable classes are not
>> suitable as is, because in cases where footprint matters, it is usually also
>> the case that the number of [Soft|Weak|Phantom]Cleanable instances created
>> is larger and that construction performance also matters. Especially
>> multi-threaded construction. I'm thinking of the use cases of auto-cleanable
>> concurrent data structures. In such use cases, the present features of
>> [Soft|Weak|Phantom]Cleanable classes, namely the guaranteed just-once
>> cleanup action invocation and keeping the Cleanable instance reachable until
>> the cleanup action is performed, are actually not needed and just present
>> footprint and performance (contention) overhead. They also present an
>> overhead as they don't allow GC to automatically collect the Cleanable
>> instances if the data structure containing them becomes unreachable and
>> corresponding registered cleanup actions obsolete.
>>
>> The mentioned features are important for public Cleaner.Cleanable instances
>> as they are usually used for cleanup of native resources where the
>> performance of their creation is not so drastically important and where
>> there is no intrinsic data structure to hold them reachable.
>>
>> I propose to move those features from the [Soft|Weak|Phantom]Cleanable
>> classes down the hierarchy to the
>> CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses:
>>
>> http://cr.openjdk.java.net/~plevart/jdk9-dev/Cleaner.cleanup/webrev.01/
>>
>>
>> In this change I also removed the CleanerImpl.[Soft|Weak]CleanableRef
>> subclasses as they are not needed and I believe will never be. I also
>> renamed the CleanerImpl.PhantomCleanableRef subclass to
>> CleanerImpl.PhantomCleanableImpl.
>>
>> Changes to the implementation are straightforward. The most work was put
>> into the corresponding test. I did some clean-up to it and also changed it
>> to accommodate for the new behavior of [Soft|Weak|Phantom]Cleanable classes.
>> The changes speak for itself. One of the not-so obvious changes was to
>> replace the CleanableCase.clearRef() action with the
>> CleanableCase.releaseReferent() action. The old clearRef() action did not
>> serve any purpose. Whether this method was called or not, the behavior of
>> the corresponding Cleanable was unchanged as the Reference instance
>> (referenced from the 'ref' field) was always of the same strength as the
>> Cleanable itself. So clearing it could not affect the behavior of the
>> Cleanable.
>>
>> I changed 'ref' to hold a direct reference to the referent and renamed the
>> field to 'referent'. I changed the EV_XXX int constants to Event enum
>> constants with helper methods used in CleanableCase.expectedCleanups()
>> method that now returns the number of expected cleanup invocations - in the
>> PhantomCleanableImpl case this is the number of expected cleanup action
>> invocations while in the plain XxxCleanable subclass cases it is the number
>> of Cleanable.clean() method invocations. I added the no-actions case to both
>> PhantomCleanableImpl and XxxCleanable cases and extended the number and
>> combinations of XxxCleanable cases.
>>
>> The checkCleaned() method was extended to verify that the number of cleanup
>> invocations is *no more* and no less then the expected.
>>
>> See how WeakKey test is now simplified. This is the typical use-case for
>> WeakCleanable I was talking about.
>>
>>
>> So, what do you think of this cleanup?
>>
>>
>>
Re: RFR[9]:Fix java/lang/invoke/MethodHandleImpl's use of Unsafe.defineAnonymousClass()
Looks good.
Best regards,
Vladimir Ivanov
PS: I noticed you missed ACC_PRIVATE flag on the invoker class, but then
erroneously put it on the method. Please, move it to class flags before
pushing. No need to send updated webrev.
+ cw.visit(52, ACC_PRIVATE | ACC_SUPER, "InjectedInvoker",
null, "java/lang/Object", null);
+
+ MethodVisitor mv = cw.visitMethod(ACC_STATIC, "invoke_V",
On 5/16/16 8:18 AM, shilpi.rast...@oracle.com wrote:
Thanks Vladimir and Remi.
Please review updated webrev
http://cr.openjdk.java.net/~srastogi/8149574/webrev/
Regards,
Shilpi
On 5/13/2016 7:31 PM, Vladimir Ivanov wrote:
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: Cleaner cleanup
Hi Christoph,
On 05/16/2016 10:37 AM, Christoph Engelbert wrote:
Hey Peter,
First of all, I really love this Cleanup API!
Anyhow I think it would make sense to have Cleanable extend AutoCloseable for
exactly the use case you rendered in your JavaDoc of the Cleaner class.
Made up example:
public T execute(BufferAction bufferAction) {
Buffer buffer = allocateBuffer();
try (Cleanable wrapper = cleaner.register(buffer, Buffer::deallocate) {
bufferAction.execute(buffer);
}
}
I agree that you probably want to encapsulate the cleaner registration and
cleaning action but for frameworks it might be beneficial to have a common
utility method implementing a pattern such as the one above.
Chris
I think the Cleaner API is more suitable to be used inside various
classes that themselves implement AutoCloseable rather than as an
(optional) aspect on top of them. So your example would simply read:
public T execute(BufferAction bufferAction) {
try (Buffer buffer = allocateBuffer()) {
bufferAction.execute(buffer);
}
}
There's no point in doing both things as in your example: using
try-with-resources + the Cleaner API on top. try-with-resources already
guarantees cleanup. Cleanable API is meant to be used inside otherwise
AutoCleanable implementations to shield the user from native resource
leaks if the user forgets to use or can't use try-with-resources.
Regards, Peter
On 16 May 2016, at 00:08, Peter Levart wrote:
Hi Roger and others,
When the new Cleaner API was created the implementation of Cleanable(s) was
split into the low-level abstract [Soft|Weak|Phantom]Cleanable classes to be
used internally for purposes where the footprint matters and their
corresponding CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses used as
implementations that take a Runnable cleanup action and are exposed via the
public Cleaner API.
When thinking of possible JDK internal use cases for the low-level API, I came
to the conclusion that [Soft|Weak|Phantom]Cleanable classes are not suitable as
is, because in cases where footprint matters, it is usually also the case that
the number of [Soft|Weak|Phantom]Cleanable instances created is larger and that
construction performance also matters. Especially multi-threaded construction.
I'm thinking of the use cases of auto-cleanable concurrent data structures. In
such use cases, the present features of [Soft|Weak|Phantom]Cleanable classes,
namely the guaranteed just-once cleanup action invocation and keeping the
Cleanable instance reachable until the cleanup action is performed, are
actually not needed and just present footprint and performance (contention)
overhead. They also present an overhead as they don't allow GC to automatically
collect the Cleanable instances if the data structure containing them becomes
unreachable and corresponding registered cleanup actions obsolete.
The mentioned features are important for public Cleaner.Cleanable instances as
they are usually used for cleanup of native resources where the performance of
their creation is not so drastically important and where there is no intrinsic
data structure to hold them reachable.
I propose to move those features from the [Soft|Weak|Phantom]Cleanable classes
down the hierarchy to the CleanerImpl.[Soft|Weak|Phantom]CleanableRef
subclasses:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Cleaner.cleanup/webrev.01/
In this change I also removed the CleanerImpl.[Soft|Weak]CleanableRef
subclasses as they are not needed and I believe will never be. I also renamed
the CleanerImpl.PhantomCleanableRef subclass to
CleanerImpl.PhantomCleanableImpl.
Changes to the implementation are straightforward. The most work was put into
the corresponding test. I did some clean-up to it and also changed it to
accommodate for the new behavior of [Soft|Weak|Phantom]Cleanable classes. The
changes speak for itself. One of the not-so obvious changes was to replace the
CleanableCase.clearRef() action with the CleanableCase.releaseReferent()
action. The old clearRef() action did not serve any purpose. Whether this
method was called or not, the behavior of the corresponding Cleanable was
unchanged as the Reference instance (referenced from the 'ref' field) was
always of the same strength as the Cleanable itself. So clearing it could not
affect the behavior of the Cleanable.
I changed 'ref' to hold a direct reference to the referent and renamed the
field to 'referent'. I changed the EV_XXX int constants to Event enum constants
with helper methods used in CleanableCase.expectedCleanups() method that now
returns the number of expected cleanup invocations - in the
PhantomCleanableImpl case this is the number of expected cleanup action
invocations while in the plain XxxCleanable subclass cases it is the number of
Cleanable.clean() method invocations. I added the no-actions case to both
PhantomCleanableImpl and XxxCleanable cases and extended the number and
combinations
Re: Cleaner cleanup
Correction in line...
On 05/16/2016 12:54 PM, Peter Levart wrote:
Hi Christoph,
On 05/16/2016 10:37 AM, Christoph Engelbert wrote:
Hey Peter,
First of all, I really love this Cleanup API!
Anyhow I think it would make sense to have Cleanable extend
AutoCloseable for exactly the use case you rendered in your JavaDoc
of the Cleaner class.
Made up example:
public T execute(BufferAction bufferAction) {
Buffer buffer = allocateBuffer();
try (Cleanable wrapper = cleaner.register(buffer,
Buffer::deallocate) {
bufferAction.execute(buffer);
}
}
I agree that you probably want to encapsulate the cleaner
registration and cleaning action but for frameworks it might be
beneficial to have a common utility method implementing a pattern
such as the one above.
Chris
I think the Cleaner API is more suitable to be used inside various
classes that themselves implement AutoCloseable rather than as an
(optional) aspect on top of them. So your example would simply read:
public T execute(BufferAction bufferAction) {
try (Buffer buffer = allocateBuffer()) {
bufferAction.execute(buffer);
}
}
There's no point in doing both things as in your example: using
try-with-resources + the Cleaner API on top. try-with-resources
already guarantees cleanup.
Sorry, the following:
*Cleanable* API is meant to be used inside otherwise *AutoCleanable*
implementations to shield the user from native resource leaks if the
user forgets to use or can't use try-with-resources.
Should read:
*Cleaner* API is meant to be used inside otherwise *AutoCloseable*
implementations to shield the user from native resource leaks if the
user forgets to use or can't use try-with-resources.
Regards, Peter
On 16 May 2016, at 00:08, Peter Levart wrote:
Hi Roger and others,
When the new Cleaner API was created the implementation of
Cleanable(s) was split into the low-level abstract
[Soft|Weak|Phantom]Cleanable classes to be used internally for
purposes where the footprint matters and their corresponding
CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses used as
implementations that take a Runnable cleanup action and are exposed
via the public Cleaner API.
When thinking of possible JDK internal use cases for the low-level
API, I came to the conclusion that [Soft|Weak|Phantom]Cleanable
classes are not suitable as is, because in cases where footprint
matters, it is usually also the case that the number of
[Soft|Weak|Phantom]Cleanable instances created is larger and that
construction performance also matters. Especially multi-threaded
construction. I'm thinking of the use cases of auto-cleanable
concurrent data structures. In such use cases, the present features
of [Soft|Weak|Phantom]Cleanable classes, namely the guaranteed
just-once cleanup action invocation and keeping the Cleanable
instance reachable until the cleanup action is performed, are
actually not needed and just present footprint and performance
(contention) overhead. They also present an overhead as they don't
allow GC to automatically collect the Cleanable instances if the
data structure containing them becomes unreachable and corresponding
registered cleanup actions obsolete.
The mentioned features are important for public Cleaner.Cleanable
instances as they are usually used for cleanup of native resources
where the performance of their creation is not so drastically
important and where there is no intrinsic data structure to hold
them reachable.
I propose to move those features from the
[Soft|Weak|Phantom]Cleanable classes down the hierarchy to the
CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Cleaner.cleanup/webrev.01/
In this change I also removed the
CleanerImpl.[Soft|Weak]CleanableRef subclasses as they are not
needed and I believe will never be. I also renamed the
CleanerImpl.PhantomCleanableRef subclass to
CleanerImpl.PhantomCleanableImpl.
Changes to the implementation are straightforward. The most work was
put into the corresponding test. I did some clean-up to it and also
changed it to accommodate for the new behavior of
[Soft|Weak|Phantom]Cleanable classes. The changes speak for itself.
One of the not-so obvious changes was to replace the
CleanableCase.clearRef() action with the
CleanableCase.releaseReferent() action. The old clearRef() action
did not serve any purpose. Whether this method was called or not,
the behavior of the corresponding Cleanable was unchanged as the
Reference instance (referenced from the 'ref' field) was always of
the same strength as the Cleanable itself. So clearing it could not
affect the behavior of the Cleanable.
I changed 'ref' to hold a direct reference to the referent and
renamed the field to 'referent'. I changed the EV_XXX int constants
to Event enum constants with helper methods used in
CleanableCase.expectedCleanups() method that now returns the number
of expected cleanup invocati
Re: Cleaner cleanup
Hey Peter,
Thanks for your answer. Actually you’re right, makes perfect sense. Somehow it
still feels wrong to implement the same pattern (inside lots of classes) over
and over again. Inheritance, on the other hand, is not always possible.
Chris
> On 16 May 2016, at 12:57, Peter Levart wrote:
>
> Correction in line...
>
> On 05/16/2016 12:54 PM, Peter Levart wrote:
>> Hi Christoph,
>>
>>
>> On 05/16/2016 10:37 AM, Christoph Engelbert wrote:
>>> Hey Peter,
>>>
>>> First of all, I really love this Cleanup API!
>>> Anyhow I think it would make sense to have Cleanable extend AutoCloseable
>>> for exactly the use case you rendered in your JavaDoc of the Cleaner class.
>>>
>>> Made up example:
>>> public T execute(BufferAction bufferAction) {
>>>Buffer buffer = allocateBuffer();
>>>try (Cleanable wrapper = cleaner.register(buffer, Buffer::deallocate) {
>>> bufferAction.execute(buffer);
>>>}
>>> }
>>>
>>> I agree that you probably want to encapsulate the cleaner registration and
>>> cleaning action but for frameworks it might be beneficial to have a common
>>> utility method implementing a pattern such as the one above.
>>>
>>> Chris
>>
>> I think the Cleaner API is more suitable to be used inside various classes
>> that themselves implement AutoCloseable rather than as an (optional) aspect
>> on top of them. So your example would simply read:
>>
>> public T execute(BufferAction bufferAction) {
>> try (Buffer buffer = allocateBuffer()) {
>> bufferAction.execute(buffer);
>> }
>> }
>>
>> There's no point in doing both things as in your example: using
>> try-with-resources + the Cleaner API on top. try-with-resources already
>> guarantees cleanup.
>
> Sorry, the following:
>> Cleanable API is meant to be used inside otherwise AutoCleanable
>> implementations to shield the user from native resource leaks if the user
>> forgets to use or can't use try-with-resources.
>
> Should read:
>
> Cleaner API is meant to be used inside otherwise AutoCloseable
> implementations to shield the user from native resource leaks if the user
> forgets to use or can't use try-with-resources.
>
>>
>> Regards, Peter
>>
>>>
On 16 May 2016, at 00:08, Peter Levart
wrote:
Hi Roger and others,
When the new Cleaner API was created the implementation of Cleanable(s)
was split into the low-level abstract [Soft|Weak|Phantom]Cleanable classes
to be used internally for purposes where the footprint matters and their
corresponding CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses used
as implementations that take a Runnable cleanup action and are exposed via
the public Cleaner API.
When thinking of possible JDK internal use cases for the low-level API, I
came to the conclusion that [Soft|Weak|Phantom]Cleanable classes are not
suitable as is, because in cases where footprint matters, it is usually
also the case that the number of [Soft|Weak|Phantom]Cleanable instances
created is larger and that construction performance also matters.
Especially multi-threaded construction. I'm thinking of the use cases of
auto-cleanable concurrent data structures. In such use cases, the present
features of [Soft|Weak|Phantom]Cleanable classes, namely the guaranteed
just-once cleanup action invocation and keeping the Cleanable instance
reachable until the cleanup action is performed, are actually not needed
and just present footprint and performance (contention) overhead. They
also present an overhead as they don't allow GC to automatically collect
the Cleanable instances if the data structure containing them becomes
unreachable and corresponding registered cleanup actions obsolete.
The mentioned features are important for public Cleaner.Cleanable
instances as they are usually used for cleanup of native resources where
the performance of their creation is not so drastically important and
where there is no intrinsic data structure to hold them reachable.
I propose to move those features from the [Soft|Weak|Phantom]Cleanable
classes down the hierarchy to the
CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Cleaner.cleanup/webrev.01/
Re: RFR 8156485 MethodHandles.varHandleExactInvoker should perform exact checks
Looks good. Best regards, Vladimir Ivanov PS: regarding naming, makeVarHandleMethodInvoker is shorter and as informative. Since there are only 2 flavors of invokers for VarHandles (for MHs there are also basic invokers), there's no need to explicitly mention the modes in method names. On 5/10/16 4:36 PM, Paul Sandoz wrote: Hi, Please review: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8156486-varHandleExactInvoker/webrev/ A bug slipped in with recent VarHandle implementation improvements, and this was not caught by relevant tests. The MethodHandle returned from MethodHandles.varHandleExactInvoker performed an incorrect match using erased signatures. Ooops. I fixed this by consolidating the lambda form generation for exact and generic VH invokers. The tests are also updated, primarily to VarHandleTestMethod* (changes to other areas are minor clean ups) to correctly test the 4 ways to obtain a MethodHandle to a VarHandle access mode method, and to correctly differentiate between testing exact and generic invocation. Thanks, Paul.
Re: Cleaner cleanup
Hi Peter, I for one think this looks like a very nice cleanup. The patch drops 5 classes from a minimal VM startup test, which is a welcome improvement. /Claes On 2016-05-16 00:08, Peter Levart wrote: Hi Roger and others, When the new Cleaner API was created the implementation of Cleanable(s) was split into the low-level abstract [Soft|Weak|Phantom]Cleanable classes to be used internally for purposes where the footprint matters and their corresponding CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses used as implementations that take a Runnable cleanup action and are exposed via the public Cleaner API. When thinking of possible JDK internal use cases for the low-level API, I came to the conclusion that [Soft|Weak|Phantom]Cleanable classes are not suitable as is, because in cases where footprint matters, it is usually also the case that the number of [Soft|Weak|Phantom]Cleanable instances created is larger and that construction performance also matters. Especially multi-threaded construction. I'm thinking of the use cases of auto-cleanable concurrent data structures. In such use cases, the present features of [Soft|Weak|Phantom]Cleanable classes, namely the guaranteed just-once cleanup action invocation and keeping the Cleanable instance reachable until the cleanup action is performed, are actually not needed and just present footprint and performance (contention) overhead. They also present an overhead as they don't allow GC to automatically collect the Cleanable instances if the data structure containing them becomes unreachable and corresponding registered cleanup actions obsolete. The mentioned features are important for public Cleaner.Cleanable instances as they are usually used for cleanup of native resources where the performance of their creation is not so drastically important and where there is no intrinsic data structure to hold them reachable. I propose to move those features from the [Soft|Weak|Phantom]Cleanable classes down the hierarchy to the CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses: http://cr.openjdk.java.net/~plevart/jdk9-dev/Cleaner.cleanup/webrev.01/ In this change I also removed the CleanerImpl.[Soft|Weak]CleanableRef subclasses as they are not needed and I believe will never be. I also renamed the CleanerImpl.PhantomCleanableRef subclass to CleanerImpl.PhantomCleanableImpl. Changes to the implementation are straightforward. The most work was put into the corresponding test. I did some clean-up to it and also changed it to accommodate for the new behavior of [Soft|Weak|Phantom]Cleanable classes. The changes speak for itself. One of the not-so obvious changes was to replace the CleanableCase.clearRef() action with the CleanableCase.releaseReferent() action. The old clearRef() action did not serve any purpose. Whether this method was called or not, the behavior of the corresponding Cleanable was unchanged as the Reference instance (referenced from the 'ref' field) was always of the same strength as the Cleanable itself. So clearing it could not affect the behavior of the Cleanable. I changed 'ref' to hold a direct reference to the referent and renamed the field to 'referent'. I changed the EV_XXX int constants to Event enum constants with helper methods used in CleanableCase.expectedCleanups() method that now returns the number of expected cleanup invocations - in the PhantomCleanableImpl case this is the number of expected cleanup action invocations while in the plain XxxCleanable subclass cases it is the number of Cleanable.clean() method invocations. I added the no-actions case to both PhantomCleanableImpl and XxxCleanable cases and extended the number and combinations of XxxCleanable cases. The checkCleaned() method was extended to verify that the number of cleanup invocations is *no more* and no less then the expected. See how WeakKey test is now simplified. This is the typical use-case for WeakCleanable I was talking about. So, what do you think of this cleanup? Regards, Peter
RFR: 8151832: Improve exception messages in exceptions thrown by jigsaw code
Some extra capturing of context in exception handling. I've re-based the original suggested patch and added some minor edits. https://bugs.openjdk.java.net/browse/JDK-8151832 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8151832/webrev/index.html -- Regards, Sean.
Re: RFR: 8151832: Improve exception messages in exceptions thrown by jigsaw code
On 16/05/2016 13:44, Seán Coffey wrote: Some extra capturing of context in exception handling. I've re-based the original suggested patch and added some minor edits. https://bugs.openjdk.java.net/browse/JDK-8151832 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8151832/webrev/index.html Would it be possible to leave out the changes to the source files in the module and loader directories for now? We have many changes to this code coming that replace parts of this code and I think would be better to do a pass over the exceptions in a few months once the code is more stable. -Alan.
Re: RFR: 8151832: Improve exception messages in exceptions thrown by jigsaw code
On 16/05/16 13:51, Alan Bateman wrote: On 16/05/2016 13:44, Seán Coffey wrote: Some extra capturing of context in exception handling. I've re-based the original suggested patch and added some minor edits. https://bugs.openjdk.java.net/browse/JDK-8151832 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8151832/webrev/index.html Would it be possible to leave out the changes to the source files in the module and loader directories for now? We have many changes to this code coming that replace parts of this code and I think would be better to do a pass over the exceptions in a few months once the code is more stable. Yes, I can hold off. I figured such improvements might help people while they adopt and set up modules but let's cancel until the code stabilizes some more! regards, Sean. -Alan.
Helpers for MethodHandles.byteArrayViewVarHandle VarHandle
Hi, what do you thing about adding helper methods for this kind of VarHandle? It would be nice to have something like Long.getFrom[LE/BE](byte[] arr, int index) // LE/BE -> endianity Long.setTo[LE/BE](long val, byte[] arr, int index) or Arrays.getLongFrom[LE/BE](byte[] arr, int index) Arrays.setLongTo[LE/BE](long val, byte[] arr, int index) Jaroslav
Re: RFR: 8151832: Improve exception messages in exceptions thrown by jigsaw code
On 16/05/2016 14:45, Seán Coffey wrote: On 16/05/16 13:51, Alan Bateman wrote: On 16/05/2016 13:44, Seán Coffey wrote: Some extra capturing of context in exception handling. I've re-based the original suggested patch and added some minor edits. https://bugs.openjdk.java.net/browse/JDK-8151832 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8151832/webrev/index.html Would it be possible to leave out the changes to the source files in the module and loader directories for now? We have many changes to this code coming that replace parts of this code and I think would be better to do a pass over the exceptions in a few months once the code is more stable. Yes, I can hold off. I figured such improvements might help people while they adopt and set up modules but let's cancel until the code stabilizes some more! Thanks. The rest mostly look okay although I think several of these exceptions in the jimage code need to re-examined - for example IOOBE is thrown in several places where the root cause must be a corrupt or truncated jimage file. -Alan
Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more
Hi Martin, As I have been studying the CleanerTest too recently, let me comment on some questions... On 05/09/2016 06:34 PM, Martin Buchholz wrote: --- Assert.assertEquals(map.get(k2), data, "value should be found in the map"); key = null; System.gc(); Assert.assertNotEquals(map.get(k2), data, "value should not be found in the map"); I initially expected this to fail at least intermittently because a cleaner may not yet have removed the entry from the map. ...The cleaner need not remove the entry from the map in order for this code to succeed. The sufficient condition is that GC discovers the k1 and k2 WeakReference(s) and clears them. When a WeakKey is cleared it is only equal to itself since the referent is gone (see equals() method). So it is important to ensure that the referent remains reachable at least until the 1st assert above (perhaps by inserting Reference.reachabilityFence(key) just after it)... Why aren't we calling whitebox.fullGC() everywhere where System.gc() is being called? What's particularly confusing here is that the WeakKey is not actually surely removed from the map, but instead two WeakKeys that may have been equal become non-equal when they get cleared by the gc. Which is generally bad practice. Which suggests we wouldn't implement a real public concurrent map of weak keys this way. Why not? We don't need to lookup the value after the WeakKey(s) are cleared, since by definition, we can't reach their referent any more (the 'key'). But the instance of the WeakKey that is inserted in the map can still be removed because it is the same instance that is referenced by the WeakCleanable and a cleared WeakKey is still equal to itself. So we have the following invariants: - while the key (the referent) is still reachable, multiple WeakKeys with the same referent are equal among themselves so a WeakKey can be constructed that is equal to some other WeakKey in the map. - after the key (the referent) becomes weakly-reachable and GC kicks-in, all WeakReferences with referents from the selected set are cleared atomically, including WeakKey(s) and WeakCleanable(s) refering to the same key referent, so only the instance of the WeakKey that is inserted in the map can be used to remove the stale entry from it. You can find a simpler implementation of WeakKey in my "Cleaner cleanup" proposal where WeakKey is a subclass of WeakCleanable and simply removes itself from the map when its clean() method is called by the Cleaner thread... --- WeakCleanable and friends are defined in jdk.internal, don't appear to be used at all, and since there are fields in CleanerImpl, they impose a hidden tax on all users of Cleaner. I don't see why there is both WeakCleanable (using subclassing) and WeakCleanableRef (using a provided Runnable). Is a compelling use case for WeakCleanables forthcoming? I have a few ideas that I would like to present, but I first wanted to propose making [Soft|Weak]Cleanable(s) even more low-level to make those use cases feasible. See the "Cleaner cleanup" thread for more info on that. Regards, Peter
RE: RFR: 8144062: Move jdk.Version to java.lang.Runtime.Version
Hi, Remi.
Thanks for taking the time to review this change.
> java.lang.Runtime.Version is used during the boot process
I don’t think that Runtime.Version is used during boot because
I'm not seeing it loaded with a small test program invoked with
"java -verbose:class Hi". In fact, I'm not seeing a difference
in the number of loaded classes between promoted build 118 and
my build for 8144062 (based on jdk9/dev). See appended stats.
If my test program is in a JAR file, then more classes are
loaded including Runtime.Version; however the equivalent number
of classes are loaded before my changes too.
The performance problem identified by the following bug should
resolve this issue:
8150678: JarFile stream() and entries(0 methods need performance
enhancement
https://bugs.openjdk.java.net/browse/JDK-8150678
Regards,
Iris
-
$ cat Hi.java
public class Hi {
public static void main(String ... args) {
System.out.println("hi");
System.exit(42);
}
$ wc Hi.ver*
501 2000 39758 Hi.verbose-118
576 2300 45915 Hi.verbose-118-jar
501 2000 39734 Hi.verbose-8144062
576 2300 45905 Hi.verbose-8144062-jar
2154 8600 171312 total
-Original Message-
From: Remi Forax [mailto:fo...@univ-mlv.fr]
Sent: Friday, May 13, 2016 4:32 PM
To: Iris Clark
Cc: Java Core Libs; compiler-...@openjdk.java.net; verona-...@openjdk.java.net
Subject: 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/Runtim
> e.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
- Mail original -
> De: "Iris Clark"
> À: "Remi Forax"
> Cc: "Java Core Libs" ,
> compiler-...@openjdk.java.net, verona-...@openjdk.java.net
> Envoyé: Lundi 16 Mai 2016 19:52:33
> Objet: RE: RFR: 8144062: Move jdk.Version to java.lang.Runtime.Version
>
> Hi, Remi.
Hi Iris,
>
> Thanks for taking the time to review this change.
>
> > java.lang.Runtime.Version is used during the boot process
>
> I don’t think that Runtime.Version is used during boot because
> I'm not seeing it loaded with a small test program invoked with
> "java -verbose:class Hi". In fact, I'm not seeing a difference
> in the number of loaded classes between promoted build 118 and
> my build for 8144062 (based on jdk9/dev). See appended stats.
>
> If my test program is in a JAR file, then more classes are
> loaded including Runtime.Version; however the equivalent number
> of classes are loaded before my changes too.
I've double checked and yes, Runtime.Version is not loaded by default,
my bad on this, it's own runtime that has an indirect dependency on
Runtime.Version.
>
> The performance problem identified by the following bug should
> resolve this issue:
>
> 8150678: JarFile stream() and entries(0 methods need performance
> enhancement
> https://bugs.openjdk.java.net/browse/JDK-8150678
>
> Regards,
> Iris
regards,
Rémi
>
> -
> $ cat Hi.java
> public class Hi {
> public static void main(String ... args) {
> System.out.println("hi");
> System.exit(42);
> }
> $ wc Hi.ver*
>501 2000 39758 Hi.verbose-118
>576 2300 45915 Hi.verbose-118-jar
>501 2000 39734 Hi.verbose-8144062
>576 2300 45905 Hi.verbose-8144062-jar
> 2154 8600 171312 total
>
> -Original Message-
> From: Remi Forax [mailto:fo...@univ-mlv.fr]
> Sent: Friday, May 13, 2016 4:32 PM
> To: Iris Clark
> Cc: Java Core Libs; compiler-...@openjdk.java.net;
> verona-...@openjdk.java.net
> Subject: 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/Runtim
> > e.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
> >
>
RFR: JDK-8157069: Assorted ZipFile improvements
Hi Xueming, https://bugs.openjdk.java.net/browse/JDK-8157069 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ZipFile-assortment/ Hopefully other improvements later. BTW, I notice that the CEN is stored in a byte[], and someday someone will create a zip file with a CEN that won't fit!
Re: RFR: JDK-8157069: Assorted ZipFile improvements
On 05/16/2016 11:53 AM, Martin Buchholz wrote: Hi Xueming, https://bugs.openjdk.java.net/browse/JDK-8157069 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ZipFile-assortment/ Hopefully other improvements later. looks fine. yes, someone will have to deal with a > byte[] cen table, some day. -sherman
RFR(s): 8133977 add specification for serial form of immutable collections
Hi all, Please review this changeset that adds specifications of the serialized forms (really, a single serialization proxy class) for the immutable collections implementation. There are no code changes in this changeset, just documentation. It's somewhat odd, but the class doc for the serialization proxy isn't actually included in the serialized-form.html output. I had to jigger around the method doc for readResolve() to include some general information about the class. I also added links manually from the List, Map, and Set interfaces to the serialized form and vice-versa. I'm not aware of another way for a private class to link to its (proxied) serialized form. I was able to coerce specdiff into giving a diff of serialized-form.html. It's not very convenient, though; like serialized-form.html, the html diff is one big file. The only difference is the addition of java.util.CollSer. Webrev: http://cr.openjdk.java.net/~smarks/reviews/8133977/webrev.0/ API specdiff: http://cr.openjdk.java.net/~smarks/reviews/8133977/specdiff.0/api.specdiff/overview-summary.html serialized-form.html diff: http://cr.openjdk.java.net/~smarks/reviews/8133977/specdiff.0/serial.specdiff/specdiff-summary.html Thanks, s'marks
RFR [9]: 8072579: XjcOptionalPropertyTest.java creates files in test.src
Hi, Please, help to review XjcOptionalPropertyTest test modification bug [1]. The list of applied changes: 1. Test files are now generated in jtreg work directory (scratch), instead of test source folder. 2. Shell script was replaced with java code: xjc tool is now invoked via jdk.testlibrary.JDKToolLauncher. 3. Compilation of xjc generated classes is done with usage of JDKToolLauncher. Webrev: http://cr.openjdk.java.net/~aefimov/8072579/9/00 With Best Regards, Aleksej [1] https://bugs.openjdk.java.net/browse/JDK-8072579
RFR [9]: 8157096: [TEST_BUG] test/javax/xml/bind/xjc/8145039/JaxbMarshallTest.java is skipped by jtreg
Hi, Please review the changes to test/javax/xml/bind/xjc/8145039/JaxbMarshallTest.java: 1. The test is skipped by JTREG due to incorrect module name in jtreg @modules tag: javax.xml.bind -> java.xml.bind 2. The compilation of XJC generated classes now requires "-addmods java.xml.bind" argument to be passed to javac tool. The test was executed with JTREG and passed. Webrev: http://cr.openjdk.java.net/~aefimov/8157096/9/00 JBS: https://bugs.openjdk.java.net/browse/JDK-8157096 With Best Regards, Aleksej
Can an object be finalized while still weakly reachable?
I have some evidence that an object's finalize method can run while a
weak reference pointing to it is not yet cleared, which surprised me.
E.g.
class F { protected void finalize() { assert wref.get() != this; } }
static WeakReference wref = new WeakReference(new F());
If this is a bug, I can try to give y'all a repro recipe.
If not, we should fix the docs
"""When the weak references to a weakly-reachable object are cleared,
the object becomes eligible for finalization."""
(It's also quite possible I made a mistake diagnosing this)
Re: RFR 8156718: Need tests for IsoFields getFrom for unsupported non-Iso Temporal fields
Hi Bhanu, I think you should add a test case comparing the return value of getFrom() ( Not an official reviewer) Regards, Nadeesh On 5/16/2016 11:46 AM, Bhanu Gopularam wrote: Hi all, Could you please review fix for following issue. Bug id: https://bugs.openjdk.java.net/browse/JDK-8156718 Solution: Added tck tests for validating getFrom method for unsupported non-Iso temporal fields Webrev: http://cr.openjdk.java.net/~bgopularam/JDK-8156718/webrev.00/ Thanks, Bhanu -- Thanks and Regards, Nadeesh TV
Re: Can an object be finalized while still weakly reachable?
Hi Martin,
On 05/17/2016 05:19 AM, Martin Buchholz wrote:
I have some evidence that an object's finalize method can run while a
weak reference pointing to it is not yet cleared, which surprised me.
E.g.
class F { protected void finalize() { assert wref.get() != this; } }
static WeakReference wref = new WeakReference(new F());
If this is a bug, I can try to give y'all a repro recipe.
If not, we should fix the docs
"""When the weak references to a weakly-reachable object are cleared,
the object becomes eligible for finalization."""
(It's also quite possible I made a mistake diagnosing this)
What can happen with above code is that you get a NPE from dereferencing
wref in finlailze(). In case NPE is not thrown and the program
constructs only a single instance of F then assert should succeed.
It is possible that you made a mistake. Can you post the real code?
Regards, Peter
