8037343 Ready for review

[Sandipan Razzaque]

Hi all,

Please find below the webrev for bug 8037343.

http://www.sandipan.net/public/webrevs/8037343/webrev.00

Thanks in advance -

Cheers,
SR

Sandipan Razzaque | www.sandipan.net

Re: RFR: 8043012: (tz) Support tzdata2014c

[Yuka Kamiya]

Hi,

The fix looks good to me.

Thanks,
--
Yuka

(5/15/14, 7:59), Aleksej Efimov wrote:

Hello,

Can I have a review for the tzdata2014c integration to JDK9. This is a standard 
release of tzdata (except the hurry with Egypt rules - the main part in this 
release).
The following set of tests was executed with no failures:
test/sun/util/calendar test/java/util/Calendar test/sun/util/resources/TimeZone 
test/sun/util/calendar test/java/util/TimeZone
 test/java/time test/java/util/Formatter test/closed/java/util/Calendar

Webrev: http://cr.openjdk.java.net/~aefimov/8043012/9/webrev.00
Bug: https://bugs.openjdk.java.net/browse/JDK-8043012

Thank you,
Aleksej

Re: RFR: 8032901: WaitForMultipleObjects() return value not handled appropriately

[David Holmes]

On 14/05/2014 11:18 PM, Aleksej Efimov wrote:

David, Vitaly,

I totally agree with Vitaly's explanation (Vitaly - thank you for that)
and code in shmemBase.c (the usage of enterMutex() function for
sending/receiving bytes through shared memory connection) illustrates on
how the connection shutdown event is used as a "cancellation token".


Thanks for clarifying that.

So if we were to encounter an abandoned mutex the code would presently 
have acquired the mutex but return an error, thus preventing a 
subsequent release, and preventing other threads from acquiring (but 
allowing current thread to recurisevely acquire. So this could both hang 
and cause data corruption.


The new code will still return an error but release the mutex. So no 
more hangs (other than by conditions caused by data corruption) but more 
opportunity for data corruption.


Obviously it depends on exactly what happens in the critical sections 
guarded by this mutex, but in general I don't agree with this rationale 
for making the change:


 204 /* If the mutex is abandoned we want to return an error
 205  * and also release the mutex so that we don't cause
 206  * other threads to be blocked. If a mutex was abandoned
 207  * we are in scary state. Data may be corrupted or inconsistent
 208  * but it is still better to let other threads run (and possibly
 209  * crash) than having them blocked (on the premise that a crash
 210  * is always easier to debug than a hang).

Considering something has to have gone drastically wrong for the mutex 
to become abandoned, I'm more inclined to consider this a fatal error 
and abort.


But I'll let the serviceability folk chime in here.

Thanks,
David



Thank you,
-Aleksej


On 05/14/2014 01:05 PM, David Holmes wrote:

On 14/05/2014 11:06 AM, Vitaly Davidovich wrote:

In windows, you acquire a mutex by waiting on it using one of the wait
functions, one of them employed in the code in question.  If
WaitForMultipleObjects succeeds and returns the index of the mutex,
current thread has ownership now.


Yes I understand the basic mechanics :)


It's also common to use multi wait functions where the event is a
"cancelation token", e.g. manual reset event; this allows someone to
cancel waiting on mutex acquisition and return from the wait function.
Presumably that's the case here, but I'll let Aleksej confirm; just
wanted to throw this out there in the meantime :).


Ah I see - yes cancellable lock acquisition would make sense.

Thanks,
David


Sent from my phone

On May 13, 2014 6:46 PM, "David Holmes" mailto:david.hol...@oracle.com>> wrote:

Hi Aleksej,

Thanks for the doc references regarding abandonment.

Let me rephrase my question. What is this logic trying to achieve by
waiting on both a mutex and an event? Do we already own the mutex
when this function is called?

David

On 13/05/2014 11:19 PM, Aleksej Efimov wrote:

David,

The Windows has a different terminology for mutex objects (much
differs
from the POSIX one). This one link gave me some understanding of
it [1].

Here is the MSDN [1] description of what "abandoned mutex" is:
" If a thread terminates without releasing its ownership of a
mutex
object, the mutex object is considered to be abandoned. A
waiting thread
can acquire ownership of an abandoned mutex object, but the wait
function will return*WAIT_ABANDONED*to indicate that the mutex
object is
abandoned. An abandoned mutex object indicates that an error has
occurred and that any shared resource being protected by the
mutex
object is in an undefined state. If the thread proceeds as
though the
mutex object had not been abandoned, it is no longer considered
abandoned after the thread releases its ownership. This restores
normal
behavior if a handle to the mutex object is subsequently
specified in a
wait function."


What does it mean to wait on mutex and ownership of the mutex
object:
"Any thread with a handle to a mutex object can use one of
thewait
functions
>to
request ownership of the mutex object. If the mutex object is
owned by
another thread, the wait function blocks the requesting thread
until the
owning thread releases the mutex object using the*ReleaseMutex*
>__function."

How we can release mutex and wait on already owned mutex:
" After a thread obtains ownership of a mutex, it can specify
the same
mutex in 

RFR: 8043012: (tz) Support tzdata2014c

[Aleksej Efimov]

Hello,

Can I have a review for the tzdata2014c integration to JDK9. This is a 
standard release of tzdata (except the hurry with Egypt rules - the main 
part in this release).

The following set of tests was executed with no failures:
test/sun/util/calendar test/java/util/Calendar 
test/sun/util/resources/TimeZone test/sun/util/calendar 
test/java/util/TimeZone

 test/java/time test/java/util/Formatter test/closed/java/util/Calendar

Webrev: http://cr.openjdk.java.net/~aefimov/8043012/9/webrev.00
Bug: https://bugs.openjdk.java.net/browse/JDK-8043012

Thank you,
Aleksej

Re: RFR JDK-7153400: ThreadPoolExecutor's setCorePoolSize method allows corePoolSize > maxPoolSize

[Pavel Rappo]

Hi Martin,

Thanks for you comments. I forgot indeed that awaitTermination indicates its 
result by returning a boolean value rather than throwing TimeoutException. So 
this should be fine now:

@@ -77,7 +77,10 @@
 private static void dispose(ThreadPoolExecutor p) {
 p.shutdownNow();
 try {
-p.awaitTermination(1, TimeUnit.MINUTES);
+boolean shutdown = p.awaitTermination(1, TimeUnit.MINUTES);
+if (!shutdown)
+throw new RuntimeException(
+"Pool did not terminate in a timely manner");
 } catch (InterruptedException e) {
 throw new RuntimeException("Should not happen", e);
 }

As for the "fail" method, it's a little bit different from "assertThrows". I 
tried to keep my checks (test payload) to be one liners. So the whole lifecycle 
of a ThreadPoolExecutor is confined in a single line. In addition to check 
whether the IllegalArgumentException is thrown, "fail" also disposes the pool. 
It's not clean object oriented design, I agree, but it was done for the sake of 
clarity. This test is supposed to be simple.

-Pavel

On 14 May 2014, at 18:21, Martin Buchholz  wrote:

> Pavel,
> 
> Thanks for writing a test.
> 
> We (jsr166 maintainers will add the jtreg test to jsr166 CVS when it has 
> passed review.
> 
> Instead of "succeed", I would just write main-line code.  If you want 
> per-api-call granularity, write a testng test.
> 
> Instead of "fail", I suggest as in jsr166 CVS 
> src/test/tck/JSR166TestCase.java :
> 
> public void assertThrows(Class 
> expectedExceptionClass,
>  Runnable... throwingActions) {
> for (Runnable throwingAction : throwingActions) {
> boolean threw = false;
> try { throwingAction.run(); }
> catch (Throwable t) {
> threw = true;
> if (!expectedExceptionClass.isInstance(t)) {
> AssertionFailedError afe =
> new AssertionFailedError
> ("Expected " + expectedExceptionClass.getName() +
>  ", got " + t.getClass().getName());
> afe.initCause(t);
> threadUnexpectedException(afe);
> }
> }
> if (!threw)
> shouldThrow(expectedExceptionClass.getName());
> }
> }
> 
> I suggest checking the return from p.awaitTermination
> p.awaitTermination(1, TimeUnit.MINUTES);
> 
> as in src/test/tck/JSR166TestCase.java:
> 
> 
> /**
>  * Waits out termination of a thread pool or fails doing so.
>  */
> void joinPool(ExecutorService exec) {
> try {
> exec.shutdown();
> assertTrue("ExecutorService did not terminate in a timely manner",
>exec.awaitTermination(2 * LONG_DELAY_MS, 
> MILLISECONDS));
> } catch (SecurityException ok) {
> // Allowed in case test doesn't have privs
> } catch (InterruptedException ie) {
> fail("Unexpected InterruptedException");
> }
> }
> 
> 
> 
> 
> On Wed, May 14, 2014 at 10:03 AM, Mike Duigou  wrote:
> Hi Pavel;
> 
> The change and test looks good. Will the test be upstreamed or will Doug be 
> adding a similar test in his upstream?
> 
> Mike
> 
> On May 14 2014, at 08:29 , Pavel Rappo  wrote:
> 
> > Hi everyone,
> >
> > could you please review my change for JDK-7153400?
> >
> > http://cr.openjdk.java.net/~chegar/7153400/00/webrev/
> > http://ccc.us.oracle.com/7153400
> >
> > It's a long expected fix for a minor issue in the ThreadPoolExecutor. This 
> > has been agreed with Doug Lea. The exact same change (except for the test) 
> > is already in jsr166 repo: 
> > http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/ThreadPoolExecutor.java?r1=1.151&r2=1.152
> >
> > Thanks
> > -Pavel
> 
> 

Re: RFR JDK-7153400: ThreadPoolExecutor's setCorePoolSize method allows corePoolSize > maxPoolSize

[Pavel Rappo]

Oh I see, thanks.

-Pavel

On 14 May 2014, at 20:56, Doug Lea  wrote:

> On 05/14/2014 03:15 PM, Pavel Rappo wrote:
>> Hi Mike,
>> 
>> Unfortunately I don't know. I suppose it is better to rely on our own jdk
>> tests. The only thing I did was sent Doug 2 patches for the tck tests. But as
>> far as I can see he hasn't applied them yet. And I don't even know if he is
>> going to. Here they are:
>> 
> 
> Sorry; I'm running behind (end of semester here). As Martin hinted at,
> the reason I hadn't gotten to this is that this is the first jdk9-only
> test revision, and we hadn't set up for this yet. But we'll definitely
> include jsr166 tck coverage by somehow adapting these.
> 
> -Doug
> 

Re: RFR 9: 8003488 Add Process.getPid

[roger riggs]

Hi,

For system local process identifiers, all of the systems I'm aware are
32 bit integers,  printed and parsed in decimal for ease of use.
I would describe the native pid as:
The native process id is the identifier commonly used in the
operating system APIs and commands to show the status of and
manage processes; typically a decimal number with 1 to 6 digits.

The Apollo system used a string to be able to uniformly address
processes across  hosts and it was parseable to host and decimal pid.

I anticipate a ProcessHandle type with methods to check if the process 
is alive,

to destroy it, wait for it to terminate, etc.
It would be a supertype of Process but not all ProcessHandles would be 
processes

since they were not created by Process/ProcessBuilder and have different
security concerns and checks.
ProcessHandles would be returned from factory methods like current(long pid)
or current().   If Java is ported to a system with non-numeric process 
identifiers

it would be viable to add appropriate factory methods.

Roger

On 5/12/2014 5:01 PM, Alan Bateman wrote:

On 12/05/2014 20:44, roger riggs wrote:

Please review and comment on this long requested addition to provide the
native process id of a spawned Process.

Webrev: http://cr.openjdk.java.net/~rriggs/webrev-getpid-8003488/
Issue:  https://bugs.openjdk.java.net/browse/JDK-8003488
I think the representation of the pid needs consideration - we need to 
be happy that using a long is the right thing to do and won't cause us 
problems in the future and won't conflict with other APIs that we 
might add in this area.


In terms of spec then it might be helpful to say a bit more to define 
a "native process id", even in an abstract way.


-Alan

Re: RFR JDK-7153400: ThreadPoolExecutor's setCorePoolSize method allows corePoolSize > maxPoolSize

[Doug Lea]

On 05/14/2014 03:15 PM, Pavel Rappo wrote:

Hi Mike,

Unfortunately I don't know. I suppose it is better to rely on our own jdk
tests. The only thing I did was sent Doug 2 patches for the tck tests. But as
far as I can see he hasn't applied them yet. And I don't even know if he is
going to. Here they are:



Sorry; I'm running behind (end of semester here). As Martin hinted at,
the reason I hadn't gotten to this is that this is the first jdk9-only
test revision, and we hadn't set up for this yet. But we'll definitely
include jsr166 tck coverage by somehow adapting these.

-Doug

Re: RFR JDK-7153400: ThreadPoolExecutor's setCorePoolSize method allows corePoolSize > maxPoolSize

[Pavel Rappo]

Hi Mike,

Unfortunately I don't know. I suppose it is better to rely on our own jdk 
tests. The only thing I did was sent Doug 2 patches for the tck tests. But as 
far as I can see he hasn't applied them yet. And I don't even know if he is 
going to. Here they are:

--- src/test/tck/ThreadPoolExecutorSubclassTest.java(revision 1.32)
+++ src/test/tck/ThreadPoolExecutorSubclassTest.java(revision )
@@ -1180,6 +1180,23 @@
 }
 
 /**
+ * setCorePoolSize(int) throws IllegalArgumentException
+ * if given a value greater the maximum pool size
+ */
+public void testCorePoolSizeIllegalArgumentException2() {
+ThreadPoolExecutor p =
+new CustomTPE(1,2,LONG_DELAY_MS, MILLISECONDS,new 
ArrayBlockingQueue(10));
+try {
+p.setCorePoolSize(3);
+shouldThrow();
+} catch (IllegalArgumentException success) {
+} finally {
+try { p.shutdown(); } catch (SecurityException ok) { return; }
+}
+joinPool(p);
+}
+
+/**
  * setMaximumPoolSize(int) throws IllegalArgumentException
  * if given a value less the core pool size
  */

--- src/test/tck/ThreadPoolExecutorTest.java(revision 1.49)
+++ src/test/tck/ThreadPoolExecutorTest.java(revision )
@@ -1302,6 +1302,25 @@
 }

 /**
+ * setCorePoolSize(int) throws IllegalArgumentException if
+ * given a value greater the maximum pool size
+ */
+public void testCorePoolSizeIllegalArgumentException2() {
+ThreadPoolExecutor p =
+new ThreadPoolExecutor(1, 2,
+LONG_DELAY_MS, MILLISECONDS,
+new ArrayBlockingQueue(10));
+try {
+p.setCorePoolSize(3);
+shouldThrow();
+} catch (IllegalArgumentException success) {
+} finally {
+try { p.shutdown(); } catch (SecurityException ok) { return; }
+}
+joinPool(p);
+}
+
+/**
  * setMaximumPoolSize(int) throws IllegalArgumentException if
  * given a value less the core pool size
  */


-Pavel

On 14 May 2014, at 18:03, Mike Duigou  wrote:

> Hi Pavel;
> 
> The change and test looks good. Will the test be upstreamed or will Doug be 
> adding a similar test in his upstream?
> 
> Mike
> 
> On May 14 2014, at 08:29 , Pavel Rappo  wrote:
> 
>> Hi everyone,
>> 
>> could you please review my change for JDK-7153400?
>> 
>> http://cr.openjdk.java.net/~chegar/7153400/00/webrev/
>> http://ccc.us.oracle.com/7153400
>> 
>> It's a long expected fix for a minor issue in the ThreadPoolExecutor. This 
>> has been agreed with Doug Lea. The exact same change (except for the test) 
>> is already in jsr166 repo: 
>> http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/ThreadPoolExecutor.java?r1=1.151&r2=1.152
>> 
>> Thanks
>> -Pavel
> 

RFR (S) 9 and 8u: JDK-8038994: AnnotatedType.getType() of a TypeVariable boundary without annotations return null

[Joel Borggren-Franck]

Hi,

Here is a fix for: https://bugs.openjdk.java.net/browse/JDK-8038994

In short, getAnnotatedFoo.getType() is supposed to return the same Type
as getGenericFoo(). This wasn't the case for a type variable bound
without an annotation previously.

Also cleaned up an allocation while in the neighborhood.

Webrev here: http://cr.openjdk.java.net/~jfranck/8038994/webrev.00/

cheers
/Joel

Re: RFR JDK-7153400: ThreadPoolExecutor's setCorePoolSize method allows corePoolSize > maxPoolSize

[Mike Duigou]

Hi Pavel;

The change and test looks good. Will the test be upstreamed or will Doug be 
adding a similar test in his upstream?

Mike

On May 14 2014, at 08:29 , Pavel Rappo  wrote:

> Hi everyone,
> 
> could you please review my change for JDK-7153400?
> 
> http://cr.openjdk.java.net/~chegar/7153400/00/webrev/
> http://ccc.us.oracle.com/7153400
> 
> It's a long expected fix for a minor issue in the ThreadPoolExecutor. This 
> has been agreed with Doug Lea. The exact same change (except for the test) is 
> already in jsr166 repo: 
> http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/ThreadPoolExecutor.java?r1=1.151&r2=1.152
> 
> Thanks
> -Pavel

Re: 8043119: (props) Properties.storeToXML closes output stream

[Mandy Chung]

On 5/14/14 4:03 AM, Alan Bateman wrote:


This is a follow-up to the recent change to the modules motivated 
switch of the Properties XML methods to consistently use the UKit 
parser. An issue that has surfaced since is that the wrapper around 
UKit is incorrectly closing the output stream after writing the XML 
document, a basic assertion not covered by the tests in the jdk 
repository. Trivial fix:


http://cr.openjdk.java.net/~alanb/8043119/webrev/


Looks good.

Mandy

Re: [9] RFR(S): 8005873: JRuby test_respond_to.rb asserts with: MT-unsafe modification of inline cache

[Christian Thalinger]

On May 14, 2014, at 3:47 AM, Vladimir Ivanov  
wrote:

> Tobias, I agree with your evaluation.
> 
> My only concern is that @Stable doesn't work for AtomicReferenceArray, so JIT 
> doesn't see what is stored in the array.

Now that is really unfortunate.

> Maybe use a lock instead?
> 
> Best regards,
> Vladimir Ivanov
> 
> On 5/14/14 2:33 PM, Tobias Hartmann wrote:
>> Hi Remi,
>> 
>> sorry, I accidentally reverted that part.. Here is the correct webrev:
>> 
>> http://cr.openjdk.java.net/~anoll/8005873/webrev.01/
>> 
>> Thanks,
>> Tobias
>> 
>> On 14.05.2014 12:04, Remi Forax wrote:
>>> your patch doesn't work !
>>> 
>>> the array is still allocated as an classical array in the constructor.
>>> 
>>> cheers,
>>> Remi
>>> 
>>> Envoyé avec AquaMail pour Android
>>> http://www.aqua-mail.com
>>> 
>>> 
>>> Le 14 mai 2014 11:04:41 Tobias Hartmann  a
>>> écrit :
>>> 
 Hi,
 
 please review the following patch for bug 8005873.
 
 *Problem*
 Bug: https://bugs.openjdk.java.net/browse/JDK-8005873
 
 The test creates multiple threads (~500) that repeatedly execute
 invokeExact on a method handle referencing a static method. The call
 to invokeExact is performed through an optimized inline cache
 (CompiledStaticCall) that points to the LambdaForm generated for the
 method handle. The IC is first set to interpreted code by
 CompiledStaticCall::set_to_interpreted(...) but soon updated to refer
 to the compiled version of the lambda form (-Xcomp).
 
 Because tiered compilation is enabled, the VM decides to compile the
 LambdaForm at a different level and sets the old version to
 not-entrant. The next call through the IC therefore triggers a
 re-resolving of the call site finally performing a Java upcall to
 java.lang.invoke.MethodHandleNatives::linkMethod(...) (see call
 sequence [1]). A *new* LambdaForm is returned and
 CompiledStaticCall::set_to_interpreted(...) is executed again to
 update the IC. The assert is hit because the callee differs.
 
 The problem is that there are multiple LambdaForms created for the
 same invokeExact instruction. Caching in the Java runtime code should
 guarantee that only one LambdaForm is created and reused.
 Instrumenting Invokers::invokeHandleForm(...) shows that almost all
 requests result in a cache miss and return a new LambdaForm.
 
 This behaviour is caused by a race condition in
 Invokers::invokeHandleForm(...). Threads may initially see a cache
 miss when invoking MethodTypeForm::cachedLambdaForm(...), then create
 a new LambdaForm and call MethodTypeForm::setCachedLambdaForm(...) to
 update the cache without checking it again. In a high concurrency
 setting, this leads to multiple LambdaForms being created for the
 same invokeExact instruction because the cache entry is overwritten
 by multiple threads.
 
 *Solution*
 Webrev: http://cr.openjdk.java.net/~anoll/8005873/webrev.00/
 
 An AtomicReferenceArray is used to cache the LambdaForms and
 .get(...) and .compareAndSet(...) are used to retrieve and update the
 cache entries. This allows only one thread to set the LambdaForm that
 is then being used by all instances of the invokeExact.
 
 *Testing*
 - Failing test (vm/mlvm/meth/stress/jni/nativeAndMH)
 - Nashorn + Octane
 - JPRT
 
 Many thanks to Christian Thalinger and Vladimir Ivanov!
 
 Best,
 Tobias
 
 [1] Call sequence of reresolving the IC target
 SharedRuntime::handle_wrong_method(...)
 -> SharedRuntime::reresolve_call_site(...)
 -> SharedRuntime::find_callee_method(...)
 -> SharedRuntime::find_callee_info_helper(...)
 -> LinkResolver::resolve_invoke(...)
 -> LinkResolver::resolve_invokehandle(...)
 -> LinkResolver::resolve_invokehandle(...)
 -> LinkResolver::lookup_polymorphic_method(...)
 -> SystemDictionary::find_method_handle_invoker(...)
 -> Java upcall to java.lang.invoke.MethodHandleNatives::linkMethod(...)
 -> Invokers::methodHandleInvokeLinkerMethod(...)
 -> Invokers::invokeHandleForm(...)
 
 
 
 
 
 
 
 
 
 Backport?
>>> 
>>> 
>> 

Re: RFR JDK-7153400: ThreadPoolExecutor's setCorePoolSize method allows corePoolSize > maxPoolSize

[Chris Hegarty]

Thanks for doing this Pavel. I can sponsor this for you.

-Chris.

On 14/05/14 16:29, Pavel Rappo wrote:

Hi everyone,

could you please review my change for JDK-7153400?

http://cr.openjdk.java.net/~chegar/7153400/00/webrev/
http://ccc.us.oracle.com/7153400

It's a long expected fix for a minor issue in the ThreadPoolExecutor. This has been 
agreed with Doug Lea. The exact same change (except for the test) is already in 
jsr166 repo: 
http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/ThreadPoolExecutor.java?r1=1.151&r2=1.152

Thanks
-Pavel

RFR JDK-7153400: ThreadPoolExecutor's setCorePoolSize method allows corePoolSize > maxPoolSize

[Pavel Rappo]

Hi everyone,

could you please review my change for JDK-7153400?

http://cr.openjdk.java.net/~chegar/7153400/00/webrev/
http://ccc.us.oracle.com/7153400

It's a long expected fix for a minor issue in the ThreadPoolExecutor. This has 
been agreed with Doug Lea. The exact same change (except for the test) is 
already in jsr166 repo: 
http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/ThreadPoolExecutor.java?r1=1.151&r2=1.152

Thanks
-Pavel

Re: RFR: 8032901: WaitForMultipleObjects() return value not handled appropriately

[Aleksej Efimov]

David, Vitaly,

I totally agree with Vitaly's explanation (Vitaly - thank you for that) 
and code in shmemBase.c (the usage of enterMutex() function for 
sending/receiving bytes through shared memory connection) illustrates on 
how the connection shutdown event is used as a "cancellation token".


Thank you,
-Aleksej


On 05/14/2014 01:05 PM, David Holmes wrote:

On 14/05/2014 11:06 AM, Vitaly Davidovich wrote:

In windows, you acquire a mutex by waiting on it using one of the wait
functions, one of them employed in the code in question.  If
WaitForMultipleObjects succeeds and returns the index of the mutex,
current thread has ownership now.


Yes I understand the basic mechanics :)


It's also common to use multi wait functions where the event is a
"cancelation token", e.g. manual reset event; this allows someone to
cancel waiting on mutex acquisition and return from the wait function.
Presumably that's the case here, but I'll let Aleksej confirm; just
wanted to throw this out there in the meantime :).


Ah I see - yes cancellable lock acquisition would make sense.

Thanks,
David


Sent from my phone

On May 13, 2014 6:46 PM, "David Holmes" mailto:david.hol...@oracle.com>> wrote:

Hi Aleksej,

Thanks for the doc references regarding abandonment.

Let me rephrase my question. What is this logic trying to achieve by
waiting on both a mutex and an event? Do we already own the mutex
when this function is called?

David

On 13/05/2014 11:19 PM, Aleksej Efimov wrote:

David,

The Windows has a different terminology for mutex objects (much
differs
from the POSIX one). This one link gave me some understanding of
it [1].

Here is the MSDN [1] description of what "abandoned mutex" is:
" If a thread terminates without releasing its ownership of a 
mutex

object, the mutex object is considered to be abandoned. A
waiting thread
can acquire ownership of an abandoned mutex object, but the wait
function will return*WAIT_ABANDONED*to indicate that the mutex
object is
abandoned. An abandoned mutex object indicates that an error has
occurred and that any shared resource being protected by the 
mutex

object is in an undefined state. If the thread proceeds as
though the
mutex object had not been abandoned, it is no longer considered
abandoned after the thread releases its ownership. This restores
normal
behavior if a handle to the mutex object is subsequently
specified in a
wait function."


What does it mean to wait on mutex and ownership of the mutex
object:
"Any thread with a handle to a mutex object can use one of 
thewait

functions
>to
request ownership of the mutex object. If the mutex object is
owned by
another thread, the wait function blocks the requesting thread
until the
owning thread releases the mutex object using the*ReleaseMutex*
>__function."

How we can release mutex and wait on already owned mutex:
" After a thread obtains ownership of a mutex, it can specify
the same
mutex in repeated calls to the wait-functions
>__without
blocking its execution. This prevents a thread from deadlocking
itself
while waiting for a mutex that it already owns. To release its
ownership
under such circumstances, the thread must call*ReleaseMutex*
>__once
for each time that the mutex satisfied the conditions of a wait
function."

[1]
http://msdn.microsoft.com/en-__gb/library/windows/desktop/__ms684266(v=vs.85).aspx


-Aleksej

On 05/13/2014 04:00 PM, David Holmes wrote:

I don't understand this one at all. What is an "abandoned
mutex"? For
that matter why does the code wait on a mutex and an event?
Do we
already own the mutex? If so what does it mean to wait on
it? If not
then how can we release it?

???

Thanks,
David


On 13/05/2014 8:57 PM, Alan Bateman wrote:

Re: [9] RFR(S): 8005873: JRuby test_respond_to.rb asserts with: MT-unsafe modification of inline cache

[Vladimir Ivanov]

On 5/14/14 4:12 PM, Paul Sandoz wrote:


On May 14, 2014, at 12:47 PM, Vladimir Ivanov  
wrote:


Tobias, I agree with your evaluation.



V. tricky one to track down!

 From @Stable:

  * It is (currently) undefined what happens if a field annotated as stable
  * is given a third value.  In practice, if the JVM relies on this annotation
  * to promote a field reference to a constant, it may be that the Java memory
  * model would appear to be broken, if such a constant (the second value of 
the field)
  * is used as the value of the field even after the field value has changed.

I dunno if that was a contributing factor in this case.

No, @Stable doesn't contribute to the problem.




My only concern is that @Stable doesn't work for AtomicReferenceArray, so JIT 
doesn't see what is stored in the array.


Yes, stability needs to be associated with the array elements.



Maybe use a lock instead?



Or perhaps use Unsafe.CAS directly within setCachedLambdaForm?
Yes, Unsafe is another option here. But since cache updates should be 
rare, Unsafe is an overkill here IMO - locking should be fine.




Also, as a consequence of using AtomicReferenceArray the following change may 
result in a memory barrier on some architectures:

  public LambdaForm cachedLambdaForm(int which) {
-return lambdaForms[which];

+return lambdaForms.get(which);
  }

since lambdaForms.get will call Unsafe.getObjectVolatile.

MTF.cachedLambaForm isn't on a fast path, so it shouldn't be a problem.


Separately, i think code that calls setCachedLambdaForm needs to be double 
checked to ensure that the return value is used. For example, in 
MethodHandleImpl.makeGuardWithCatchForm i see:

 basicType.form().setCachedLambdaForm(MethodTypeForm.LF_GWC, lform);
 return lform;

which i think needs to be:

 return basicType.form().setCachedLambdaForm(MethodTypeForm.LF_GWC, 
lform);

Good catch! MTF.setCachedLambdaForm usages should be fixed as well.

Best regards,
Vladimir Ivanov



Paul.

Re: [9] RFR(S): 8005873: JRuby test_respond_to.rb asserts with: MT-unsafe modification of inline cache

[Paul Sandoz]

On May 14, 2014, at 12:47 PM, Vladimir Ivanov  
wrote:

> Tobias, I agree with your evaluation.
> 

V. tricky one to track down!

From @Stable:

 * It is (currently) undefined what happens if a field annotated as stable
 * is given a third value.  In practice, if the JVM relies on this annotation
 * to promote a field reference to a constant, it may be that the Java memory
 * model would appear to be broken, if such a constant (the second value of the 
field)
 * is used as the value of the field even after the field value has changed.

I dunno if that was a contributing factor in this case.


> My only concern is that @Stable doesn't work for AtomicReferenceArray, so JIT 
> doesn't see what is stored in the array.

Yes, stability needs to be associated with the array elements.


> Maybe use a lock instead?
> 

Or perhaps use Unsafe.CAS directly within setCachedLambdaForm?

Also, as a consequence of using AtomicReferenceArray the following change may 
result in a memory barrier on some architectures:

 public LambdaForm cachedLambdaForm(int which) {
-return lambdaForms[which];

+return lambdaForms.get(which);
 }

since lambdaForms.get will call Unsafe.getObjectVolatile.

Separately, i think code that calls setCachedLambdaForm needs to be double 
checked to ensure that the return value is used. For example, in 
MethodHandleImpl.makeGuardWithCatchForm i see:

basicType.form().setCachedLambdaForm(MethodTypeForm.LF_GWC, lform);
return lform;

which i think needs to be:

return basicType.form().setCachedLambdaForm(MethodTypeForm.LF_GWC, 
lform);

Paul.

Re: 8043119: (props) Properties.storeToXML closes output stream

[Lance Andersen]

looks fine
On May 14, 2014, at 7:03 AM, Alan Bateman  wrote:

> 
> This is a follow-up to the recent change to the modules motivated switch of 
> the Properties XML methods to consistently use the UKit parser. An issue that 
> has surfaced since is that the wrapper around UKit is incorrectly closing the 
> output stream after writing the XML document, a basic assertion not covered 
> by the tests in the jdk repository. Trivial fix:
> 
> http://cr.openjdk.java.net/~alanb/8043119/webrev/
> 
> -Alan.



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

8043119: (props) Properties.storeToXML closes output stream

[Alan Bateman]

This is a follow-up to the recent change to the modules motivated switch 
of the Properties XML methods to consistently use the UKit parser. An 
issue that has surfaced since is that the wrapper around UKit is 
incorrectly closing the output stream after writing the XML document, a 
basic assertion not covered by the tests in the jdk repository. Trivial fix:


http://cr.openjdk.java.net/~alanb/8043119/webrev/

-Alan.

Re: [9] RFR(S): 8005873: JRuby test_respond_to.rb asserts with: MT-unsafe modification of inline cache

[Vladimir Ivanov]

Tobias, I agree with your evaluation.

My only concern is that @Stable doesn't work for AtomicReferenceArray, 
so JIT doesn't see what is stored in the array. Maybe use a lock instead?


Best regards,
Vladimir Ivanov

On 5/14/14 2:33 PM, Tobias Hartmann wrote:

Hi Remi,

sorry, I accidentally reverted that part.. Here is the correct webrev:

http://cr.openjdk.java.net/~anoll/8005873/webrev.01/

Thanks,
Tobias

On 14.05.2014 12:04, Remi Forax wrote:

your patch doesn't work !

the array is still allocated as an classical array in the constructor.

cheers,
Remi

Envoyé avec AquaMail pour Android
http://www.aqua-mail.com


Le 14 mai 2014 11:04:41 Tobias Hartmann  a
écrit :


Hi,

please review the following patch for bug 8005873.

*Problem*
Bug: https://bugs.openjdk.java.net/browse/JDK-8005873

The test creates multiple threads (~500) that repeatedly execute
invokeExact on a method handle referencing a static method. The call
to invokeExact is performed through an optimized inline cache
(CompiledStaticCall) that points to the LambdaForm generated for the
method handle. The IC is first set to interpreted code by
CompiledStaticCall::set_to_interpreted(...) but soon updated to refer
to the compiled version of the lambda form (-Xcomp).

Because tiered compilation is enabled, the VM decides to compile the
LambdaForm at a different level and sets the old version to
not-entrant. The next call through the IC therefore triggers a
re-resolving of the call site finally performing a Java upcall to
java.lang.invoke.MethodHandleNatives::linkMethod(...) (see call
sequence [1]). A *new* LambdaForm is returned and
CompiledStaticCall::set_to_interpreted(...) is executed again to
update the IC. The assert is hit because the callee differs.

The problem is that there are multiple LambdaForms created for the
same invokeExact instruction. Caching in the Java runtime code should
guarantee that only one LambdaForm is created and reused.
Instrumenting Invokers::invokeHandleForm(...) shows that almost all
requests result in a cache miss and return a new LambdaForm.

This behaviour is caused by a race condition in
Invokers::invokeHandleForm(...). Threads may initially see a cache
miss when invoking MethodTypeForm::cachedLambdaForm(...), then create
a new LambdaForm and call MethodTypeForm::setCachedLambdaForm(...) to
update the cache without checking it again. In a high concurrency
setting, this leads to multiple LambdaForms being created for the
same invokeExact instruction because the cache entry is overwritten
by multiple threads.

*Solution*
Webrev: http://cr.openjdk.java.net/~anoll/8005873/webrev.00/

An AtomicReferenceArray is used to cache the LambdaForms and
.get(...) and .compareAndSet(...) are used to retrieve and update the
cache entries. This allows only one thread to set the LambdaForm that
is then being used by all instances of the invokeExact.

*Testing*
- Failing test (vm/mlvm/meth/stress/jni/nativeAndMH)
- Nashorn + Octane
- JPRT

Many thanks to Christian Thalinger and Vladimir Ivanov!

Best,
Tobias

[1] Call sequence of reresolving the IC target
SharedRuntime::handle_wrong_method(...)
-> SharedRuntime::reresolve_call_site(...)
-> SharedRuntime::find_callee_method(...)
-> SharedRuntime::find_callee_info_helper(...)
-> LinkResolver::resolve_invoke(...)
-> LinkResolver::resolve_invokehandle(...)
-> LinkResolver::resolve_invokehandle(...)
-> LinkResolver::lookup_polymorphic_method(...)
-> SystemDictionary::find_method_handle_invoker(...)
-> Java upcall to java.lang.invoke.MethodHandleNatives::linkMethod(...)
-> Invokers::methodHandleInvokeLinkerMethod(...)
-> Invokers::invokeHandleForm(...)









Backport?

Re: [9] RFR(S): 8005873: JRuby test_respond_to.rb asserts with: MT-unsafe modification of inline cache

[Remi Forax]

your patch doesn't work !

the array is still allocated as an classical array in the constructor.

cheers,
Remi

Envoyé avec AquaMail pour Android
http://www.aqua-mail.com


Le 14 mai 2014 11:04:41 Tobias Hartmann  a écrit :


Hi,

please review the following patch for bug 8005873.

*Problem*
Bug: https://bugs.openjdk.java.net/browse/JDK-8005873

The test creates multiple threads (~500) that repeatedly execute 
invokeExact on a method handle referencing a static method. The call to 
invokeExact is performed through an optimized inline cache 
(CompiledStaticCall) that points to the LambdaForm generated for the method 
handle. The IC is first set to interpreted code by 
CompiledStaticCall::set_to_interpreted(...) but soon updated to refer to 
the compiled version of the lambda form (-Xcomp).


Because tiered compilation is enabled, the VM decides to compile the 
LambdaForm at a different level and sets the old version to not-entrant. 
The next call through the IC therefore triggers a re-resolving of the call 
site finally performing a Java upcall to 
java.lang.invoke.MethodHandleNatives::linkMethod(...) (see call sequence 
[1]). A *new* LambdaForm is returned and 
CompiledStaticCall::set_to_interpreted(...) is executed again to update the 
IC. The assert is hit because the callee differs.


The problem is that there are multiple LambdaForms created for the same 
invokeExact instruction. Caching in the Java runtime code should guarantee 
that only one LambdaForm is created and reused. Instrumenting 
Invokers::invokeHandleForm(...) shows that almost all requests result in a 
cache miss and return a new LambdaForm.


This behaviour is caused by a race condition in 
Invokers::invokeHandleForm(...). Threads may initially see a cache miss 
when invoking MethodTypeForm::cachedLambdaForm(...), then create a new 
LambdaForm and call MethodTypeForm::setCachedLambdaForm(...) to update the 
cache without checking it again. In a high concurrency setting, this leads 
to multiple LambdaForms being created for the same invokeExact instruction 
because the cache entry is overwritten by multiple threads.


*Solution*
Webrev: http://cr.openjdk.java.net/~anoll/8005873/webrev.00/

An AtomicReferenceArray is used to cache the LambdaForms and .get(...) and 
.compareAndSet(...) are used to retrieve and update the cache entries. This 
allows only one thread to set the LambdaForm that is then being used by all 
instances of the invokeExact.


*Testing*
- Failing test (vm/mlvm/meth/stress/jni/nativeAndMH)
- Nashorn + Octane
- JPRT

Many thanks to Christian Thalinger and Vladimir Ivanov!

Best,
Tobias

[1] Call sequence of reresolving the IC target
SharedRuntime::handle_wrong_method(...)
-> SharedRuntime::reresolve_call_site(...)
-> SharedRuntime::find_callee_method(...)
-> SharedRuntime::find_callee_info_helper(...)
-> LinkResolver::resolve_invoke(...)
-> LinkResolver::resolve_invokehandle(...)
-> LinkResolver::resolve_invokehandle(...)
-> LinkResolver::lookup_polymorphic_method(...)
-> SystemDictionary::find_method_handle_invoker(...)
-> Java upcall to java.lang.invoke.MethodHandleNatives::linkMethod(...)
-> Invokers::methodHandleInvokeLinkerMethod(...)
-> Invokers::invokeHandleForm(...)









Backport?

Re: [OpenJDK 2D-Dev] JDK-8041679 Replace uses of StringBuffer with StringBuilder within the JDK

[Alan Bateman]

On 14/05/2014 09:15, Paul Sandoz wrote:

:

And here what could have been a 2 line change is 25 ..
http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-media/webrev/src/share/classes/sun/font/StandardTextSource.java.sdiff.html
So I would say leave the variable names alone unless there's a compelling 
reason - and I don't see one.

"buf" no longer corresponds to a buffer but to a builder, so i think it is reasonable in 
this case to use the canonical "sb" name.
I agree, "buf" or "buffer" looks odd when the type is changed to 
StringBuilder.




:

This code is under src/share/classes, so it's not clear to me what classes i 
can modify and push in the jdk9-dev/jdk repo or not. It's confusing! Is there a 
list online somewhere?

Out of all the classes here:

   
http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-media/webrev/

exactly which ones should be pushed to the client forest and which ones to the 
jdk9-dev forest? all of them, or just a sub-set?

How long would it take for the changes to the client jdk repo to make it's way 
into the jdk9-dev jdk repo?

Really what i am getting at here is i think we need to change our process of 
how we manage such forests; we need to consolidate.

I think Sergey and Phil are asking for the bean classes from -core patch 
and all of -media to be pushed to jdk9/client.


From an approachability perspective then it's total baloney for library 
changes to take different routes. On the other hand there may be 
exceptions needed for areas or changes that involve manual or other 
special testing before integration. There was a lengthy thread on 
jdk9-dev about the integration forests and processes [1] where this was 
discussed. I don't recall seeing a boolean result to the questions that 
asked whether the client libraries require any manual testing before 
integration. As things stand then what we loosely call "client 
libraries" are pushed to jdk9/client, everything else is pushed to 
jdk9/dev. I really hope this can be re-visited soon.


-Alan

[1] 
http://mail.openjdk.java.net/pipermail/jdk9-dev/2013-November/00.html

Re: RFR: 8032901: WaitForMultipleObjects() return value not handled appropriately

[David Holmes]

On 14/05/2014 11:06 AM, Vitaly Davidovich wrote:

In windows, you acquire a mutex by waiting on it using one of the wait
functions, one of them employed in the code in question.  If
WaitForMultipleObjects succeeds and returns the index of the mutex,
current thread has ownership now.


Yes I understand the basic mechanics :)


It's also common to use multi wait functions where the event is a
"cancelation token", e.g. manual reset event; this allows someone to
cancel waiting on mutex acquisition and return from the wait function.
Presumably that's the case here, but I'll let Aleksej confirm; just
wanted to throw this out there in the meantime :).


Ah I see - yes cancellable lock acquisition would make sense.

Thanks,
David


Sent from my phone

On May 13, 2014 6:46 PM, "David Holmes" mailto:david.hol...@oracle.com>> wrote:

Hi Aleksej,

Thanks for the doc references regarding abandonment.

Let me rephrase my question. What is this logic trying to achieve by
waiting on both a mutex and an event? Do we already own the mutex
when this function is called?

David

On 13/05/2014 11:19 PM, Aleksej Efimov wrote:

David,

The Windows has a different terminology for mutex objects (much
differs
from the POSIX one). This one link gave me some understanding of
it [1].

Here is the MSDN [1] description of what "abandoned mutex" is:
" If a thread terminates without releasing its ownership of a mutex
object, the mutex object is considered to be abandoned. A
waiting thread
can acquire ownership of an abandoned mutex object, but the wait
function will return*WAIT_ABANDONED*to indicate that the mutex
object is
abandoned. An abandoned mutex object indicates that an error has
occurred and that any shared resource being protected by the mutex
object is in an undefined state. If the thread proceeds as
though the
mutex object had not been abandoned, it is no longer considered
abandoned after the thread releases its ownership. This restores
normal
behavior if a handle to the mutex object is subsequently
specified in a
wait function."


What does it mean to wait on mutex and ownership of the mutex
object:
"Any thread with a handle to a mutex object can use one of thewait
functions

>to
request ownership of the mutex object. If the mutex object is
owned by
another thread, the wait function blocks the requesting thread
until the
owning thread releases the mutex object using the*ReleaseMutex*

>__function."

How we can release mutex and wait on already owned mutex:
" After a thread obtains ownership of a mutex, it can specify
the same
mutex in repeated calls to the wait-functions

>__without
blocking its execution. This prevents a thread from deadlocking
itself
while waiting for a mutex that it already owns. To release its
ownership
under such circumstances, the thread must call*ReleaseMutex*

>__once
for each time that the mutex satisfied the conditions of a wait
function."

[1]

http://msdn.microsoft.com/en-__gb/library/windows/desktop/__ms684266(v=vs.85).aspx



-Aleksej

On 05/13/2014 04:00 PM, David Holmes wrote:

I don't understand this one at all. What is an "abandoned
mutex"? For
that matter why does the code wait on a mutex and an event?
Do we
already own the mutex? If so what does it mean to wait on
it? If not
then how can we release it?

???

Thanks,
David


On 13/05/2014 8:57 PM, Alan Bateman wrote:


This is debugger's shared memory transport so cc'ing
serviceability-dev
as that is there this code is maintained.

Is there a test case or any outline of the conditions
that cause this? I
 

Re: [OpenJDK 2D-Dev] JDK-8041679 Replace uses of StringBuffer with StringBuilder within the JDK

[Paul Sandoz]

HI Phil,

Thanks for looking at this.

On May 13, 2014, at 11:15 PM, Phil Race  wrote:

> Paul,
> 
> I don't see why you changed the variable names in some cases.

Note it's not me :-) I am, mostly, the proxy.


> See here where one change is only one line since you left it alone and the 
> other is 6 lines since you changed it
> http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-media/webrev/src/share/classes/javax/print/attribute/Size2DSyntax.java.sdiff.html
> As it is, its just inconsistent and makes it less obvious to the eye that 
> nothing unexpected changed.

That's a fair point.


> And here what could have been a 2 line change is 25 ..
> http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-media/webrev/src/share/classes/sun/font/StandardTextSource.java.sdiff.html
> So I would say leave the variable names alone unless there's a compelling 
> reason - and I don't see one.

"buf" no longer corresponds to a buffer but to a builder, so i think it is 
reasonable in this case to use the canonical "sb" name.


> Also pushing the 2D, AWT and Swing changes to client is requested as although 
> your changes are
> small its what is appropriate. I would not push hotspot changes to client 
> either. Also lots of files
> are being updated in client and doing it this way will minimise merges ...

Although hotspot is a *separate* repository, there is a much clearer dividing 
line. 

This code is under src/share/classes, so it's not clear to me what classes i 
can modify and push in the jdk9-dev/jdk repo or not. It's confusing! Is there a 
list online somewhere?

Out of all the classes here:

  
http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-media/webrev/

exactly which ones should be pushed to the client forest and which ones to the 
jdk9-dev forest? all of them, or just a sub-set?

How long would it take for the changes to the client jdk repo to make it's way 
into the jdk9-dev jdk repo?

Really what i am getting at here is i think we need to change our process of 
how we manage such forests; we need to consolidate.

Paul.