gmane.comp.java.openjdk.core-libs.devel

Re: Code Review Request: 7164636: (prefs) Cleanupsrc/macosx/classes/java/util/prefs

Chris Hegarty — 2012-05-15T18:00:24

This looks fine to me.

-Chris.

hg: jdk8/tl/jdk: 7164191: properties.putAll API may fail withConcurrentModifcationException on multi-thread scenario

2012-05-15T12:49:20

Changeset: df33f5f750ec
Author:    dsamersoff
Date:      2012-05-15 16:46 +0400
URL:       http://hg.openjdk.java.net/jdk8/tl/jdk/rev/df33f5f750ec

7164191: properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario
Reviewed-by: dholmes, sla
Contributed-by: Deven You <youdwei-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8< at >public.gmane.org>

! src/share/classes/sun/management/Agent.java
+ test/sun/management/AgentCMETest.java

Re: Code Review Request: 7164636: (prefs) Cleanupsrc/macosx/classes/java/util/prefs

David Holmes — 2012-05-15T02:24:33

I do not agree.

The above can only print zero if program-order is violated, which I 
don't believe it can or should be.

But the "set of writes" is the same regardless of whether default or 
explicit initialization is used. The JMM explicitly states (17.4.4) that 
the write of the default value synchronizes with the first action in 
every thread and acts as-if each object were allocated and initialized 
to default values when the VM starts.


Initializing to a default value should be a no-op. I had thought javac 
already optimized these away but perhaps it is only the JIT.

David
-----

JSR 310 Date and Time Method naming

Roger Riggs — 2012-05-14T20:40:33

Hi,

JSR 310 is developing a new date and time API expected to be included in 
the JDK.
We would appreciate your input to help refine the method naming 
alternatives.

Please a take few minutes to give your input.
         http://www.rationalsurvey.com/s/3775

Thanks, Roger

Re: Code Review Request: 7164636: (prefs) Cleanupsrc/macosx/classes/java/util/prefs

Kurchi Hazra — 2012-05-14T20:00:11

Thanks for the detailed explanation.
I have removed the default initializations: 
http://cr.openjdk.java.net/~khazra/7164636/webrev.01/

- Kurchi

On 5/14/2012 10:07 AM, Chris Hegarty wrote:

Re: Code Review Request: 7164636: (prefs) Cleanupsrc/macosx/classes/java/util/prefs

Chris Hegarty — 2012-05-14T17:07:46

I always thought that removing these superfluous "default" values would 
be a good clean up opportunity.

Maybe one to be added to the warnings/StringBuilder potential cleanup 
targets for external folks?

-Chris.

On 14/05/12 17:41, Brian Goetz wrote:

Re: Code Review Request: 7164636: (prefs) Cleanupsrc/macosx/classes/java/util/prefs

Brian Goetz — 2012-05-14T16:41:54

 From a concurrency perspective it is also preferable to NOT initialize 
variables to their default values, as doing so can cause some weird 
problems.  For example:

class A {
     public int x = 0;

     public void increment() { ++x; }
     public int get() { return x; }
}

// Thread X
// Assume: Thread X never touches 'a' again
A a = new A();

// Thread Y
// Assume: No other thread than Y touches 'a'
if (a != null) {
     a.increment();
     System.out.println(a.get());
}

With the explicit initialization, this code could print zero (because 
the set of writes to 'x' contains two writes, one by X to zero and one 
by Y to 1), whereas without the explicit initialization, it would always 
print one.

Now, this is an example of "improper publication" of the A by Thread X, 
but this is the sort of improper publication (where an object was 
initialized by one thread and then "handed off" to another) that was 
widely thought to be safe a long time ago and was enshrined in many 
examples, particularly Swing exam

Re: Code Review Request: 7164636: (prefs) Cleanupsrc/macosx/classes/java/util/prefs

Chris Hegarty — 2012-05-14T09:28:32

This change looks fine to me.

Trivially, changedFiles and cachedFiles do not need to be set to null (I 
believe this will generate a little less bytecode), as this is the 
default reference value. Since you are in clean-up mode ;-)

-Chris.

On 11/05/2012 22:46, Kurchi Hazra wrote:

Re: review request: 4244896: (process) Provide System.getPid(),System.killProcess(String pid)

David Holmes — 2012-05-14T06:22:39

Hi Rob,

Your specification for Process.waitFor(long timeout, TimeUnit unit) has 
a conflict. You presently say:

"If the subprocess has already exited then this method returns
immediately with the value {< at >code true}"

and you also say:

"If the current thread:
   has its interrupted status set on entry to this method; or ...
then {< at >link InterruptedException} is thrown and the current thread's
interrupted status is cleared."

It can't do both. The code implements the first statement. The IE will 
only be thrown if the thread will actually invoke sleep(). I suggest:

+  * <p>If the current thread:
+  * <ul>
+   * <li>has its interrupted status set when it would wait; or
+   * <li>is {< at >linkplain Thread#interrupt interrupted} while waiting,

Also note that the documentation for:

public abstract int waitFor() throws InterruptedException;

should be updated to properly document its interruption response, in 
line with what is said for the waitFor(timeout) method.
---

In UNIXProcess.java as noted already you haq

Re: Review Request : 7071826 : Avoid benign race condition ininitialization of UUID

Brian Goetz — 2012-05-12T18:58:36

Oops, forgot to actually review the patch: approved.

On 5/10/2012 7:11 PM, Mike Duigou wrote:

Re: Review Request : 7071826 : Avoid benign race condition ininitialization of UUID

Brian Goetz — 2012-05-12T18:57:34

Farther afield: the "Holder" idiom for thread-safe lazy initialization 
is one that we could now replace with invokedynamic (since a 
ConstantCallSize acts as a lazily initialize cache), if there were only 
a way to actually express the indy code from Java.  This would let us 
replace a class with a call site.

On 5/10/2012 7:11 PM, Mike Duigou wrote:

Code Review Request: 7164636: (prefs) Cleanupsrc/macosx/classes/java/util/prefs

Kurchi Hazra — 2012-05-11T21:46:21

Hi,

    This change aims at removing some rawtype usages in 
src/macosx/classes/java/util/prefs that were brought
into jdk8 with the macport. I have added < at >Override tags too where 
applicable.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7164636
Webrev: http://cr.openjdk.java.net/~khazra/7164636/webrev.00/


Thanks,
Kurchi

Re: Review Request : 7071826 : Avoid benign race condition ininitialization of UUID

Mike Duigou — 2012-05-11T18:13:10

On May 11 2012, at 03:12 , Alan Bateman wrote:


Yes.


It doesn't demonstrate the race condition. The test changes are mostly just a result of reviewing the tests while I planned the changes.


According to javastyle guidelines we're supposed to always use braces. I've not modified existing cases where braces are not present unless modifying the if expression.


Fixed.

Re: review request: 4244896: (process) Provide System.getPid(),System.killProcess(String pid)

Paul Sandoz — 2012-05-11T15:51:15

Yes, or you could use:

   wait(long timeout, int nanos)

Paul.

Re: review request: 4244896: (process) Provide System.getPid(),System.killProcess(String pid)

Rob McKenna — 2012-05-11T15:24:18

Hi Paul,

Comments inline:

On 11/05/12 12:29, Paul Sandoz wrote:
Thanks for catching that.
Eesh. wait(0) will wait until notified so we could end up waiting past 
our timeout expiry. Would:

wait(Math.max(TimeUnit.NANOSECONDS.toMillis(rem), 1));

alleviate all concerns here?

     -Rob

Re: Request for CR : 7144861 RMI activation tests are too slow(webrev.02)

Olivier Lagneau — 2012-05-11T14:09:17

I made the changes below and checked again all the rmi regression test.

This updated webrev is here :
http://cr.openjdk.java.net/~olagneau/7144861/webrev.02/

Olivier

Olivier Lagneau said  on date 5/11/2012 12:18 PM:

Re: review request: 4244896: (process) Provide System.getPid(),System.killProcess(String pid)

Paul Sandoz — 2012-05-11T11:29:17

Hi Rob,

I dunno if the following has been pointed out before. It's hard to track review comments.


The implementation of Process.waitFor normalizes the end time and duration remaining time to nano seconds.  However, the Thread.sleep method expects a duration in units of milliseconds.

You could use:

  http://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#sleep(long, int)

Or just round things up to the nearest millisecond:

  Thread.sleep(Math.min(TimeUnit.NANOSECONDS.toMillis(rem) + 1, 100));

That would be more consistent and wait for less time over that of the requested duration.


For the following code:

 227         long rem = end - now;
 228         while (!hasExited && (rem > 0)) {
 229             wait(TimeUnit.NANOSECONDS.toMillis(rem));
 230             rem = end - System.nanoTime();
 231         }

Can the above go into a spin loop once the duration is less than 1 millisecond for the rest of that duration? If so you may want to round it up to the nearest millisecond.

Paul.

On May 10

Re: Review Request for CR : 7144861 RMI activation tests are tooslow (wrong webev link fixed)

Olivier Lagneau — 2012-05-11T10:18:35

Stuart Marks said  on date 5/11/2012 3:04 AM:
Ok given Darryl's additional comment (see below), I will suppress this 
comment.


I am going to provide another webrev containing Darryl's plus your 
change requests within a couple of hours.

Olivier.

Re: Review Request : 7071826 : Avoid benign race condition ininitialization of UUID

Alan Bateman — 2012-05-11T10:12:15

The implementation change looks good to me. For the javadoc change then 
could you link to RFC 4122 on the IEFT site?

I don't quite get the test update, it improves the test but I don't see 
how it might demonstrate the race condition. Minor consistency comment 
is that the if statements use braces whereas the existing code doesn't, 
also missing space in "if(".

-Alan.

Re: Review Request : 7071826 : Avoid benign race condition ininitialization of UUID

Chris Hegarty — 2012-05-11T10:05:33

The changes look good to me.

-Chris.

On 11/05/12 00:11, Mike Duigou wrote:

Re: Review Request for CR : 7144861 RMI activation tests are tooslow (wrong webev link fixed)

David Holmes — 2012-05-11T01:14:57

IE handling seems fine. No further comments from me.

Note I was not a full reviewer of this.

David

On 11/05/2012 11:04 AM, Stuart Marks wrote: