Re: Review request JDK-8004729: Parameter Reflection API

[Remi Forax]

On 01/10/2013 07:23 AM, Joe Darcy wrote:

Hi Eric,

A few quick comments.  In Executable,

 228 /**
 229  * Returns the number of formal parameters for the executable
 230  * represented by this object.
 231  *
 232  * @return The number of formal parameters for the executable 
this

 233  * object represents
 234  */
 235 public abstract int getNumParameters();

I think it is important of the getNumParameters javadoc is clarified 
to state ... represented by this object, including any synthesized 
and synthetic parameters.  In other words, the number of parameters 
of the VM view of the query and *not* (necessarily) the language view.


And please rename getNumParameters to getParameterCount, so all methods 
related to parameters starts with the same prefix.




Also in Executable, I think the body of

 279 public Parameter[] getParameters() {
 280 // TODO: This may eventually need to be guarded by security
 281 // mechanisms similar to those in Field, Method, etc.
 282 Parameter[] raw = privateGetParameters();
 283 Parameter[] out = new Parameter[raw.length];
 284 // Need to copy the cached array to prevent users from 
messing

 285 // with it
 286 for (int i = 0; i  raw.length; i++) {
 287 out[i] = new Parameter(raw[i]);
 288 }
 289 return out;
 290 }

could be replaced with

return privateGetParameters().clone();

IIRC, other parts of core reflection have similar calls to clone.


yes, and the copy constructor in Parameter is not needed (we are in Java 
after all, not in C++).
Also null == foo is not a standard idiom in Java because there is no 
operator overloading

(likewise i++ doesn't need to be replaced by ++i).



I would prefer to see the getNumParameters method in Executable be 
declared to be non-abstract, but with a body that threw some kind of 
exception, even an abstract-method-error.  Since only types within 
java.lang.reflect can subclass Executable, it is not generally 
extensible.  Alternate implementations of java.lang.reflect should not 
be forced to not share a getNumParameters implementation from Executable.


yes !



All the public methods and constructors in java.lang.reflect.Parameter 
need proper javadoc.


I strongly recommend rewriting the tests in the style used, for 
example, here:


http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0a1398021c7c

where an annotation is used to record the expect result and then a 
simple checker loops over the reflective constructs and verifies that 
the computed values match the expected ones.


The tests should include cases where the VM has a different notion of 
the number of parameter than the language; typical cases are 
constructors of inner classes (compiler is required to insert a 
leading other$this parameter) and constructors of enums (javac 
prepends two synthesized parameters).


Cheers,

-Joe


cheers,
Rémi



On 01/09/2013 01:55 PM, Eric McCorkle wrote:

Hello,

Please review the core reflection API implementation of parameter
reflection.  This is the final component of method parameter reflection.
  This was posted for review before, then delayed until the check-in for
JDK-8004728 (hotspot support for parameter reflection), which occurred
yesterday.

Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, *not*
jdk8/tl; therefore, it may be a while before the changeset makes its way
into jdk8/tl.

Also note: since the check-in of JDK-8004727 (javac support for
parameter reflection), there has been a failure in the tests for
Pack200.  This is being addressed in a fix contributed by Kumar, which I
believe has also been posted for review.

The open webrev is here:
http://cr.openjdk.java.net/~coleenp/JDK-8004729

The feature request is here:
http://bugs.sun.com/view_bug.do?bug_id=8004729

The latest version of the spec can be found here:
http://cr.openjdk.java.net/~abuckley/8misc.pdf


Thanks,
Eric

Re: Review request JDK-8004729: Parameter Reflection API

[Paul Sandoz]

Hi Eric,

In Parameter.java:

--

  31 public final class Parameter implements AnnotatedElement {
  32 
  33 private String name;
  34 private int modifiers;
  35 private Executable executable;
  36 private int index;
  37 

Can all the above fields be marked as final?

--

 172 public T extends Annotation T getDeclaredAnnotation(ClassT 
annotationClass) {
 173 if (annotationClass == null)
 174 throw new NullPointerException();

Objects.requireNonNull(annotationClass);

--

 179 public T extends Annotation T getDeclaredAnnotations(ClassT 
annotationClass) {

 202 public T extends Annotation T getAnnotations(ClassT 
annotationClass) {

Should return T[] i presume. Likewise the spec needs to be updated on page 25.

Paul.

On Jan 9, 2013, at 10:55 PM, Eric McCorkle eric.mccor...@oracle.com wrote:

 Hello,
 
 Please review the core reflection API implementation of parameter
 reflection.  This is the final component of method parameter reflection.
 This was posted for review before, then delayed until the check-in for
 JDK-8004728 (hotspot support for parameter reflection), which occurred
 yesterday.
 
 Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, *not*
 jdk8/tl; therefore, it may be a while before the changeset makes its way
 into jdk8/tl.
 
 Also note: since the check-in of JDK-8004727 (javac support for
 parameter reflection), there has been a failure in the tests for
 Pack200.  This is being addressed in a fix contributed by Kumar, which I
 believe has also been posted for review.
 
 The open webrev is here:
 http://cr.openjdk.java.net/~coleenp/JDK-8004729
 
 The feature request is here:
 http://bugs.sun.com/view_bug.do?bug_id=8004729
 
 The latest version of the spec can be found here:
 http://cr.openjdk.java.net/~abuckley/8misc.pdf
 
 
 Thanks,
 Eric

Re: Review request JDK-8004729: Parameter Reflection API

[Peter Levart]

Hello Eric,

You must have missed my comment from the previous webrev:

 292 private Parameter[] privateGetParameters() {
 293 if (null != parameters)
 294 return parameters.get();

If/when the 'parameters' SoftReference is cleared, the method will be 
returning null forever after...


You should also retrieve the referent and check for it's presence before 
returning it:


Parameter[] res;
if (parameters != null  (res = parameters.get()) != null)
return res;
...
...

Regards, Peter

On 01/09/2013 10:55 PM, Eric McCorkle wrote:

Hello,

Please review the core reflection API implementation of parameter
reflection.  This is the final component of method parameter reflection.
  This was posted for review before, then delayed until the check-in for
JDK-8004728 (hotspot support for parameter reflection), which occurred
yesterday.

Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, *not*
jdk8/tl; therefore, it may be a while before the changeset makes its way
into jdk8/tl.

Also note: since the check-in of JDK-8004727 (javac support for
parameter reflection), there has been a failure in the tests for
Pack200.  This is being addressed in a fix contributed by Kumar, which I
believe has also been posted for review.

The open webrev is here:
http://cr.openjdk.java.net/~coleenp/JDK-8004729

The feature request is here:
http://bugs.sun.com/view_bug.do?bug_id=8004729

The latest version of the spec can be found here:
http://cr.openjdk.java.net/~abuckley/8misc.pdf


Thanks,
Eric

Re: JDK 8 code review request for 8005298 Add FunctionalInterface type to the core libraries

[Florian Weimer]

On 01/08/2013 10:24 PM, Joe Darcy wrote:

As discussed over on one of the Project Lambda lists [1], we're adding
an interface type to the platform to explicitly mark interface types as
being functional interfaces suitable for use in lambda expressions.
Please review the addition of this new type:

 http://cr.openjdk.java.net/~darcy/8005298.0/


Shouldn't this annotation have @Retention(RetentionPolicy.SOURCE), 
similar to the @Override annotation?


--
Florian Weimer / Red Hat Product Security Team

Re: Final RFR: 8005232 (JEP-149) Class Instance size reduction

[Peter Levart]

On 01/10/2013 02:25 AM, Mandy Chung wrote:

On 1/6/2013 2:46 PM, David Holmes wrote:


http://cr.openjdk.java.net/~dholmes/8005232/webrev/


This looks good to me. David - besides the footprint performance data,
do you observe any performance difference in your performance testing
(I think refworkload is what you ran)?

On 01/09/2013 01:19 PM, Aleksey Shipilev wrote:

On 01/09/2013 05:04 PM, Peter Levart wrote:


Strictly speaking, CAS is actually not needed here. Since we keep
classRedefinedCount snapshot inside the ReflectionData, any races that
would install older ReflectionData over newer, would be quickly
caught at next invocation to reflectionData(). So if there are any
objections to usage of Unsafe, it can be removed and replaced by simple
volatile write.


Yes, I think that would be more clear without any adverse impacts on
performance. Also, that better expresses the intent of requiring the
visibility, not the atomicity of cache update.


I agree with Alekesey that it'd be simpler and clearer if CAS is replaced
with the simple volatile write.  But this change will require another 
round
of performance measurement to determine its impact.  I'd suggest to 
follow

up this cleanup in a separate bug and changeset.

Mandy
FWIW, I ran my micro benchmarks with the variant of using simple 
volatile write instead of CAS and there was no visible differences. But 
I don't have access to 256 core machines ;-). Anyway I think It should 
not present any performance difference, since the rate of class 
re-definitions is usually not very high...


Although the slow-path is a simple as:

this.reflectionData = new SoftReference(rd = new 
ReflectionData(classRedefinitionCount));

return rd;

... I had to split this too into a separate method to retain the same 
inlienability.


Such fine-tunnings tend to be lost in other sub-optimalities of the code 
that implements public reflection API, such as copying the arrays and 
array elements in bulk-returning methods and linear searches for the 
elements in single-element returning methods...


There is another simplification of code possible. The method 
checkInited() and its usages could be removed and it's logic 
incorporated into the slow-path newReflectionData() method while adding 
another boolean variable test into the fast-path.


Regards, Peter

Re: RFR: 8005582 - java/lang/Runtime/exec/WinCommand.java intermittent test failures

[Chris Hegarty]

On 01/10/2013 12:59 AM, Stuart Marks wrote:

On 1/9/13 11:33 AM, Martin Buchholz wrote:

On Wed, Jan 9, 2013 at 10:49 AM, Jim Gish jim.g...@oracle.com wrote:


I'm in the process of adding deletion retry behavior to jtreg, but in
the
meantime we think it makes sense to provide a more stable test
environment
by simply getting rid of the redundant deletes by the test itself.
There
really is no need for tests to delete files from the scratch
directory at
the end of a test because jtreg carries a guarantee of cleanup.



I consider it good practice for a test to delete temporary files - it's
just a java program that might be run outside of the test harness.


I'm in favor of tests cleaning up after themselves too, but am 
sympathetic to this issue ( I've seen this test fail too many times! ).


 I'm in the process of adding deletion retry behavior to jtreg, but in
 the meantime we think it makes sense to provide a more stable test

I guess you could call these changes a workaround to the issue with 
the jtreg harness. Once Jim adds deletion retry behavior to jtreg 
these changes can be reverted, and the test can go back to being a good 
standalone citizen, right? If so, I agree with the changes.


-Chris.

Re: RFR: 8005582 - java/lang/Runtime/exec/WinCommand.java intermittent test failures

[Alan Bateman]

On 09/01/2013 19:46, Jim Gish wrote:
It's a Windows feature.  We discovered this recently in debugging 
another test failure.  Windows is documented to do asynchronous 
deletes.  You can't depend on a file.delete() which returns true to 
have actually deleted the file.  It may be the case that another 
process has a file handle which it has not yet released, or it's 
simply a delay.
I don't get this, the issue sounds more like AV software or Windows 
application quality service/agent thing accessing the file but I might 
be wrong of course. Are you able to duplicate this reliably and if so, 
have you looked at it with any tools to see what/who is accessing it 
that is causing the delay?


-Alan.

Re: RFR 8005311: Add Scalable Updatable Variables, DoubleAccumulator, DoubleAdder, LongAccumulator, LongAdder

[Peter Levart]

On 01/10/2013 02:23 AM, Doug Lea wrote:

On further consideration...

On 01/08/13 10:01, Peter Levart wrote:

- accumulate(long x) returns the post-modification value of the 
modified cell or
base (the returned value of the function that was used to update the 
state)
- the accumulator function is always called for initial allocations 
of cells
(with identity value as 1st argument, like when accumulating on the 
base) - the
original code optimizes this situation and just installs the 
parameter x into

the cell.



... I'm no longer seeing a reason to support this kind of use,
even with protected methods.
The particular cells used, even for a particular thread, can
and do change over time, so returning the pre-accumulate value for the
cell used means only this was at some moment a partial accumulation
value. The next one returned after another call might be completely
unrelated. The only possible uses I can imagine, for example a
not-at-all random progress sampling mechanism, can be done in
better ways. And as you showed, while you could make a sort of
RNG out of it, it is not competitive with ThreadLocalRandom,
and has unknowable statistical properties.
So for now anyway, I don't plan on doing this.
Another use could be a unique identifier generator. But all that is 
possible with ThreadLocal also...




Thanks for the opportunity to do these thought experiments though :-)
A related question: What do you think of a variant of 
[Double|Long]Adder.sumThenReset like the following:


/**
 * Like {@link #sumThenReset()}, but with a guarantee that this 
adder's amount
 * is decreased by the same exact amount that was returned. 
Invoking this method
 * during frequent concurrent updates can disturb concurrent 
threads slightly,

 * so it is not advisable to call it very frequently...
 */
public long drain() {
Cell[] as = cells; Cell a;

long sum = gasBase(0L);
if (as != null) {
for (int i = 0; i  as.length; ++i) {
if ((a = as[i]) != null) {
sum += a.gas(0L);
}
}
}
return sum;
}

with the following additions in Striped64, using new Unsafe intrinsics:

final long gasBase(long val) {
return UNSAFE.getAndSetLong(this, BASE, val);
}

static final class Cell {
...
final long gas(long val) {
return UNSAFE.getAndSetLong(this, valueOffset, val);
}


Regards, Peter



-Doug

Re: RFR: 4247235 (spec str) StringBuffer.insert(int, char[]) specification is inconsistent

[Stephen Colebourne]

I would encourage null checking to be done first, rather than
sometimes getting StringIndexOutOfBoundsException for a null input.
Reasoning about the JDK methods is much easier that way around.

Stephen


On 9 January 2013 23:09, Mike Duigou mike.dui...@oracle.com wrote:
 AbstractStringBuilder probably needs the Unless otherwise noted, blanket 
 statement as well (Same as StringBuffer/StringBuilder)

 You might want to move the Objects.requireNonNull(dst); in String.getBytes() 
 to after the existing checks so as not to unnecessarily change the exception 
 thrown for bad inputs. An exception will still be thrown but some bad code 
 may anticipate StringIndexOutOfBoundsException rather than NPE. This is a 
 very minor matter though.

 Otherwise, it looks good.

 Mike

 On Jan 9 2013, at 14:46 , Jim Gish wrote:

 Please review the following:

 Webrev: http://cr.openjdk.java.net/~jgish/Bug4247235-Add-Blanket-Null-Stmt/ 
 http://cr.openjdk.java.net/%7Ejgish/Bug4247235-Add-Blanket-Null-Stmt/
 Here's a specdiff: 
 http://cr.openjdk.java.net/~jgish/Bug4247235-string-specdiff/ 
 http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/


 Summary:  This change replaces java/lang/*String*.java javadoc, 
 method-specific @throws NullPointerException clauses with  blanket 
 null-handling statements like that currently in String.java

 That was motivated by a discussion awhile back, strongly favoring a blanket 
 statement over individual @throws clauses.

 For String, the following blanket statement is already in place:

 * p Unless otherwise noted, passing a ttnull/tt argument to a 
 constructor
 * or method in this class will cause a {@link NullPointerException} to be
 * thrown.


 However, despite the blanket statement, the following methods also have 
 @throws clauses:

 public boolean contains(java.lang.CharSequence s)

 Throws:
   |java.lang.NullPointerException|- if|s|is|null|
 ---

 public staticString  
 http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html
   format(String  
 http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html
   format,
java.lang.Object... args)


 Throws:
 |java.lang.NullPointerException|- If the|format|is|null
 |---
 ||

 public staticString  
 http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html
   format(java.util.Locale l,
String  
 http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html
   format,
java.lang.Object... args)

 Throws:
 |java.lang.NullPointerException|- If the|format|is|null
 |--
 All of the above are properly specified with the blanket statement or other 
 parts of their spec (such as format w.r.t. null Locale) and the @throws can 
 safely be removed.

 Because the blanket statement is already in effect for String.java, I wrote 
 tests for all methods/constructors to ensure compliance.  Running them 
 revealed the following:

 public void getBytes(int srcBegin, int srcEnd, byte dst[], int dstBegin)
 - I would expect an NPE to be thrown if dst == null.  However, this isn't 
 always the case.  If dst isn't accessed because of the values of the other 
 parameters (as in getBytes(1,2,(byte[]null,1), then no NPE is thrown.
 - This brings up the question as to the correctness of the blanket statement 
 w.r.t. this method.  I think this method (and others like it) should use 
 Objects.requireNonNull(dst).  (The correspoding method public void 
 getChars(int srcBegin, int srcEnd, char dst[], int dstBegin), does always 
 throw NPE for dst==null)

 All other methods/constructors in StringBuffer and StringBuilder either 
 correctly state null-handling behavior that differs from the blanket 
 statement or are correct w.r.t. the blanket statement.

 This has been reviewed by the JCK team.  It will require CCC approval 
 (because of the new blanket statement, and the fact that some of the 
 existing methods were previously silent on null handling, but all already 
 conform to the blanket statement).

 Thanks,
Jim Gish


 --
 Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
 Oracle Java Platform Group | Core Libraries Team
 35 Network Drive
 Burlington, MA 01803
 jim.g...@oracle.com

 --
 Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
 Oracle Java Platform Group | Core Libraries Team
 35 Network Drive
 Burlington, MA 01803
 jim.g...@oracle.com

Re: javax.sql.rowset.serial.SerialBlob doesn't support free and getBinaryStream

[Lance Andersen - Oracle]

Deven,

This was pushed a while ago you should have seen the putback on the alias

See http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7a8978a5bb6e

Best
Lance


On Jan 10, 2013, at 12:49 AM, Deven You wrote:

 Hi Lance,
 
 Is there any update for this issue. If you have anything I can do, please let 
 me know.
 
 Thanks a lot!
 On 11/24/2012 12:45 AM, Lance Andersen - Oracle wrote:
 It is on my list.  to update the javadocs I need a ccc which I have not done 
 yet and is needed as part of this change
 
 On Nov 23, 2012, at 3:07 AM, Deven You wrote:
 
 Hi Lance,
 
 Is there any plan for this issue, if any could you update to me?
 
 Thanks a lot!
 On 10/29/2012 06:39 PM, Lance Andersen - Oracle wrote:
 Hi Deven,
 
 I will address the needed updates a bit later.
 
 Thank you for your input
 
 Best
 Lance
 On Oct 29, 2012, at 3:51 AM, Deven You wrote:
 
 Hi Alan,
 
 The Java Spec does not mention the thread safe for JDBC API. But I see 
 the other code in SerialBlob/SerialClob have not consider it.
 
 I think use buff == null to replace isFree is a good idea because it also 
 avoid the problem for the condition buf == null  isFree == false so we 
 won't need create a readObject method.
 
 Thanks for your suggestion for isFree, I will correct it later.
 
 Lance: How about your suggestion? Since you mentioned you will develop 
 the implementation yourself. I use my implementation mainly for the test 
 cases. But you may also take a look my implementation.
 
 Thanks a lot!
 
 On 09/21/2012 04:37 PM, Alan Bateman wrote:
 On 21/09/2012 04:21, Deven You wrote:
 Hi Lance,
 
 I am very busy with other work so I can't work with the 
 SerialBlob/SerialClob item for long time. I am very happy to refine the 
 current test case and create new tests for SerialClob.
 
 I have create a new webre[1] for this task, please review it.
 
 [1] http://cr.openjdk.java.net/~youdwei/OJDK-576/webrev.01/ 
 http://cr.openjdk.java.net/%7Eyoudwei/OJDK-576/webrev.01/
 
 PS: If the isFree is not transient, I want to know how we add this 
 field to the javadoc serialized form?
 I don't know very much about the rowset API and I can't see anything to 
 specify whether it is meant to be safe for use by concurrent threads. 
 There are clearly lots of issues here and implementing free introduces a 
 lot more, especially with the possibility of an asynchronous free or 
 more than one thread calling free at around the same time.
 
 Have you considered buf == null to mean that the resources are freed? 
 That might avoid needing to change the serialized form. Also as these 
 types are serializable it means you have to consider the case where you 
 deserialize to buf == null  isFree == false for example. On that 
 point, it looks to me that this code needs a readObject anyway (for 
 several reasons).
 
 A small point is that isFree is a odd name for a method that doesn't 
 return a boolean. If the patch goes ahead then I think it needs a better 
 name, ensureNotFree or requireNotFree or something like that.
 
 -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
 
 
 
 
 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering 
 1 Network Drive 
 Burlington, MA 01803
 lance.ander...@oracle.com
 
 


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

Re: Improving performance and reducing object allocations of java.util.UUID to/from string

[Aleksey Shipilev]

On 01/09/2013 09:51 PM, Steven Schlansker wrote:
 Hello again,
 
 I sent this email a week ago and have received no replies.  Is there
 any step I have missed necessary to contribute to the JDK libraries?

I think the crucial part is OCA, as per:
 http://openjdk.java.net/contribute/

 I am very interested in making your lives easier, so please let me
 know if I am in the wrong place or are otherwise misguided.

You are at the correct place.

On the first glance, the change looks good for the start. A few comments
though:
 a) Do you need the masks before or-ing with most/leastSigBits?
 b) Is there a more standard (and still performant) way to do the hex
conversion? Look around JDK source, I think there should be something
else needing the same kind of conversion.
 c) I'd go for making utility methods a bit more generic. For one, I
would rather make decodeHex(String str, int start, int end), and
encodeHex(char[] dest, int offset, int value).

Microbenchmark glitches:
 a) % is the integer division, and at the scale of the operations you
are measuring, it could incur significant costs; the usual practice is
having power-of-2 size, and then (i % size) - (i  (size - 1)).
 b) Not sure if you want to stick with random UUIDs for comparisons.
While the law of large numbers is on your side, 1000 random UUIDs might
be not random enough.

-Aleksey.

8005978: shell tests need to use the $COMPILEJDK for javac, jar and other tools

[Alan Bateman]

I'm currently trying to run the jdk tests on the compact profiles. As 
the profile builds are runtimes only, they don't have tools such as 
javac and so need to be run with both -jdk and -compilejdk. That works 
okay except for the shell tests because many of them launch the javac 
or jar tools directly and so fail because the runtime under test 
($TESTJAVA) is not a JDK. I'd like to update the shell tests to use the 
tools in $COMPILEJAVA, this is the JDK specified to jtreg with 
-compilejdk or it's the same as $TESTJAVA when -compilejdk is not specified.


The webrev with the proposed changes is here:

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

For the most part this is just s/TESTJAVA/COMPILEJAVA/ although there 
are a couple of subtle things to watch out for - for example tool tests 
should be testing the tools in $TESTJAVA, not $COMPILEJAVA. Many of the 
shell tests were created to be runnable outside of jtreg so I've 
preserved this where it was previously supported.


I should note that this is not all of the shell tests, I don't have time 
to fix the tests in areas that aren't interesting for the profiles so 
contributions to do more would be welcome.  Another thing is that a lot 
of these shell tests are old tests and a lot of them don't really need 
to be shell tests. I mention this because another great effort would be 
be gradually replace many of these shell tests.


-Alan.

Re: javax.sql.rowset.serial.SerialBlob doesn't support free and getBinaryStream

[Lance Andersen - Oracle]

The JCK and RowSet TCK will have base tests to validate.  I have a separate set 
of unit tests for JDBC which includes more detailed tests than what you had 
suggested as well as end to end tests.

Best
Lance 
On Jan 10, 2013, at 8:24 AM, Deven You wrote:

 Hi Lance,
 
 I am glad to see the patch for implementing these methods are committed.
 
 Do you have a plan to add the test cases[1] I created too?
 
 Thanks a lot!
 
 [1]http://cr.openjdk.java.net/~youdwei/OJDK-576/webrev.01/
 
 
 On 01/10/2013 08:31 PM, Lance Andersen - Oracle wrote:
 Deven,
 
 This was pushed a while ago you should have seen the putback on the alias
 
 See http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7a8978a5bb6e
 
 Best
 Lance
 
 
 On Jan 10, 2013, at 12:49 AM, Deven You wrote:
 
 Hi Lance,
 
 Is there any update for this issue. If you have anything I can do, please 
 let me know.
 
 Thanks a lot!
 On 11/24/2012 12:45 AM, Lance Andersen - Oracle wrote:
 It is on my list.  to update the javadocs I need a ccc which I have not 
 done yet and is needed as part of this change
 
 On Nov 23, 2012, at 3:07 AM, Deven You wrote:
 
 Hi Lance,
 
 Is there any plan for this issue, if any could you update to me?
 
 Thanks a lot!
 On 10/29/2012 06:39 PM, Lance Andersen - Oracle wrote:
 Hi Deven,
 
 I will address the needed updates a bit later.
 
 Thank you for your input
 
 Best
 Lance
 On Oct 29, 2012, at 3:51 AM, Deven You wrote:
 
 Hi Alan,
 
 The Java Spec does not mention the thread safe for JDBC API. But I see 
 the other code in SerialBlob/SerialClob have not consider it.
 
 I think use buff == null to replace isFree is a good idea because it 
 also avoid the problem for the condition buf == null  isFree == false 
 so we won't need create a readObject method.
 
 Thanks for your suggestion for isFree, I will correct it later.
 
 Lance: How about your suggestion? Since you mentioned you will develop 
 the implementation yourself. I use my implementation mainly for the 
 test cases. But you may also take a look my implementation.
 
 Thanks a lot!
 
 On 09/21/2012 04:37 PM, Alan Bateman wrote:
 On 21/09/2012 04:21, Deven You wrote:
 Hi Lance,
 
 I am very busy with other work so I can't work with the 
 SerialBlob/SerialClob item for long time. I am very happy to refine 
 the current test case and create new tests for SerialClob.
 
 I have create a new webre[1] for this task, please review it.
 
 [1]http://cr.openjdk.java.net/~youdwei/OJDK-576/webrev.01/  
 http://cr.openjdk.java.net/%7Eyoudwei/OJDK-576/webrev.01/
 
 PS: If the isFree is not transient, I want to know how we add this 
 field to the javadoc serialized form?
 I don't know very much about the rowset API and I can't see anything 
 to specify whether it is meant to be safe for use by concurrent 
 threads. There are clearly lots of issues here and implementing free 
 introduces a lot more, especially with the possibility of an 
 asynchronous free or more than one thread calling free at around the 
 same time.
 
 Have you considered buf == null to mean that the resources are 
 freed? That might avoid needing to change the serialized form. Also as 
 these types are serializable it means you have to consider the case 
 where you deserialize to buf == null  isFree == false for example. 
 On that point, it looks to me that this code needs a readObject anyway 
 (for several reasons).
 
 A small point is that isFree is a odd name for a method that doesn't 
 return a boolean. If the patch goes ahead then I think it needs a 
 better name, ensureNotFree or requireNotFree or something like that.
 
 -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
 
 
 
 http://oracle.com/us/design/oracle-email-sig-198324.gif
 http://oracle.com/us/design/oracle-email-sig-198324.gifLance Andersen| 
 Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering
 1 Network Drive
 Burlington, MA 01803
 lance.ander...@oracle.com mailto:lance.ander...@oracle.com
 
 
 
 http://oracle.com/us/design/oracle-email-sig-198324.gif
 http://oracle.com/us/design/oracle-email-sig-198324.gifLance Andersen| 
 Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering
 1 Network Drive
 Burlington, MA 01803
 lance.ander...@oracle.com mailto:lance.ander...@oracle.com
 
 


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

Re: RFR: 4247235 (spec str) StringBuffer.insert(int, char[]) specification is inconsistent

[Jim Gish]

Stephen,
Currently here's (a sampling) of how the other methods work. 
Although most check indices first, not all do... (The original bug was 
about this very inconsistency, however one answer given to it was that 
in general we don't say anything about order of exceptions).  However, 
given that most of the methods do index checking first, I think I agree 
with Mike on this one.


Jim

getChars(int srcBegin, int srcEnd, char[]dst, int dstBegin)
- will check for srcBegin, srcEnd first and throw 
StringIndexOutOfBoundsException

- then System.arraycopy checks for null and throws NPE

replace(int start, int end, String str)
- same as above (checking start and end first)

insert(int index, char[] str, int offset, int len)
- same as above (checking index and offset first)

insert(int offset, char[] str)
- same (checking offset first)

String(char value[], int offset, int count)
- same
String(int[] codePoints, int offset, int count)
- same
String(byte ascii[], int hibyte, int offset, int count)
- same
String(byte bytes[], int offset, int length, String charsetName)
- same

*   String(byte bytes[], int offset, int length, Charset charset)
- checks charset == null first!
- then checks offset and length
- and then checks bytes==null

String.getChars(char dst[], int dstBegin)
- checks for dst==null first
- then for bad and throw ArrayIndexOutOfBoundsException


On 01/10/2013 06:47 AM, Stephen Colebourne wrote:

I would encourage null checking to be done first, rather than
sometimes getting StringIndexOutOfBoundsException for a null input.
Reasoning about the JDK methods is much easier that way around.

Stephen


On 9 January 2013 23:09, Mike Duigou mike.dui...@oracle.com wrote:

AbstractStringBuilder probably needs the Unless otherwise noted, blanket 
statement as well (Same as StringBuffer/StringBuilder)

You might want to move the Objects.requireNonNull(dst); in String.getBytes() to 
after the existing checks so as not to unnecessarily change the exception 
thrown for bad inputs. An exception will still be thrown but some bad code may 
anticipate StringIndexOutOfBoundsException rather than NPE. This is a very 
minor matter though.

Otherwise, it looks good.

Mike

On Jan 9 2013, at 14:46 , Jim Gish wrote:


Please review the following:

Webrev: http://cr.openjdk.java.net/~jgish/Bug4247235-Add-Blanket-Null-Stmt/ 
http://cr.openjdk.java.net/%7Ejgish/Bug4247235-Add-Blanket-Null-Stmt/
Here's a specdiff: http://cr.openjdk.java.net/~jgish/Bug4247235-string-specdiff/ 
http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/


Summary:  This change replaces java/lang/*String*.java javadoc, method-specific 
@throws NullPointerException clauses with  blanket null-handling statements 
like that currently in String.java

That was motivated by a discussion awhile back, strongly favoring a blanket 
statement over individual @throws clauses.

For String, the following blanket statement is already in place:

* p Unless otherwise noted, passing a ttnull/tt argument to a constructor
* or method in this class will cause a {@link NullPointerException} to be
* thrown.


However, despite the blanket statement, the following methods also have @throws 
clauses:

public boolean contains(java.lang.CharSequence s)

Throws:
   |java.lang.NullPointerException|- if|s|is|null|
---

public staticString  
http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html
  format(String  
http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html
  format,
java.lang.Object... args)


Throws:
|java.lang.NullPointerException|- If the|format|is|null
|---
||

public staticString  
http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html
  format(java.util.Locale l,
String  
http://cr.openjdk.java.net/%7Ejgish/Bug4247235-string-specdiff/java/lang/String.html
  format,
java.lang.Object... args)

Throws:
|java.lang.NullPointerException|- If the|format|is|null
|--
All of the above are properly specified with the blanket statement or other 
parts of their spec (such as format w.r.t. null Locale) and the @throws can 
safely be removed.

Because the blanket statement is already in effect for String.java, I wrote 
tests for all methods/constructors to ensure compliance.  Running them revealed 
the following:

public void getBytes(int srcBegin, int srcEnd, byte dst[], int dstBegin)
- I would expect an NPE to be thrown if dst == null.  However, this isn't 
always the case.  If dst isn't accessed because 

Re: RFR: 8005582 - java/lang/Runtime/exec/WinCommand.java intermittent test failures

[Jim Gish]

Would you be so kind as to push it, please?

Thanks,
   Jim

On 01/10/2013 12:41 AM, Stuart Marks wrote:

On 1/9/13 10:49 AM, Jim Gish wrote:

Please review:
http://cr.openjdk.java.net/~jgish/Bug8005582-WinCommand-test-failures/
http://cr.openjdk.java.net/%7Ejgish/Bug8005582-WinCommand-test-failures/ 



Summary:  this test, when run on Windows, fails intermittently 
because of
asynchronous windows deletes.  The test passes, deletes two files 
that it has
created for the test in the scratch directory, and exits. Then, jtreg 
upon
attempting to cleanup after the test, tries to delete the files after 
doing a
listFiles() on the scratch directory, which despite the delete by the 
test
itself, still contains the files.  The jtreg delete fails and jtreg 
changes the

state of the test from passed to failed.

I'm in the process of adding deletion retry behavior to jtreg, but in 
the
meantime we think it makes sense to provide a more stable test 
environment by
simply getting rid of the redundant deletes by the test itself. There 
really
is no need for tests to delete files from the scratch directory at 
the end of a

test because jtreg carries a guarantee of cleanup.


By the way, this fix looks fine to me.

s'marks



--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com

Re: RFR: 8005582 - java/lang/Runtime/exec/WinCommand.java intermittent test failures

[Jim Gish]

I have not yet been able to reproduce it, but now that I have a Windows 
7 VM setup, I'm going to try.  Windows sysinternals has a program called 
handle.exe which I have used for years in determining who is holding 
file handles.  If we could install this on our test machines and invoke 
it after a failed test like this, we'd have a better shot at tracking 
this down.


Jim

On 01/10/2013 06:34 AM, Alan Bateman wrote:

On 09/01/2013 19:46, Jim Gish wrote:
It's a Windows feature.  We discovered this recently in debugging 
another test failure.  Windows is documented to do asynchronous 
deletes.  You can't depend on a file.delete() which returns true to 
have actually deleted the file.  It may be the case that another 
process has a file handle which it has not yet released, or it's 
simply a delay.
I don't get this, the issue sounds more like AV software or Windows 
application quality service/agent thing accessing the file but I might 
be wrong of course. Are you able to duplicate this reliably and if so, 
have you looked at it with any tools to see what/who is accessing it 
that is causing the delay?


-Alan.


--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com

RFR 8006007: j.u.c.atomic classes should use intrinsic getAndXXX provided by 7023898

[Chris Hegarty]

Doug, Aleksey,

I updated the appropriate methods in the Atomic classes to use the 
instinsics defined by 7023898 , Unsafe getAndAddInt, getAndSetInt, 
getAndAddLong, getAndSetLong, getAndSetObject.


http://cr.openjdk.java.net/~chegar/8006007/webrev.00/webrev/

This patch also includes some trivial cleanup and consistent style CAS 
loop from Dougs CVS, but excludes the new lambda versions of 
get/updateAndUpdate/Get and get/accumulateAndAccumulate/Get. These new 
methods are not impacted by the intrinsis. I will immediately follow 
these changes with a request to add these new methods, I just don't want 
to tie this update to a spec change request.


Doug,
 If you are agree I will create a patch containing these changes, based 
on the sources in your CVS.


-Chris.

Re: RFR 8006007: j.u.c.atomic classes should use intrinsic getAndXXX provided by 7023898

[Doug Lea]

On 01/10/13 11:40, Chris Hegarty wrote:

Doug, Aleksey,

I updated the appropriate methods in the Atomic classes to use the instinsics
defined by 7023898 , Unsafe getAndAddInt, getAndSetInt, getAndAddLong,
getAndSetLong, getAndSetObject.

http://cr.openjdk.java.net/~chegar/8006007/webrev.00/webrev/

This patch also includes some trivial cleanup and consistent style CAS loop from
Dougs CVS, but excludes the new lambda versions of get/updateAndUpdate/Get and
get/accumulateAndAccumulate/Get. These new methods are not impacted by the
intrinsis. I will immediately follow these changes with a request to add these
new methods, I just don't want to tie this update to a spec change request.



All OK by me. Too bad that you must make two webrevs for this.

-Doug

Re: RFR 8006007: j.u.c.atomic classes should use intrinsic getAndXXX provided by 7023898

[Chris Hegarty]

On 01/10/2013 04:48 PM, Doug Lea wrote:

On 01/10/13 11:40, Chris Hegarty wrote:

Doug, Aleksey,

I updated the appropriate methods in the Atomic classes to use the
instinsics
defined by 7023898 , Unsafe getAndAddInt, getAndSetInt, getAndAddLong,
getAndSetLong, getAndSetObject.

http://cr.openjdk.java.net/~chegar/8006007/webrev.00/webrev/

This patch also includes some trivial cleanup and consistent style CAS
loop from
Dougs CVS, but excludes the new lambda versions of
get/updateAndUpdate/Get and
get/accumulateAndAccumulate/Get. These new methods are not impacted by
the
intrinsis. I will immediately follow these changes with a request to
add these
new methods, I just don't want to tie this update to a spec change
request.



All OK by me. Too bad that you must make two webrevs for this.


That's ok, at least these changes will hit tl later today. I'll prepare 
the request for the new methods now.


-Chris.



-Doug

Re: RFR 8006007: j.u.c.atomic classes should use intrinsic getAndXXX provided by 7023898

[Aleksey Shipilev]

On 01/10/2013 08:40 PM, Chris Hegarty wrote:
 Doug, Aleksey,
 
 I updated the appropriate methods in the Atomic classes to use the
 instinsics defined by 7023898 , Unsafe getAndAddInt, getAndSetInt,
 getAndAddLong, getAndSetLong, getAndSetObject.
 
 http://cr.openjdk.java.net/~chegar/8006007/webrev.00/webrev/

Good. Two comments:
 a) Any java-concurrency-torture [1] failures for these classes?
 b) Can we delegate all the suitable methods to Unsafe directly, without
calling the middleman (i.e. getAndDec() - getAndAdd() - unsafe), as in
[2]?

-Aleksey.

[1] https://github.com/shipilev/java-concurrency-torture/
[2]
https://github.com/shipilev/java-concurrency-torture/blob/master/src/main/java/org/openjdk/util/concurrent/atomic/AtomicIntegerV8.java

Re: RFR 8006007: j.u.c.atomic classes should use intrinsic getAndXXX provided by 7023898

[Chris Hegarty]

On 01/10/2013 05:05 PM, Aleksey Shipilev wrote:

On 01/10/2013 08:40 PM, Chris Hegarty wrote:

Doug, Aleksey,

I updated the appropriate methods in the Atomic classes to use the
instinsics defined by 7023898 , Unsafe getAndAddInt, getAndSetInt,
getAndAddLong, getAndSetLong, getAndSetObject.

http://cr.openjdk.java.net/~chegar/8006007/webrev.00/webrev/


Good. Two comments:
  a) Any java-concurrency-torture [1] failures for these classes?


Can you give me a brief introduction to running these? I have run the 
JDK regression tests and the appropriate JCK tests, all pass.



  b) Can we delegate all the suitable methods to Unsafe directly, without
calling the middleman (i.e. getAndDec() - getAndAdd() - unsafe), as in
[2]?


Yes, we could. The existing implementation was not consistent.

I took the view that this was not performance critical, since some 
existing methods already delegate, and my preference, for simplicity, is 
for the middleman ;-) Do you think there is a perf benefit to changing 
this, or is this a style issue?


-Chris.



-Aleksey.

[1] https://github.com/shipilev/java-concurrency-torture/
[2]
https://github.com/shipilev/java-concurrency-torture/blob/master/src/main/java/org/openjdk/util/concurrent/atomic/AtomicIntegerV8.java

Re: RFR 8005962 : TEST_BUG: java/util/Properties/MacJNUEncoding can fail in certain environments

[Naoto Sato]

Looks good to me.

BTW, I thought that webrev would effectively extract the diffs even when 
files were moved around.


Naoto

On 1/10/13 9:49 AM, Brent Christian wrote:

Hi,

The test case for 8003228 fails in certain environments.  Also the
version that was pushed was missing a couple small changes.

The code changes to fix these issues are here:
http://cr.openjdk.java.net/~bchristi/8005962/webrev.00/

The test case will also be moved from java/util/Properties/ to
java/lang/System/.  Webrev wouldn't do well showing that part of the
change, so it's not reflected there.  Instead I generated a webrev to
better show the code changes.

Thanks,
-Brent

Re: Improving performance and reducing object allocations of java.util.UUID to/from string

[Mike Duigou]

I apologize for the lack of reply. I had missed this thread while on holiday.

As Aleksey suggests the first step is to complete the OCA/CLA.

Until that step is complete we won't be able to act upon this contribution in 
any formal way. I encourage you to update the patch with any specific feedback 
you receive.

Thanks in advance for this contribution!

Mike

On Jan 9 2013, at 09:51 , Steven Schlansker wrote:

 Hello again,
 
 I sent this email a week ago and have received no replies.  Is there any step 
 I have missed necessary to contribute to the JDK libraries?
 
 I am very interested in making your lives easier, so please let me know if I 
 am in the wrong place or are otherwise misguided.
 
 Thanks!
 Steven
 
 
 On Dec 29, 2012, at 9:25 AM, Steven Schlansker stevenschlans...@gmail.com 
 wrote:
 
 Hello core-libs-dev,
 
 My company uses UUIDs throughout our software.  We recently identified that 
 the java.util.UUID implementations of fromString and toString are horridly 
 inefficient.
 
 An incomplete list of the inefficiencies:
 
 * fromString uses a regular expression that is not even precompiled
 * fromString uses a regular expression to parse out the - markers, 
 requiring the allocation of many String and String[] objects, when a simple 
 linear scan works just fine
 * fromString unnecessarily allocates multiple Long objects
 
 * toString creates a char[64], a String object which copies that, and a 
 sub-String object for *each* of the 5 hex digit segments
 * toString produces a fixed-length result but involves multiple 
 StringBuilder concatenations
 
 Everyone that I have shown the relevant code to has been horrified by the 
 complete lack of care towards object allocations.  While I am a big fan of 
 the object allocation is free until otherwise proven philosophy, we traced 
 multiple production problems down to these hotspots.
 
 But instead of just whining about it, I wish to contribute a patch which I 
 believe fixes the problem.  This is my first attempt to submit anything to 
 the JDK, so please be nice :-)
 
 My initial attempt has yielded micro benchmarks with very favorable 
 outcomes.  By taking the appropriate code from java.lang.Long, modifying it 
 to work on a single pre-allocated array+offset rather than returning a 
 String, and scanning for dashes instead of regex splitting I reduced times 
 3-6x and object allocations to the low single digits.
 
 My Google Caliper benchmark is available here, to ensure that I have not 
 committed any benchmark sins:
 https://gist.github.com/4356621
 
   benchmark instances  Bytesns linear runtime
 JdkUuidFromString 51.00 1544.0 608.2 ==
 NewUuidFromString  2.00   72.0 179.1 
 
 JdkUuidToString 31.00 1720.0 321.4 ===
 NewUuidToString  3.00  200.0  51.5 ==
 
 In particular, the reduction of object allocations from ~1.5KB to 72/200 
 bytes dramatically reduces GC pressure when you sit in a tight loop 
 deserializing millions of UUIDs from strings.
 
 I believe the same patch can (and should?) apply to jdk7u and jdk8, as the 
 code does not seem to have changed.
 
 I would appreciate any feedback on the code style, efficiency, or 
 correctness that you can offer.  I have run the java/util/UUID jtreg suite 
 against this and it passes.  We stole the more thorough Apache Harmony tests 
 for this and they pass as well: 
 https://github.com/stevenschlansker/components-ness-core/blob/master/src/test/java/com/nesscomputing/uuid/TestNessUUID.java
 
 I have not yet secured a CLA, but my company is in the process of signing 
 one.
 
 The rest of the message is a hg export of my change set, which is current 
 to the tip of jdk7.
 Happy holidays, and thank you for your time!
 Steven Schlansker
 
 
 
 
 # HG changeset patch
 # User Steven Schlansker ste...@nesscomputing.com
 # Date 1356737383 0
 # Node ID a812c963b96f08112f6805cbc380b12af196f788
 # Parent  9b8c96f96a0f9a5801b55530a387fefbe50482a3
 java.util.UUID: improve performance of UUID#toString and UUID#fromString by 
 avoiding many unnecessary object allocations.
 
   benchmark instances  Bytesns linear runtime
 JdkUuidFromString 51.00 1544.0 608.2 ==
 NewUuidFromString  2.00   72.0 179.1 
 
 JdkUuidToString 31.00 1720.0 321.4 ===
 NewUuidToString  3.00  200.0  51.5 ==
 
 diff -r 9b8c96f96a0f -r a812c963b96f src/share/classes/java/util/UUID.java
 --- a/src/share/classes/java/util/UUID.java  Mon Jun 27 13:21:34 2011 -0700
 +++ b/src/share/classes/java/util/UUID.java  Fri Dec 28 23:29:43 2012 +
 @@ -189,26 +189,79 @@
*  described in {@link #toString}
*
*/
 -public static UUID fromString(String name) {
 -String[] components = name.split(-);
 -if (components.length != 5)
 -throw new IllegalArgumentException(Invalid UUID string: 
 +name);
 -for (int i=0; i5; i++)
 -components[i] = 

Re: RFR: 8005582 - java/lang/Runtime/exec/WinCommand.java intermittent test failures

[Stuart Marks]

On 1/10/13 3:34 AM, Alan Bateman wrote:

On 09/01/2013 19:46, Jim Gish wrote:

It's a Windows feature.  We discovered this recently in debugging another
test failure.  Windows is documented to do asynchronous deletes.  You can't
depend on a file.delete() which returns true to have actually deleted the
file.  It may be the case that another process has a file handle which it has
not yet released, or it's simply a delay.

I don't get this, the issue sounds more like AV software or Windows application
quality service/agent thing accessing the file but I might be wrong of course.
Are you able to duplicate this reliably and if so, have you looked at it with
any tools to see what/who is accessing it that is causing the delay?


Dave DeHaven was able to reproduce this in his diagnosis of the Arrrghs test 
failure.


It's a race condition, because the Windows Application Experience daemon opens 
up files with certain extensions (.bat, .cmd ?). I suspect it gets a filesystem 
notification that a file matching the right pattern has been created, so some 
time later it gets around to opening the file; and then after processing it, it 
closes the file.


That a file remains in the filesystem after having been deleted indeed is 
documented Windows behavior. The doc for the DeleteFile function [1] says,


 The DeleteFile function marks a file for deletion on close. Therefore, the 
file deletion does not occur until the last handle to the file is closed. 
Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED. 


(I'm not a Windows developer, so I may be looking in the wrong place or 
misinterpreting something. Please correct me if I'm wrong.)


If the AE daemon has the file open the moment the test deletes it, the file 
will remain present until the AE daemon has closed it.


This seems built into Windows. I think we have to live with it. Presumably 
these various daemons open the file with CreateFile(FILE_SHARE_DELETE) [2] 
allowing the owner of the file to delete it (eventually). Note this also 
allows renaming of the file. So the rename-before-delete technique seems like 
the most promising approach.


s'marks

[1] 
http://msdn.microsoft.com/en-us/library/windows/desktop/aa363915%28v=vs.85%29.aspx


[2] 
http://msdn.microsoft.com/en-us/library/windows/desktop/aa363858%28v=vs.85%29.aspx

Re: RFR 8005962 : TEST_BUG: java/util/Properties/MacJNUEncoding can fail in certain environments

[Brent Christian]

Thanks, Naoto.

AFAICT, in a case like this there where a file is moved *and* changes 
are made to it, webrev shows that the new file is renamed from the old 
file, but doesn't provide cdiffs/sdiffs/etc for the code changes.


'hg diff' behaves the same way WRT the code changes - it tells you that 
the old file was removed and the new file was added, but you don't get 
code diffs for the changes.


(Interestingly, the NetBeans editor seemed to figure out what was 
happening.)


That's how it works for me, anyway.

-Brent

On 1/10/13 10:27 AM, Naoto Sato wrote:

Looks good to me.

BTW, I thought that webrev would effectively extract the diffs even when
files were moved around.

Naoto

On 1/10/13 9:49 AM, Brent Christian wrote:

Hi,

The test case for 8003228 fails in certain environments.  Also the
version that was pushed was missing a couple small changes.

The code changes to fix these issues are here:
http://cr.openjdk.java.net/~bchristi/8005962/webrev.00/

The test case will also be moved from java/util/Properties/ to
java/lang/System/.  Webrev wouldn't do well showing that part of the
change, so it's not reflected there.  Instead I generated a webrev to
better show the code changes.

Thanks,
-Brent

hg: jdk8/tl/jdk: 8005962: TEST_BUG: java/util/Properties/MacJNUEncoding can fail in certain environments

[naoto . sato]

Changeset: c622df692bfb
Author:bchristi
Date:  2013-01-10 10:21 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c622df692bfb

8005962: TEST_BUG: java/util/Properties/MacJNUEncoding can fail in certain 
environments
Summary: Test script now sets LC_ALL, other small changes, relocate test
Reviewed-by: naoto, alanb

+ test/java/lang/System/MacJNUEncoding/ExpectedEncoding.java
+ test/java/lang/System/MacJNUEncoding/MacJNUEncoding.sh
- test/java/util/Properties/MacJNUEncoding/ExpectedEncoding.java
- test/java/util/Properties/MacJNUEncoding/MacJNUEncoding.sh

Re: RFR 8006007: j.u.c.atomic classes should use intrinsic getAndXXX provided by 7023898

[Aleksey Shipilev]

On 01/10/2013 09:15 PM, Chris Hegarty wrote:
 On 01/10/2013 05:05 PM, Aleksey Shipilev wrote:
 On 01/10/2013 08:40 PM, Chris Hegarty wrote:
 Doug, Aleksey,

 I updated the appropriate methods in the Atomic classes to use the
 instinsics defined by 7023898 , Unsafe getAndAddInt, getAndSetInt,
 getAndAddLong, getAndSetLong, getAndSetObject.

 http://cr.openjdk.java.net/~chegar/8006007/webrev.00/webrev/

 Good. Two comments:
   a) Any java-concurrency-torture [1] failures for these classes?
 
 Can you give me a brief introduction to running these? I have run the
 JDK regression tests and the appropriate JCK tests, all pass.

Build it, run it, see results/index.html. Should be 100% pass rate. If
not, drill down to exact tests.

   b) Can we delegate all the suitable methods to Unsafe directly, without
 calling the middleman (i.e. getAndDec() - getAndAdd() - unsafe), as in
 [2]?
 
 Yes, we could. The existing implementation was not consistent.
 
 I took the view that this was not performance critical, since some
 existing methods already delegate, and my preference, for simplicity, is
 for the middleman ;-) Do you think there is a perf benefit to changing
 this, or is this a style issue?

Yeah, that's mostly stylistic issue. If that's not in Doug's repo, you
can just disregard this. (There is a tempting desire to not to blow up
the call tree to help inliner, since the delegating method is not private).

-Aleksey.

Re: 8005978: shell tests need to use the $COMPILEJDK for javac, jar and other tools

[Chris Hegarty]

Skimming over this it looks fine to me.

You could add TESTTOOLVMOPTS while there ;-)

-Chris.

On 01/10/2013 01:14 PM, Alan Bateman wrote:


I'm currently trying to run the jdk tests on the compact profiles. As
the profile builds are runtimes only, they don't have tools such as
javac and so need to be run with both -jdk and -compilejdk. That works
okay except for the shell tests because many of them launch the javac
or jar tools directly and so fail because the runtime under test
($TESTJAVA) is not a JDK. I'd like to update the shell tests to use the
tools in $COMPILEJAVA, this is the JDK specified to jtreg with
-compilejdk or it's the same as $TESTJAVA when -compilejdk is not
specified.

The webrev with the proposed changes is here:

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

For the most part this is just s/TESTJAVA/COMPILEJAVA/ although there
are a couple of subtle things to watch out for - for example tool tests
should be testing the tools in $TESTJAVA, not $COMPILEJAVA. Many of the
shell tests were created to be runnable outside of jtreg so I've
preserved this where it was previously supported.

I should note that this is not all of the shell tests, I don't have time
to fix the tests in areas that aren't interesting for the profiles so
contributions to do more would be welcome.  Another thing is that a lot
of these shell tests are old tests and a lot of them don't really need
to be shell tests. I mention this because another great effort would be
be gradually replace many of these shell tests.

-Alan.

Re: Review request JDK-8004729: Parameter Reflection API

[Eric McCorkle]

Thanks to all for initial reviews; however, it appears that the version
you saw was somewhat stale.  I've applied your comments (and some
changes that I'd made since the version that was posted).

Please take a second look.

Thanks,
Eric


On 01/10/13 04:19, Peter Levart wrote:
 Hello Eric,
 
 You must have missed my comment from the previous webrev:
 
  292 private Parameter[] privateGetParameters() {
  293 if (null != parameters)
  294 return parameters.get();
 
 If/when the 'parameters' SoftReference is cleared, the method will be
 returning null forever after...
 
 You should also retrieve the referent and check for it's presence before
 returning it:
 
 Parameter[] res;
 if (parameters != null  (res = parameters.get()) != null)
 return res;
 ...
 ...
 
 Regards, Peter
 
 On 01/09/2013 10:55 PM, Eric McCorkle wrote:
 Hello,

 Please review the core reflection API implementation of parameter
 reflection.  This is the final component of method parameter reflection.
   This was posted for review before, then delayed until the check-in for
 JDK-8004728 (hotspot support for parameter reflection), which occurred
 yesterday.

 Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, *not*
 jdk8/tl; therefore, it may be a while before the changeset makes its way
 into jdk8/tl.

 Also note: since the check-in of JDK-8004727 (javac support for
 parameter reflection), there has been a failure in the tests for
 Pack200.  This is being addressed in a fix contributed by Kumar, which I
 believe has also been posted for review.

 The open webrev is here:
 http://cr.openjdk.java.net/~coleenp/JDK-8004729

 The feature request is here:
 http://bugs.sun.com/view_bug.do?bug_id=8004729

 The latest version of the spec can be found here:
 http://cr.openjdk.java.net/~abuckley/8misc.pdf


 Thanks,
 Eric
 

Re: Review request JDK-8004729: Parameter Reflection API

[Peter Levart]

A apologise. I don't know how that happened (browser cache? strange - 
the URL is different)...


Regards, Peter

On 01/10/2013 08:22 PM, Eric McCorkle wrote:

Thanks to all for initial reviews; however, it appears that the version
you saw was somewhat stale.  I've applied your comments (and some
changes that I'd made since the version that was posted).

Please take a second look.

Thanks,
Eric

On 01/10/13 04:19, Peter Levart wrote:

Hello Eric,

You must have missed my comment from the previous webrev:

  292 private Parameter[] privateGetParameters() {
  293 if (null != parameters)
  294 return parameters.get();

If/when the 'parameters' SoftReference is cleared, the method will be
returning null forever after...

You should also retrieve the referent and check for it's presence before
returning it:

Parameter[] res;
if (parameters != null  (res = parameters.get()) != null)
 return res;
...
...

Regards, Peter

On 01/09/2013 10:55 PM, Eric McCorkle wrote:

Hello,

Please review the core reflection API implementation of parameter
reflection.  This is the final component of method parameter reflection.
   This was posted for review before, then delayed until the check-in for
JDK-8004728 (hotspot support for parameter reflection), which occurred
yesterday.

Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, *not*
jdk8/tl; therefore, it may be a while before the changeset makes its way
into jdk8/tl.

Also note: since the check-in of JDK-8004727 (javac support for
parameter reflection), there has been a failure in the tests for
Pack200.  This is being addressed in a fix contributed by Kumar, which I
believe has also been posted for review.

The open webrev is here:
http://cr.openjdk.java.net/~coleenp/JDK-8004729

The feature request is here:
http://bugs.sun.com/view_bug.do?bug_id=8004729

The latest version of the spec can be found here:
http://cr.openjdk.java.net/~abuckley/8misc.pdf


Thanks,
Eric

Review request 8005080: JDBC 4.2

[Lance Andersen - Oracle]

The following webrev has the bulk of  the JDBC 4.2 changes: 
http://cr.openjdk.java.net/~lancea/8005080/webrev.00/

There will be additional updates to java.sql.Date/TIme/Timestamp (by Sherman) 
once JSR 310 is integrated to aide in moving to and from the new date time 
datatypes.  

I will also probably have one more set of minor changes once 310 is integrated.

The CCC has been approved for these changes the specdiff of the javadocs is 
still available at http://cr.openjdk.java.net/~lancea/8005080/specdiffs.02/ 
(note some of the changes highlighted were put back previously)


Best
Lance

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

Re: Review request JDK-8004729: Parameter Reflection API

[Peter Levart]

Hello Eric,

I have another one. Although not very likely, the reference to the same 
Method/Constructor can be shared among multiple threads. The publication 
of a parameters array should therefore be performed via a volatile write 
/ volatile read, otherwise it can happen that some thread sees 
half-initialized array content. The 'parameters' field in Executable 
should be declared as volatile and there should be a single read from it 
and a single write to it in the privateGetParameters() method (you need 
a local variable to hold intermediate states)...


Regards, Peter

On 01/10/2013 09:42 PM, Eric McCorkle wrote:

Thanks to all for initial reviews; however, it appears that the version
you saw was somewhat stale.  I've applied your comments (and some
changes that I'd made since the version that was posted).

Please take a second look.

Thanks,
Eric


On 01/10/13 04:19, Peter Levart wrote:

Hello Eric,

You must have missed my comment from the previous webrev:

  292 private Parameter[] privateGetParameters() {
  293 if (null != parameters)
  294 return parameters.get();

If/when the 'parameters' SoftReference is cleared, the method will be
returning null forever after...

You should also retrieve the referent and check for it's presence before
returning it:

Parameter[] res;
if (parameters != null  (res = parameters.get()) != null)
 return res;
...
...

Regards, Peter

On 01/09/2013 10:55 PM, Eric McCorkle wrote:

Hello,

Please review the core reflection API implementation of parameter
reflection.  This is the final component of method parameter reflection.
   This was posted for review before, then delayed until the check-in for
JDK-8004728 (hotspot support for parameter reflection), which occurred
yesterday.

Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, *not*
jdk8/tl; therefore, it may be a while before the changeset makes its way
into jdk8/tl.

Also note: since the check-in of JDK-8004727 (javac support for
parameter reflection), there has been a failure in the tests for
Pack200.  This is being addressed in a fix contributed by Kumar, which I
believe has also been posted for review.

The open webrev is here:
http://cr.openjdk.java.net/~coleenp/JDK-8004729

The feature request is here:
http://bugs.sun.com/view_bug.do?bug_id=8004729

The latest version of the spec can be found here:
http://cr.openjdk.java.net/~abuckley/8misc.pdf


Thanks,
Eric

Re: RFR 8006007: j.u.c.atomic classes should use intrinsic getAndXXX provided by 7023898

[Chris Hegarty]

On 01/10/2013 07:48 PM, Aleksey Shipilev wrote:

On 01/10/2013 09:15 PM, Chris Hegarty wrote:

On 01/10/2013 05:05 PM, Aleksey Shipilev wrote:

On 01/10/2013 08:40 PM, Chris Hegarty wrote:

Doug, Aleksey,

I updated the appropriate methods in the Atomic classes to use the
instinsics defined by 7023898 , Unsafe getAndAddInt, getAndSetInt,
getAndAddLong, getAndSetLong, getAndSetObject.

http://cr.openjdk.java.net/~chegar/8006007/webrev.00/webrev/


Good. Two comments:
   a) Any java-concurrency-torture [1] failures for these classes?


Can you give me a brief introduction to running these? I have run the
JDK regression tests and the appropriate JCK tests, all pass.


Build it, run it, see results/index.html. Should be 100% pass rate. If
not, drill down to exact tests.


Maven has just finished downloading the dependencies to build this 
project! ;-) All tests pass.


You can probably remove the *.atomic.*V8 source and tests once these 
changes are integrated.





   b) Can we delegate all the suitable methods to Unsafe directly, without
calling the middleman (i.e. getAndDec() - getAndAdd() - unsafe), as in
[2]?


Yes, we could. The existing implementation was not consistent.

I took the view that this was not performance critical, since some
existing methods already delegate, and my preference, for simplicity, is
for the middleman ;-) Do you think there is a perf benefit to changing
this, or is this a style issue?


Yeah, that's mostly stylistic issue. If that's not in Doug's repo, you
can just disregard this. (There is a tempting desire to not to blow up
the call tree to help inliner, since the delegating method is not private).


I'll leave it as is, I find it much less error prone. We can revisit if 
necessary.


Thanks,
-Chris.



-Aleksey.

hg: jdk8/tl/jdk: 8005582: java/lang/Runtime/exec/WinCommand.java intermittent test failures

[stuart . marks]

Changeset: 13ff1089e625
Author:jgish
Date:  2013-01-10 15:09 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/13ff1089e625

8005582: java/lang/Runtime/exec/WinCommand.java intermittent test failures
Summary: Remove file-deletion code at cleanup which conflicts with jtreg cleanup
Reviewed-by: chegar

! test/java/lang/Runtime/exec/WinCommand.java

hg: jdk8/tl/langtools: 8006037: extra space in javac -help for -J and @ options

[jonathan . gibbons]

Changeset: d462da465da6
Author:jjg
Date:  2013-01-10 14:09 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/d462da465da6

8006037: extra space in javac -help for -J and @ options
Reviewed-by: darcy

! src/share/classes/com/sun/tools/javac/main/Option.java
+ test/tools/javac/main/Option_J_At_Test.java

Re: RFR 8006007: j.u.c.atomic classes should use intrinsic getAndXXX provided by 7023898

[Aleksey Shipilev]

On 01/11/2013 01:46 AM, Chris Hegarty wrote:
 Maven has just finished downloading the dependencies to build this
 project! ;-) All tests pass.

Nice. So, you didn't broke most of the things ;)

 You can probably remove the *.atomic.*V8 source and tests once these
 changes are integrated.

Already did.

 I'll leave it as is, I find it much less error prone. We can revisit if
 necessary.

Fine with me.

-Aleksey.

Re: Review request JDK-8004729: Parameter Reflection API

[Eric McCorkle]

Good catch there.  I made the field volatile, and I also did the same
with the cache fields in Parameter.

It is possible with what exists that you could wind up with multiple
copies of identical parameter objects in existence.  It goes something
like this

thread A sees Executable.parameters is null, goes into the VM to get them
thread B sees Executable.parameters is null, goes into the VM to get them
thread A stores to Executable.parameters
thread B stores to Executable.parameters

Since Parameters is immutable (except for its caches, which will always
end up containing the same things), this *should* have no visible
effects, unless someone does == instead of .equals.

This can be avoided by doing a CAS, which is more expensive execution-wise.

My vote is to *not* do a CAS, and accept that (in extremely rare cases,
even as far as concurrency-related anomalies go), you may end up with
duplicates, and document that very well.

Thoughts?

On 01/10/13 16:10, Peter Levart wrote:
 Hello Eric,
 
 I have another one. Although not very likely, the reference to the same
 Method/Constructor can be shared among multiple threads. The publication
 of a parameters array should therefore be performed via a volatile write
 / volatile read, otherwise it can happen that some thread sees
 half-initialized array content. The 'parameters' field in Executable
 should be declared as volatile and there should be a single read from it
 and a single write to it in the privateGetParameters() method (you need
 a local variable to hold intermediate states)...
 
 Regards, Peter
 
 On 01/10/2013 09:42 PM, Eric McCorkle wrote:
 Thanks to all for initial reviews; however, it appears that the version
 you saw was somewhat stale.  I've applied your comments (and some
 changes that I'd made since the version that was posted).

 Please take a second look.

 Thanks,
 Eric


 On 01/10/13 04:19, Peter Levart wrote:
 Hello Eric,

 You must have missed my comment from the previous webrev:

  292 private Parameter[] privateGetParameters() {
  293 if (null != parameters)
  294 return parameters.get();

 If/when the 'parameters' SoftReference is cleared, the method will be
 returning null forever after...

 You should also retrieve the referent and check for it's presence before
 returning it:

 Parameter[] res;
 if (parameters != null  (res = parameters.get()) != null)
 return res;
 ...
 ...

 Regards, Peter

 On 01/09/2013 10:55 PM, Eric McCorkle wrote:
 Hello,

 Please review the core reflection API implementation of parameter
 reflection.  This is the final component of method parameter reflection.
   This was posted for review before, then delayed until the check-in for
 JDK-8004728 (hotspot support for parameter reflection), which occurred
 yesterday.

 Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, *not*
 jdk8/tl; therefore, it may be a while before the changeset makes its way
 into jdk8/tl.

 Also note: since the check-in of JDK-8004727 (javac support for
 parameter reflection), there has been a failure in the tests for
 Pack200.  This is being addressed in a fix contributed by Kumar, which I
 believe has also been posted for review.

 The open webrev is here:
 http://cr.openjdk.java.net/~coleenp/JDK-8004729

 The feature request is here:
 http://bugs.sun.com/view_bug.do?bug_id=8004729

 The latest version of the spec can be found here:
 http://cr.openjdk.java.net/~abuckley/8misc.pdf


 Thanks,
 Eric
 

RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread

[Aleksey Shipilev]

Hi,

Submitting this on behalf of Doug Lea. The webrev is here:
  http://cr.openjdk.java.net/~shade/8005926/webrev.00/

Bottom-line: merge ThreadLocalRandom state into Thread, to optimize many
use cases around j.u.c.* code. The simple performance tests on 2x2 i5,
Linux x86_64, 4 threads, 5 forks, 3x3s warmup, 5x3s measurement:

JDK8 (baseline)
  TLR.nextInt():  6.4 +- 0.1 ns/op
TLR.current().nextInt(): 16.1 +- 0.4 ns/op
 TL.get().nextInt(): 19.1 +- 0.6 ns/op

JDK8 (patched)
  TLR.nextInt():  6.5 +- 0.2 ns/op
TLR.current().nextInt():  6.4 +- 0.1 ns/op
 TL.get().nextInt(): 17.2 +- 2.0 ns/op

First line shows the peak performance of TLR itself, everything over
that is the ThreadLocal overhead. One can see the patched version
bypasses ThreadLocal machinery completely, and the overhead is slim to none.

N.B. It gets especially interesting when there are many ThreadLocals
registered. Making 1M ThreadLocals and pre-touching them bloats the
thread-local maps, and we get:

JDK8 (baseline), contaminators = 1M:
  TLR.nextInt():  6.4 +- 0.1 ns/op
TLR.current().nextInt(): 21.7 +- 5.3 ns/op
 TL.get().nextInt(): 28.7 +- 1.1 ns/op

JDK8 (patched), contaminators = 1M:
  TLR.nextInt():  6.6 +- 0.2 ns/op
TLR.current().nextInt():  6.5 +- 0.1 ns/op
 TL.get().nextInt(): 29.4 +- 0.5 ns/op

Note that patched version successfully dodges this pathological case.

Testing:
  - Doug tested on his platforms
  - Tested Linux x86_64 to build and run successfully
  - JPRT builds are OK
  - JPRT tests are OK (modulo some weird lambda/default-methods test
failures in jdk8/tl)

Attribution:
  - dl: original patch
  - shade: testing, copyright headers, etc.

-Aleksey.

Re: Review request JDK-8004729: Parameter Reflection API

[Eric McCorkle]

The webrev has been refreshed with the solution I describe below
implemented.  Please make additional comments.

On 01/10/13 17:29, Eric McCorkle wrote:
 Good catch there.  I made the field volatile, and I also did the same
 with the cache fields in Parameter.
 
 It is possible with what exists that you could wind up with multiple
 copies of identical parameter objects in existence.  It goes something
 like this
 
 thread A sees Executable.parameters is null, goes into the VM to get them
 thread B sees Executable.parameters is null, goes into the VM to get them
 thread A stores to Executable.parameters
 thread B stores to Executable.parameters
 
 Since Parameters is immutable (except for its caches, which will always
 end up containing the same things), this *should* have no visible
 effects, unless someone does == instead of .equals.
 
 This can be avoided by doing a CAS, which is more expensive execution-wise.
 
 My vote is to *not* do a CAS, and accept that (in extremely rare cases,
 even as far as concurrency-related anomalies go), you may end up with
 duplicates, and document that very well.
 
 Thoughts?
 
 On 01/10/13 16:10, Peter Levart wrote:
 Hello Eric,

 I have another one. Although not very likely, the reference to the same
 Method/Constructor can be shared among multiple threads. The publication
 of a parameters array should therefore be performed via a volatile write
 / volatile read, otherwise it can happen that some thread sees
 half-initialized array content. The 'parameters' field in Executable
 should be declared as volatile and there should be a single read from it
 and a single write to it in the privateGetParameters() method (you need
 a local variable to hold intermediate states)...

 Regards, Peter

 On 01/10/2013 09:42 PM, Eric McCorkle wrote:
 Thanks to all for initial reviews; however, it appears that the version
 you saw was somewhat stale.  I've applied your comments (and some
 changes that I'd made since the version that was posted).

 Please take a second look.

 Thanks,
 Eric


 On 01/10/13 04:19, Peter Levart wrote:
 Hello Eric,

 You must have missed my comment from the previous webrev:

  292 private Parameter[] privateGetParameters() {
  293 if (null != parameters)
  294 return parameters.get();

 If/when the 'parameters' SoftReference is cleared, the method will be
 returning null forever after...

 You should also retrieve the referent and check for it's presence before
 returning it:

 Parameter[] res;
 if (parameters != null  (res = parameters.get()) != null)
 return res;
 ...
 ...

 Regards, Peter

 On 01/09/2013 10:55 PM, Eric McCorkle wrote:
 Hello,

 Please review the core reflection API implementation of parameter
 reflection.  This is the final component of method parameter reflection.
   This was posted for review before, then delayed until the check-in for
 JDK-8004728 (hotspot support for parameter reflection), which occurred
 yesterday.

 Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, *not*
 jdk8/tl; therefore, it may be a while before the changeset makes its way
 into jdk8/tl.

 Also note: since the check-in of JDK-8004727 (javac support for
 parameter reflection), there has been a failure in the tests for
 Pack200.  This is being addressed in a fix contributed by Kumar, which I
 believe has also been posted for review.

 The open webrev is here:
 http://cr.openjdk.java.net/~coleenp/JDK-8004729

 The feature request is here:
 http://bugs.sun.com/view_bug.do?bug_id=8004729

 The latest version of the spec can be found here:
 http://cr.openjdk.java.net/~abuckley/8misc.pdf


 Thanks,
 Eric

Re: RFR (XS): CR 8004330: Add missing Unsafe entry points for addAndGet() family

[Aleksey Shipilev]

On 01/09/2013 05:54 PM, Doug Lea wrote:
 On 01/09/13 08:04, Aleksey Shipilev wrote:
   c) While existing, Java-level AtomicLong.VM_SUPPORT_LONG_CAS is
 redundant, and can be eliminated. AtomicLongFieldUpdater can be
 rewritten to use Unsafe on all the paths.

 
 There is one little nicety here that does rely on 
 VM_SUPPORT_LONG_CAS. There is a disclaimer somewhere that CAS is
 guaranteed to atomic only wrt other CASes. But in the emulation code
 for updaters, we also lock unconditional writes, because not doing so
 would be surprising.

Ok, I spent some time thinking about this, so I can ask the stupid
question now. What's surprising? I can't understand how that blows up if
we do put/getLongVolatile() in ALFU.LockedUpdater, and do get()/set()
without the locking.

 A good case can be made that the fallback wrapper for
 putLongVolatile should itself use a lock in this case but for reasons
 I don't remember, this wasn't done. (and in any case wouldn't trap
 syntactically generated volatile writes.) So there may be some usage
 code that is officially wrong/surprising on these grounds.

Ummm. Examples? I'm OK with exploring the darkest side of Unsafe :)

-Aleksey.

hg: jdk8/tl/langtools: 8006033: bug in Pretty.toSimpleString

[jonathan . gibbons]

Changeset: 7d2f628f04f1
Author:jjg
Date:  2013-01-10 15:48 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/7d2f628f04f1

8006033: bug in Pretty.toSimpleString
Reviewed-by: darcy

! src/share/classes/com/sun/tools/javac/tree/Pretty.java
+ test/tools/javac/tree/PrettySimpleStringTest.java

Re: Review request JDK-8004729: Parameter Reflection API

[Vitaly Davidovich]

Hi Eric,

Parameter.equals() doesn't need null check - instanceof covers that already.

Maybe this has been mentioned already, but personally I'm not a fan of null
checks such as if (null == x) - I prefer the null on the right hand side,
but that's just stylistic.

Perhaps I'm looking at a stale webrev but Executable.privateGetParameters()
reads and writes from/to the volatile field more than once.  I think Peter
already mentioned that it should use one read into a local and one write to
publish the final version to the field (it can return the temp as well).

Thanks

Sent from my phone
On Jan 10, 2013 6:05 PM, Eric McCorkle eric.mccor...@oracle.com wrote:

 The webrev has been refreshed with the solution I describe below
 implemented.  Please make additional comments.

 On 01/10/13 17:29, Eric McCorkle wrote:
  Good catch there.  I made the field volatile, and I also did the same
  with the cache fields in Parameter.
 
  It is possible with what exists that you could wind up with multiple
  copies of identical parameter objects in existence.  It goes something
  like this
 
  thread A sees Executable.parameters is null, goes into the VM to get them
  thread B sees Executable.parameters is null, goes into the VM to get them
  thread A stores to Executable.parameters
  thread B stores to Executable.parameters
 
  Since Parameters is immutable (except for its caches, which will always
  end up containing the same things), this *should* have no visible
  effects, unless someone does == instead of .equals.
 
  This can be avoided by doing a CAS, which is more expensive
 execution-wise.
 
  My vote is to *not* do a CAS, and accept that (in extremely rare cases,
  even as far as concurrency-related anomalies go), you may end up with
  duplicates, and document that very well.
 
  Thoughts?
 
  On 01/10/13 16:10, Peter Levart wrote:
  Hello Eric,
 
  I have another one. Although not very likely, the reference to the same
  Method/Constructor can be shared among multiple threads. The publication
  of a parameters array should therefore be performed via a volatile write
  / volatile read, otherwise it can happen that some thread sees
  half-initialized array content. The 'parameters' field in Executable
  should be declared as volatile and there should be a single read from it
  and a single write to it in the privateGetParameters() method (you need
  a local variable to hold intermediate states)...
 
  Regards, Peter
 
  On 01/10/2013 09:42 PM, Eric McCorkle wrote:
  Thanks to all for initial reviews; however, it appears that the version
  you saw was somewhat stale.  I've applied your comments (and some
  changes that I'd made since the version that was posted).
 
  Please take a second look.
 
  Thanks,
  Eric
 
 
  On 01/10/13 04:19, Peter Levart wrote:
  Hello Eric,
 
  You must have missed my comment from the previous webrev:
 
   292 private Parameter[] privateGetParameters() {
   293 if (null != parameters)
   294 return parameters.get();
 
  If/when the 'parameters' SoftReference is cleared, the method will be
  returning null forever after...
 
  You should also retrieve the referent and check for it's presence
 before
  returning it:
 
  Parameter[] res;
  if (parameters != null  (res = parameters.get()) != null)
  return res;
  ...
  ...
 
  Regards, Peter
 
  On 01/09/2013 10:55 PM, Eric McCorkle wrote:
  Hello,
 
  Please review the core reflection API implementation of parameter
  reflection.  This is the final component of method parameter
 reflection.
This was posted for review before, then delayed until the check-in
 for
  JDK-8004728 (hotspot support for parameter reflection), which
 occurred
  yesterday.
 
  Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, *not*
  jdk8/tl; therefore, it may be a while before the changeset makes its
 way
  into jdk8/tl.
 
  Also note: since the check-in of JDK-8004727 (javac support for
  parameter reflection), there has been a failure in the tests for
  Pack200.  This is being addressed in a fix contributed by Kumar,
 which I
  believe has also been posted for review.
 
  The open webrev is here:
  http://cr.openjdk.java.net/~coleenp/JDK-8004729
 
  The feature request is here:
  http://bugs.sun.com/view_bug.do?bug_id=8004729
 
  The latest version of the spec can be found here:
  http://cr.openjdk.java.net/~abuckley/8misc.pdf
 
 
  Thanks,
  Eric
 

Re: RFR 8005962 : TEST_BUG: java/util/Properties/MacJNUEncoding can fail in certain environments

[Stuart Marks]

FYI, if the file is moved using 'hg mv' then 'hg log -f' will show the history 
across the rename, and 'hg diff --git' will show the rename plus diffs instead 
of all-lines-deleted followed by all-lines-added. Looks like this was done 
using rm + add instead of mv. Too late now, oh well.


s'marks

On 1/10/13 11:13 AM, Brent Christian wrote:

Thanks, Naoto.

AFAICT, in a case like this there where a file is moved *and* changes are made
to it, webrev shows that the new file is renamed from the old file, but doesn't
provide cdiffs/sdiffs/etc for the code changes.

'hg diff' behaves the same way WRT the code changes - it tells you that the old
file was removed and the new file was added, but you don't get code diffs for
the changes.

(Interestingly, the NetBeans editor seemed to figure out what was happening.)

That's how it works for me, anyway.

-Brent

On 1/10/13 10:27 AM, Naoto Sato wrote:

Looks good to me.

BTW, I thought that webrev would effectively extract the diffs even when
files were moved around.

Naoto

On 1/10/13 9:49 AM, Brent Christian wrote:

Hi,

The test case for 8003228 fails in certain environments.  Also the
version that was pushed was missing a couple small changes.

The code changes to fix these issues are here:
http://cr.openjdk.java.net/~bchristi/8005962/webrev.00/

The test case will also be moved from java/util/Properties/ to
java/lang/System/.  Webrev wouldn't do well showing that part of the
change, so it's not reflected there.  Instead I generated a webrev to
better show the code changes.

Thanks,
-Brent

Re: Review request JDK-8004729: Parameter Reflection API

[Eric McCorkle]

On 01/10/13 19:50, Vitaly Davidovich wrote:
 Hi Eric,
 
 Parameter.equals() doesn't need null check - instanceof covers that already.
 

Removed.

 Maybe this has been mentioned already, but personally I'm not a fan of
 null checks such as if (null == x) - I prefer the null on the right
 hand side, but that's just stylistic.

Changed.

 
 Perhaps I'm looking at a stale webrev but
 Executable.privateGetParameters() reads and writes from/to the volatile
 field more than once.  I think Peter already mentioned that it should
 use one read into a local and one write to publish the final version to
 the field (it can return the temp as well).
 

You weren't.  From a pure correctness standpoint, there is nothing wrong
with what is there.  getParameters0 is a constant function, and
parameters is writable only if null.  Hence, we only every see one
nontrivial write to it.

But you are right, it should probably be reduced to a single write, for
performance reasons (to avoid unnecessary memory barriers).  Therefore,
I changed it.

However, I won't be able to refresh the webrev until tomorrow.

 Thanks
 
 Sent from my phone
 
 On Jan 10, 2013 6:05 PM, Eric McCorkle eric.mccor...@oracle.com
 mailto:eric.mccor...@oracle.com wrote:
 
 The webrev has been refreshed with the solution I describe below
 implemented.  Please make additional comments.
 
 On 01/10/13 17:29, Eric McCorkle wrote:
  Good catch there.  I made the field volatile, and I also did the same
  with the cache fields in Parameter.
 
  It is possible with what exists that you could wind up with multiple
  copies of identical parameter objects in existence.  It goes something
  like this
 
  thread A sees Executable.parameters is null, goes into the VM to
 get them
  thread B sees Executable.parameters is null, goes into the VM to
 get them
  thread A stores to Executable.parameters
  thread B stores to Executable.parameters
 
  Since Parameters is immutable (except for its caches, which will
 always
  end up containing the same things), this *should* have no visible
  effects, unless someone does == instead of .equals.
 
  This can be avoided by doing a CAS, which is more expensive
 execution-wise.
 
  My vote is to *not* do a CAS, and accept that (in extremely rare
 cases,
  even as far as concurrency-related anomalies go), you may end up with
  duplicates, and document that very well.
 
  Thoughts?
 
  On 01/10/13 16:10, Peter Levart wrote:
  Hello Eric,
 
  I have another one. Although not very likely, the reference to
 the same
  Method/Constructor can be shared among multiple threads. The
 publication
  of a parameters array should therefore be performed via a
 volatile write
  / volatile read, otherwise it can happen that some thread sees
  half-initialized array content. The 'parameters' field in Executable
  should be declared as volatile and there should be a single read
 from it
  and a single write to it in the privateGetParameters() method
 (you need
  a local variable to hold intermediate states)...
 
  Regards, Peter
 
  On 01/10/2013 09:42 PM, Eric McCorkle wrote:
  Thanks to all for initial reviews; however, it appears that the
 version
  you saw was somewhat stale.  I've applied your comments (and some
  changes that I'd made since the version that was posted).
 
  Please take a second look.
 
  Thanks,
  Eric
 
 
  On 01/10/13 04:19, Peter Levart wrote:
  Hello Eric,
 
  You must have missed my comment from the previous webrev:
 
   292 private Parameter[] privateGetParameters() {
   293 if (null != parameters)
   294 return parameters.get();
 
  If/when the 'parameters' SoftReference is cleared, the method
 will be
  returning null forever after...
 
  You should also retrieve the referent and check for it's
 presence before
  returning it:
 
  Parameter[] res;
  if (parameters != null  (res = parameters.get()) != null)
  return res;
  ...
  ...
 
  Regards, Peter
 
  On 01/09/2013 10:55 PM, Eric McCorkle wrote:
  Hello,
 
  Please review the core reflection API implementation of parameter
  reflection.  This is the final component of method parameter
 reflection.
This was posted for review before, then delayed until the
 check-in for
  JDK-8004728 (hotspot support for parameter reflection), which
 occurred
  yesterday.
 
  Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, *not*
  jdk8/tl; therefore, it may be a while before the changeset
 makes its way
  into jdk8/tl.
 
  Also note: since the check-in of JDK-8004727 (javac support for
  parameter reflection), there has been a 

Re: RFR 8005962 : TEST_BUG: java/util/Properties/MacJNUEncoding can fail in certain environments

[Xueming Shen]

I may not understand the real issue here, but if you list the new and 
old file
names at the same line in the list file, the webrev should generate 
the diff for

you.

-Sherman

On 1/10/13 11:13 AM, Brent Christian wrote:

Thanks, Naoto.

AFAICT, in a case like this there where a file is moved *and* changes 
are made to it, webrev shows that the new file is renamed from the old 
file, but doesn't provide cdiffs/sdiffs/etc for the code changes.


'hg diff' behaves the same way WRT the code changes - it tells you 
that the old file was removed and the new file was added, but you 
don't get code diffs for the changes.


(Interestingly, the NetBeans editor seemed to figure out what was 
happening.)


That's how it works for me, anyway.

-Brent

On 1/10/13 10:27 AM, Naoto Sato wrote:

Looks good to me.

BTW, I thought that webrev would effectively extract the diffs even when
files were moved around.

Naoto

On 1/10/13 9:49 AM, Brent Christian wrote:

Hi,

The test case for 8003228 fails in certain environments.  Also the
version that was pushed was missing a couple small changes.

The code changes to fix these issues are here:
http://cr.openjdk.java.net/~bchristi/8005962/webrev.00/

The test case will also be moved from java/util/Properties/ to
java/lang/System/.  Webrev wouldn't do well showing that part of the
change, so it's not reflected there.  Instead I generated a webrev to
better show the code changes.

Thanks,
-Brent

Re: Review request JDK-8004729: Parameter Reflection API

[Vitaly Davidovich]

Yup, avoiding multiple read/write ops on the volatile field is just for
perf - I saw the null guard there; sorry, should've been clearer.

Thanks

Sent from my phone
On Jan 10, 2013 9:47 PM, Eric McCorkle eric.mccor...@oracle.com wrote:

 On 01/10/13 19:50, Vitaly Davidovich wrote:
  Hi Eric,
 
  Parameter.equals() doesn't need null check - instanceof covers that
 already.
 

 Removed.

  Maybe this has been mentioned already, but personally I'm not a fan of
  null checks such as if (null == x) - I prefer the null on the right
  hand side, but that's just stylistic.

 Changed.

 
  Perhaps I'm looking at a stale webrev but
  Executable.privateGetParameters() reads and writes from/to the volatile
  field more than once.  I think Peter already mentioned that it should
  use one read into a local and one write to publish the final version to
  the field (it can return the temp as well).
 

 You weren't.  From a pure correctness standpoint, there is nothing wrong
 with what is there.  getParameters0 is a constant function, and
 parameters is writable only if null.  Hence, we only every see one
 nontrivial write to it.

 But you are right, it should probably be reduced to a single write, for
 performance reasons (to avoid unnecessary memory barriers).  Therefore,
 I changed it.

 However, I won't be able to refresh the webrev until tomorrow.

  Thanks
 
  Sent from my phone
 
  On Jan 10, 2013 6:05 PM, Eric McCorkle eric.mccor...@oracle.com
  mailto:eric.mccor...@oracle.com wrote:
 
  The webrev has been refreshed with the solution I describe below
  implemented.  Please make additional comments.
 
  On 01/10/13 17:29, Eric McCorkle wrote:
   Good catch there.  I made the field volatile, and I also did the
 same
   with the cache fields in Parameter.
  
   It is possible with what exists that you could wind up with
 multiple
   copies of identical parameter objects in existence.  It goes
 something
   like this
  
   thread A sees Executable.parameters is null, goes into the VM to
  get them
   thread B sees Executable.parameters is null, goes into the VM to
  get them
   thread A stores to Executable.parameters
   thread B stores to Executable.parameters
  
   Since Parameters is immutable (except for its caches, which will
  always
   end up containing the same things), this *should* have no visible
   effects, unless someone does == instead of .equals.
  
   This can be avoided by doing a CAS, which is more expensive
  execution-wise.
  
   My vote is to *not* do a CAS, and accept that (in extremely rare
  cases,
   even as far as concurrency-related anomalies go), you may end up
 with
   duplicates, and document that very well.
  
   Thoughts?
  
   On 01/10/13 16:10, Peter Levart wrote:
   Hello Eric,
  
   I have another one. Although not very likely, the reference to
  the same
   Method/Constructor can be shared among multiple threads. The
  publication
   of a parameters array should therefore be performed via a
  volatile write
   / volatile read, otherwise it can happen that some thread sees
   half-initialized array content. The 'parameters' field in
 Executable
   should be declared as volatile and there should be a single read
  from it
   and a single write to it in the privateGetParameters() method
  (you need
   a local variable to hold intermediate states)...
  
   Regards, Peter
  
   On 01/10/2013 09:42 PM, Eric McCorkle wrote:
   Thanks to all for initial reviews; however, it appears that the
  version
   you saw was somewhat stale.  I've applied your comments (and some
   changes that I'd made since the version that was posted).
  
   Please take a second look.
  
   Thanks,
   Eric
  
  
   On 01/10/13 04:19, Peter Levart wrote:
   Hello Eric,
  
   You must have missed my comment from the previous webrev:
  
292 private Parameter[] privateGetParameters() {
293 if (null != parameters)
294 return parameters.get();
  
   If/when the 'parameters' SoftReference is cleared, the method
  will be
   returning null forever after...
  
   You should also retrieve the referent and check for it's
  presence before
   returning it:
  
   Parameter[] res;
   if (parameters != null  (res = parameters.get()) != null)
   return res;
   ...
   ...
  
   Regards, Peter
  
   On 01/09/2013 10:55 PM, Eric McCorkle wrote:
   Hello,
  
   Please review the core reflection API implementation of
 parameter
   reflection.  This is the final component of method parameter
  reflection.
 This was posted for review before, then delayed until the
  check-in for
   JDK-8004728 (hotspot support 

hg: jdk8/tl/jdk: 2 new changesets

[jonathan . gibbons]

Changeset: c6e8a518c3cd
Author:jjg
Date:  2013-01-10 19:36 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c6e8a518c3cd

8004834: Add doclint support into javadoc
Reviewed-by: erikj, tbell

! make/docs/Makefile

Changeset: c9308137ad9e
Author:jjg
Date:  2013-01-10 19:37 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c9308137ad9e

Merge

- test/java/util/Properties/MacJNUEncoding/ExpectedEncoding.java
- test/java/util/Properties/MacJNUEncoding/MacJNUEncoding.sh

hg: jdk8/tl: 8004834: Add doclint support into javadoc

[jonathan . gibbons]

Changeset: 1129fb75f611
Author:jjg
Date:  2013-01-10 19:36 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/rev/1129fb75f611

8004834: Add doclint support into javadoc
Reviewed-by: erikj, tbell

! common/makefiles/javadoc/Javadoc.gmk

hg: jdk8/tl/jaxp: 8003147: port fix for BCEL bug 39695

[vikram . aroskar]

Changeset: 47738fa4d411
Author:dbuck
Date:  2013-01-10 20:26 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jaxp/rev/47738fa4d411

8003147: port fix for BCEL bug 39695
Summary: Added support for Local Variable Type Table so that BCEL library can 
be used to modify methods with generics-related debug data without violating 
class file format
Reviewed-by: lancea

! src/com/sun/org/apache/bcel/internal/Constants.java
! src/com/sun/org/apache/bcel/internal/classfile/Attribute.java
! src/com/sun/org/apache/bcel/internal/classfile/DescendingVisitor.java
! src/com/sun/org/apache/bcel/internal/classfile/EmptyVisitor.java
+ src/com/sun/org/apache/bcel/internal/classfile/LocalVariableTypeTable.java
! src/com/sun/org/apache/bcel/internal/classfile/Visitor.java
! src/com/sun/org/apache/bcel/internal/generic/MethodGen.java

hg: jdk8/tl/jdk: 8006062: Add @Repeatable to repeating annotations regression tests in JDK repo

[joe . darcy]

Changeset: 86c563dc70ca
Author:darcy
Date:  2013-01-10 21:12 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/86c563dc70ca

8006062: Add @Repeatable to repeating annotations regression tests in JDK repo
Reviewed-by: jjg

! test/java/lang/annotation/repeatingAnnotations/subpackage/Containee.java
! 
test/java/lang/annotation/repeatingAnnotations/subpackage/InheritedContainee.java

Re: Request for review: 6896617: Optimize sun.nio.cs.ISO_8859_1$Encode.encodeArrayLoop() on x86

[Xueming Shen]

Vladimir,

(1) sp++ at Ln#159 probably can be moved into ln#155? the local sp here 
should

 not change the real sp after break+return.
(2) maybe the overflowflag can just be replaced by CoderResult cr, 
with init value
 of CoderResult.UNDERFLOW;,  then cr = CoderResult.OVERFLOW at 
ln#182, and

 simply return cr at ln#193, without another if.
(3) change in encode(char[], int, int, byte[]) does not appear to be 
correct.
 I doubt the slen will get calculated correctly for next round of 
encoding, if len
 is used for the src side length without considering the ret. 
Maybe ln#259 should

 be  slen = Math.min(sl -sp, dst.length - dp);

I'm surprised we only get 1.6% boost.  Maybe it is worth measuring the 
performance
of some small buf/array encoding? I'm a little concerned that the 
overhead may
slow down the small size buf/array encoding. There is a simple benchmark 
for String

en/decoding at test/sun/nio/cs/StrCodingBenchmark.

-Sherman

On 1/8/13 3:18 PM, Vladimir Kozlov wrote:

http://cr.openjdk.java.net/~kvn/6896617_jdk/webrev

Move encoding loop into separate method for which VM will use 
intrinsic on x86. I want to get agreement on this jdk change before I 
submit VM part.


This gives +1.6% performance improvement on SPECjAppServer2004 on x86.
Note, even without intrinsic (on other platforms) JIT will produce 
better code for this loop.


Thanks,
Vladimir

Re: Request for review: 6896617: Optimize sun.nio.cs.ISO_8859_1$Encode.encodeArrayLoop() on x86

[Xueming Shen]

On 1/10/13 6:03 AM, Ulf Zibis wrote:


About overflow:
There was a question some posts above, if it is necessary at all. Can 
one please answer?




Other error results do not have higher priority. But you need to keep
encoding/updating the buffer until you really overflow, since the
overflow detecting in the proposed change is at the very beginning,
yes, something need to be marked, not necessary to be a boolean
overflow though.  Personally I think a direct CoderResult cr may be
better as I suggested in my previous email.

-Sherman

The question is, if other error results have higher priority over 
CoderResult.OVERFLOW.


-Ulf