Re: PPC Linux 64 needs -fsigned-char option for gcc
Hi Volker, Do you have any idea about the modification considering David Holmes' comments ? On Fri, Dec 21, 2012 at 6:48 PM, Volker Simonis volker.simo...@gmail.comwrote: Hi Sean, honestly speaking, I wasn't aware of this problem until now and I just checked that we currently don't use this option, neither internally nor in our port. I found the following nice explanation of the issue: http://www.network-theory.co.uk/docs/gccintro/gccintro_71.html It seems that you only get problems if your programs relies on the fact that 'char' is either unsigned or signed. I suppose that the current OpenJDK doesn't rely on such assumptions (which is good) because we didn't saw any of them until now. If I understand you right, you add some closed code the the JDK which has problems because it makes such assumptions. Is that right? If yes, you should probably first fix that code in the way described in the referenced document. Wouldn't that be possible? Regarding your patch: I suppose you took it against an original JDK and not our port, because in our port we already have the following lines (at least in http://hg.openjdk.java.net/ppc-aix-port/jdk7u//jdk because we haven't started to work on jdk8 until now) CFLAGS_REQUIRED_arm += -fsigned-char -D_LITTLE_ENDIAN CFLAGS_REQUIRED_ppc += -fsigned-char -D_BIG_ENDIAN CFLAGS_REQUIRED_ppc64 += -m64 LDFLAGS_COMMON_ppc64+= -m64 -L/lib64 -Wl,-melf64ppc Notice that we don't set '-D_BIG_ENDIAN' because it is the default. Didn't you observed your problems with jdk7 on Linux/PPC? I think we should patch JDK7 first if this is really necessary. Regards, Volker On Fri, Dec 21, 2012 at 10:40 AM, Sean Chou zho...@linux.vnet.ibm.comwrote: Hello, We found -fsigned-char is added to ppc platform, but not added to ppc64 platform. As they are different platforms, I think it is needed for ppc64 as well. Currently I just added one line modification as follow, but there may be more places to modify. If some one can give some comments, I can make a complete webrev. The buggy scenario we found needs closed code to reproduce, so it is not reproduced with current openjdk build on ppc linux from AIX porting project. I tested with ibmjdk, the patch works. I found CFLAGS_REQUIRED_ppc is from changeset http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/54d8193f177b . Is it enough to add ppc64 option for places ppc appears in that patch? / the patch diff --git a/make/common/Defs-linux.gmk b/make/common/Defs-linux.gmk --- a/make/common/Defs-linux.gmk +++ b/make/common/Defs-linux.gmk @@ -196,6 +196,7 @@ LDFLAGS_COMMON_sparc+= -m32 -mcpu=v9 CFLAGS_REQUIRED_arm += -fsigned-char -D_LITTLE_ENDIAN CFLAGS_REQUIRED_ppc += -fsigned-char -D_BIG_ENDIAN +CFLAGS_REQUIRED_ppc64 += -fsigned-char -D_BIG_ENDIAN ifeq ($(ZERO_BUILD), true) CFLAGS_REQUIRED = $(ZERO_ARCHFLAG) ifeq ($(ZERO_ENDIANNESS), little) -- Best Regards, Sean Chou -- Best Regards, Sean Chou
Re: Review request JDK-8004729: Parameter Reflection API
On 01/10/2013 11:29 PM, 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?
We can not guarantee the singularity of a Parameter instance (like for
example Class instance) because of various reasons, among them:
- the Method/Constructor instances that are available via public API are
allways copied and caching of Parameter instances is performed on each
individual copied Method/Constructor instance. I already had a patch
which moved caching of annotations, for example, to the root instance,
but it has been postponed because of big anticipated annotations changes
comming in. Maybe we should revisit the same also for Parameter
instances when time comes...
- even if caching of Parameters/annotations is performed on root
instances, the root instances can change over time because they are
cached in j.l.Class via a SoftReference which can be cleared and new
root instances can be created for the same methods/constructors.
So as currently stands, the effort to do CAS for Parameters is useless.
Regards, Peter
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
hg: jdk8/tl/jdk: 8005566: (fs) test/java/nio/file/Files/Misc.java failing (sol)
Changeset: 0ca2e39a110d Author:alanb Date: 2013-01-11 12:27 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0ca2e39a110d 8005566: (fs) test/java/nio/file/Files/Misc.java failing (sol) Reviewed-by: chegar ! src/solaris/classes/sun/nio/fs/SolarisAclFileAttributeView.java ! test/java/nio/file/Files/Misc.java
Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)
On 1/9/13 9:30 PM, Mandy Chung wrote:
L152: would it be better to replace the base service class name with
the classname (i.e. javax.xml.XMLEventFactory)
152* If {@code factoryId} is the base service class name,
153* use the service-provider loading facilities, defined by the
154* {@link java.util.ServiceLoader} class, to attempt to locate
and load an
155* implementation of the service.
Hi Mandy,
This is done. I have updated the webrev:
http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.05/
best regards,
-- daniel
RFR: javax.xml.xpath: Using ServiceLoader to load JAXP XPath factories (7169894: JAXP Plugability Layer: using service loader)
Hi, Here is a new webrev in the series that addresses using ServiceLoader in JAXP for JDK 8. 7169894: JAXP Plugability Layer: using service loader This changeset addresses modification in the javax.xml.xpath package. This changes are very similar to the changes proposed for the javax.xml.validation package, except that here we didn't need to add a new Error class. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.xpath/webrev.00/ best regards, -- daniel previous unreviewed webrevs in the series: [5] javax.xml.validation: http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.validation/webrev.00/ previous reviewed webrevs: [1] javax.xml.parsers: http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.06/ [2] javax.xml.datatype: http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.datatype/webrev.02/ [3] javax.xml.stream http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.05/ [4] javax.xml.transform http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.transform/webrev.01/
Re: 8005978: shell tests need to use the $COMPILEJDK for javac, jar and other tools
On 10/01/2013 20:36, Chris Hegarty wrote:
Skimming over this it looks fine to me.
You could add TESTTOOLVMOPTS while there ;-)
-Chris.
We probably should have added ${TESTTOOLVMOPTS} as part of Mark
Sheppard's work in 8003890.
Anyway as I'm adding ${TESTJAVACOPTS} to the javac usages then it's easy
to add ${TESTTOOLVMOPTS} too, I've also added to the other tools used
by the tests that I'm changing. There are a couple of cases where it's
not possible to do this, for example there are number of older security
tests that compile with -source and -target 1.4 and these will conflict
if I run jtreg with something like -javacoptions:-profile compac1.
I've created a bug for this as it's not clear why these tests want to
target 1.4. Another one is the security tests that use NSS where VM
options such as -d64 will not work because the tests are choosing the
architecture of the shared library to use.
I've also added a few ${TESTVMOPTS} to tests that were skipped by
8003890. Interestingly, adding this to the Formater/Basic.sh test
exposes a bug in Formatter when running the tests with -esa. Previously
the VM options were being dropped on the floor so this was not noticed.
I've created a bug for this too and added the test to the
ProblemList.txt until this gets fixed.
The webrev is updated:
http://cr.openjdk.java.net/~alanb/8005978/webrev/
I'd like to this out of the way, and I will of course run the tests on
all platforms before pushing it.
-Alan.
Re: 8005978: shell tests need to use the $COMPILEJDK for javac, jar and other tools
Wow, you've actually done it, I was only joking! The updated webrev (
after a quick skim ) looks great.
-Chris.
On 11/01/2013 14:37, Alan Bateman wrote:
On 10/01/2013 20:36, Chris Hegarty wrote:
Skimming over this it looks fine to me.
You could add TESTTOOLVMOPTS while there ;-)
-Chris.
We probably should have added ${TESTTOOLVMOPTS} as part of Mark
Sheppard's work in 8003890.
Anyway as I'm adding ${TESTJAVACOPTS} to the javac usages then it's easy
to add ${TESTTOOLVMOPTS} too, I've also added to the other tools used
by the tests that I'm changing. There are a couple of cases where it's
not possible to do this, for example there are number of older security
tests that compile with -source and -target 1.4 and these will conflict
if I run jtreg with something like -javacoptions:-profile compac1.
I've created a bug for this as it's not clear why these tests want to
target 1.4. Another one is the security tests that use NSS where VM
options such as -d64 will not work because the tests are choosing the
architecture of the shared library to use.
I've also added a few ${TESTVMOPTS} to tests that were skipped by
8003890. Interestingly, adding this to the Formater/Basic.sh test
exposes a bug in Formatter when running the tests with -esa. Previously
the VM options were being dropped on the floor so this was not noticed.
I've created a bug for this too and added the test to the
ProblemList.txt until this gets fixed.
The webrev is updated:
http://cr.openjdk.java.net/~alanb/8005978/webrev/
I'd like to this out of the way, and I will of course run the tests on
all platforms before pushing it.
-Alan.
8001666: Add updateAndGet, getAndUpdate, getAndAccumulate, accumulateAndGet methods to AtomicXXX
This proposal is to add lambda-compatible atomics and accumulators to the ActomicXXX classes. Webrev: http://cr.openjdk.java.net/~chegar/8001666/ver.00/webrev/ SpecDiff: http://cr.openjdk.java.net/~chegar/8001666/ver.00/specdiff/java/util/concurrent/atomic/package-summary.html This changes come from Doug, with the exception of the AtomicXXXArray classes. Doug, You have not added the equivalent atomics and accumulators the Array classes. Was this intentional? I no reason why this shouldn't be done, so went ahead and did it. -Chris.
Re: 8001666: Add updateAndGet, getAndUpdate, getAndAccumulate, accumulateAndGet methods to AtomicXXX
On 01/11/13 10:25, Chris Hegarty wrote: This proposal is to add lambda-compatible atomics and accumulators to the ActomicXXX classes. Webrev: http://cr.openjdk.java.net/~chegar/8001666/ver.00/webrev/ SpecDiff: Doug, You have not added the equivalent atomics and accumulators the Array classes. Was this intentional? I no reason why this shouldn't be done, so went ahead and did it. I vaguely recall doing these but they must not have gotten committed :-) Thanks for make sure they are all complete. (Life will be easier when lambda, hotspot, and tl start converging...) -Doug
Re: 8001666: Add updateAndGet, getAndUpdate, getAndAccumulate, accumulateAndGet methods to AtomicXXX
On 11/01/2013 15:35, Doug Lea wrote: On 01/11/13 10:25, Chris Hegarty wrote: This proposal is to add lambda-compatible atomics and accumulators to the ActomicXXX classes. Webrev: http://cr.openjdk.java.net/~chegar/8001666/ver.00/webrev/ SpecDiff: Doug, You have not added the equivalent atomics and accumulators the Array classes. Was this intentional? I no reason why this shouldn't be done, so went ahead and did it. I vaguely recall doing these but they must not have gotten committed :-) Thanks for make sure they are all complete. (Life will be easier when lambda, hotspot, and tl start converging...) Nearly there, with the Atomics anyway. Once these changes get in, for once jdk8/tl will be ahead of the others (first time ever?) I'll create and send you a patch based on your CVS, to make it easier for you to update to the new intrinsics and these Array changes. -Chris. -Doug
Re: Review request JDK-8004729: Parameter Reflection API
The webrev has been updated again.
The multiple writes to parameters have been removed, and the tests have
been expanded to look at inner classes, and to test modifiers.
Please look over it again.
Test-wise, I've got a clean run on JPRT (there were some failures in
lambda stuff, but I've been seeing that for some time now).
On 01/10/13 21:47, Eric McCorkle 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
Re: RFR 8005311: Add Scalable Updatable Variables, DoubleAccumulator, DoubleAdder, LongAccumulator, LongAdder
Now with explicit disclaimer on DoubleA* The order of accumulation within or across threads is not guaranteed. Thus, this class may not be applicable if numerical stability is required when combining values of substantially different orders of magnitude. Updated spec: http://cr.openjdk.java.net/~chegar/8005311/ver.02/specdiff/java/util/concurrent/atomic/package-summary.html Unless there are any objections, I'll finalize this spec and seek approval for its integration. -Chris. On 07/01/2013 19:40, Doug Lea wrote: On 01/07/13 14:07, Joe Darcy wrote: Hello, I had a question about how the double accumulation logic was intended to be used. I've taken a quick look at the code and it uses straightforward sum = sum + nextValue code to compute the double sum. Summing doubles values with code numerical accuracy is surprisingly tricky and if the DoubleAccumulator code is meant for wide use, I'd recommend using instead some form of compensated summation: http://en.wikipedia.org/wiki/Kahan_summation_algorithm I'm sympathetic... Complete lack of control over arithmetic issues (here and for plain atomics) led me to resist offering Double versions for years. But many people are content with the scalability vs numerical stability tradeoffs intrinsic here (and I think unsolvable). I suppose it could stand an explicit disclaimer rather than the implicit one there now. How about: The order of accumulation of sums across threads is uncontrolled. Numerical stability of results is not guaranteed when values of substantially different orders of magnitude are combined Or something better? -Doug
Re: Review request JDK-8004729: Parameter Reflection API
On 01/11/2013 04:54 PM, Eric McCorkle wrote:
The webrev has been updated again.
The multiple writes to parameters have been removed, and the tests have
been expanded to look at inner classes, and to test modifiers.
Please look over it again.
Hello Eric,
You still have 2 reads of volatile even in fast path. I would do it this
way:
private Parameter[] privateGetParameters() {
Parameter[] tmp = parameters; // one and only read
if (tmp != null)
return tmp;
// Otherwise, go to the JVM to get them
tmp = getParameters0();
// If we get back nothing, then synthesize parameters
if (tmp == null) {
final int num = getParameterCount();
tmp = new Parameter[num];
for (int i = 0; i num; i++)
// TODO: is there a way to synthetically derive the
// modifiers? Probably not in the general case, since
// we'd have no way of knowing about them, but there
// may be specific cases.
tmp[i] = new Parameter(arg + i, 0, this, i);
// This avoids possible races from seeing a
// half-initialized parameters cache.
}
parameters = tmp;
return tmp;
}
Regards, Peter
Test-wise, I've got a clean run on JPRT (there were some failures in
lambda stuff, but I've been seeing that for some time now).
On 01/10/13 21:47, Eric McCorkle 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:
Re: RFR 8005311: Add Scalable Updatable Variables, DoubleAccumulator, DoubleAdder, LongAccumulator, LongAdder
On 01/11/2013 05:18 PM, Chris Hegarty wrote: Now with explicit disclaimer on DoubleA* The order of accumulation within or across threads is not guaranteed. Thus, this class may not be applicable if numerical stability is required when combining values of substantially different orders of magnitude. It doesn't have to be substantially different order of magnitude. For example: double a = 1d/3d; double b = 1d; double x = a + a + a + b; double y = b + a + a + a; System.out.println(x == y); ... prints false. Regards, Peter Updated spec: http://cr.openjdk.java.net/~chegar/8005311/ver.02/specdiff/java/util/concurrent/atomic/package-summary.html Unless there are any objections, I'll finalize this spec and seek approval for its integration. -Chris. On 07/01/2013 19:40, Doug Lea wrote: On 01/07/13 14:07, Joe Darcy wrote: Hello, I had a question about how the double accumulation logic was intended to be used. I've taken a quick look at the code and it uses straightforward sum = sum + nextValue code to compute the double sum. Summing doubles values with code numerical accuracy is surprisingly tricky and if the DoubleAccumulator code is meant for wide use, I'd recommend using instead some form of compensated summation: http://en.wikipedia.org/wiki/Kahan_summation_algorithm I'm sympathetic... Complete lack of control over arithmetic issues (here and for plain atomics) led me to resist offering Double versions for years. But many people are content with the scalability vs numerical stability tradeoffs intrinsic here (and I think unsolvable). I suppose it could stand an explicit disclaimer rather than the implicit one there now. How about: The order of accumulation of sums across threads is uncontrolled. Numerical stability of results is not guaranteed when values of substantially different orders of magnitude are combined Or something better? -Doug
Re: Review request JDK-8004729: Parameter Reflection API
Yes that's exactly what I'm looking for as well.
Sent from my phone
On Jan 11, 2013 11:25 AM, Peter Levart peter.lev...@gmail.com wrote:
On 01/11/2013 04:54 PM, Eric McCorkle wrote:
The webrev has been updated again.
The multiple writes to parameters have been removed, and the tests have
been expanded to look at inner classes, and to test modifiers.
Please look over it again.
Hello Eric,
You still have 2 reads of volatile even in fast path. I would do it this
way:
private Parameter[] privateGetParameters() {
Parameter[] tmp = parameters; // one and only read
if (tmp != null)
return tmp;
// Otherwise, go to the JVM to get them
tmp = getParameters0();
// If we get back nothing, then synthesize parameters
if (tmp == null) {
final int num = getParameterCount();
tmp = new Parameter[num];
for (int i = 0; i num; i++)
// TODO: is there a way to synthetically derive the
// modifiers? Probably not in the general case, since
// we'd have no way of knowing about them, but there
// may be specific cases.
tmp[i] = new Parameter(arg + i, 0, this, i);
// This avoids possible races from seeing a
// half-initialized parameters cache.
}
parameters = tmp;
return tmp;
}
Regards, Peter
Test-wise, I've got a clean run on JPRT (there were some failures in
lambda stuff, but I've been seeing that for some time now).
On 01/10/13 21:47, Eric McCorkle 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.mccorkle@oracle.**com 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
Re: RFR 8005311: Add Scalable Updatable Variables, DoubleAccumulator, DoubleAdder, LongAccumulator, LongAdder
On 01/11/13 11:35, Peter Levart wrote: On 01/11/2013 05:18 PM, Chris Hegarty wrote: Now with explicit disclaimer on DoubleA* The order of accumulation within or across threads is not guaranteed. Thus, this class may not be applicable if numerical stability is required when combining values of substantially different orders of magnitude. It doesn't have to be substantially different order of magnitude. Thanks. Chris, please add especially: The order of accumulation within or across threads is not guaranteed. Thus, this class may not be applicable if numerical stability is required, especially when combining values of substantially different orders of magnitude. -Doug
Re: RFR: javax.xml.validation: Using ServiceLoader to load JAXP schema factories (7169894: JAXP Plugability Layer: using service loader)
On 09/01/2013 14:28, Daniel Fuchs wrote: Hi, Here is a new webrev in the series that addresses using ServiceLoader in JAXP for JDK 8. 7169894: JAXP Plugability Layer: using service loader This changeset addresses modification in the javax.xml.validation package. It is a bit more complex than the changes required for the other packages because the newInstance methods takes an additional schemaLanguage parameter, and therefore we need to loop over the providers in order to find one that supports the language. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.validation/webrev.00/ Also this particular package did not have any specific configuration error to throw in case of misconfiguration (the xpath package in which the logic is very similar had one for instance), so we're adding a new SchemaFactoryConfigurationError for that purpose. I've taken an initial look at this and I'm wondering about SchemeFactory.newInstance throwing SchemaFactoryConfigurationError. Technically this is an incompatible change but in practical terms it may not be concern as this provider interface may not be used very much. Joe Wang - have you come across SchemaFactory implementations, I'm trying to get a feel for how much this is used, if ever. -Alan
Re: Review request JDK-8004729: Parameter Reflection API
Update should be visible now.
On 01/11/13 11:54, Vitaly Davidovich wrote:
Yes that's exactly what I'm looking for as well.
Sent from my phone
On Jan 11, 2013 11:25 AM, Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com wrote:
On 01/11/2013 04:54 PM, Eric McCorkle wrote:
The webrev has been updated again.
The multiple writes to parameters have been removed, and the
tests have
been expanded to look at inner classes, and to test modifiers.
Please look over it again.
Hello Eric,
You still have 2 reads of volatile even in fast path. I would do it
this way:
private Parameter[] privateGetParameters() {
Parameter[] tmp = parameters; // one and only read
if (tmp != null)
return tmp;
// Otherwise, go to the JVM to get them
tmp = getParameters0();
// If we get back nothing, then synthesize parameters
if (tmp == null) {
final int num = getParameterCount();
tmp = new Parameter[num];
for (int i = 0; i num; i++)
// TODO: is there a way to synthetically derive the
// modifiers? Probably not in the general case, since
// we'd have no way of knowing about them, but there
// may be specific cases.
tmp[i] = new Parameter(arg + i, 0, this, i);
// This avoids possible races from seeing a
// half-initialized parameters cache.
}
parameters = tmp;
return tmp;
}
Regards, Peter
Test-wise, I've got a clean run on JPRT (there were some failures in
lambda stuff, but I've been seeing that for some time now).
On 01/10/13 21:47, Eric McCorkle 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
mailto:eric.mccorkle@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
Re: RFR 8005311: Add Scalable Updatable Variables, DoubleAccumulator, DoubleAdder, LongAccumulator, LongAdder
On 11/01/2013 16:57, Doug Lea wrote: On 01/11/13 11:35, Peter Levart wrote: On 01/11/2013 05:18 PM, Chris Hegarty wrote: Now with explicit disclaimer on DoubleA* The order of accumulation within or across threads is not guaranteed. Thus, this class may not be applicable if numerical stability is required when combining values of substantially different orders of magnitude. It doesn't have to be substantially different order of magnitude. Thanks. Chris, please add especially: Thanks Doug, done. -Chris. The order of accumulation within or across threads is not guaranteed. Thus, this class may not be applicable if numerical stability is required, especially when combining values of substantially different orders of magnitude. -Doug
Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)
On 1/11/2013 5:59 AM, Daniel Fuchs wrote:
Hi Mandy,
This is done. I have updated the webrev:
http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.05/
typo in:
242* If {@code factoryId} is javax.xml.stream.XMLIntputFactory,
Otherwise, looks good. Thanks for the update.
Mandy
Re: RFR 8005962 : TEST_BUG: java/util/Properties/MacJNUEncoding can fail in certain environments
Thanks, Stuart. I did an 'hg mv' of the directory containing the (two) test files. But I then had to 'hg export' to send the changeset for pushing. I'm guessing that the export wasn't able to preserve the history across the rename. -Brent On 1/10/13 6:25 PM, Stuart Marks wrote: 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: RFR 8005962 : TEST_BUG: java/util/Properties/MacJNUEncoding can fail in certain environments
On 11/01/2013 17:50, Brent Christian wrote: Thanks, Stuart. I did an 'hg mv' of the directory containing the (two) test files. But I then had to 'hg export' to send the changeset for pushing. I'm guessing Yes, I noticed this before. 'hg export' and 'hg mv' are not friends. -Chris. that the export wasn't able to preserve the history across the rename. -Brent On 1/10/13 6:25 PM, Stuart Marks wrote: 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: RFR 8005962 : TEST_BUG: java/util/Properties/MacJNUEncoding can fail in certain environments
I've been using 'webrev -N' and leaving it to webrev to determine which files to include based on the hg status. I'll try giving webrev a filelist the next time I encounter this situation. Thanks, -Brent On 1/10/13 6:50 PM, Xueming Shen wrote: 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: RFR 8005962 : TEST_BUG: java/util/Properties/MacJNUEncoding can fail in certain environments
On 1/11/13 9:54 AM, Chris Hegarty wrote: On 11/01/2013 17:50, Brent Christian wrote: I did an 'hg mv' of the directory containing the (two) test files. But I then had to 'hg export' to send the changeset for pushing. I'm guessing Yes, I noticed this before. 'hg export' and 'hg mv' are not friends. -Chris. I see that hg also has a 'bundle' and 'unbundle'. Has anyone used it? Would that have been better in this situation? -Brent On 1/10/13 6:25 PM, Stuart Marks wrote: 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: Request for review: 6896617: Optimize sun.nio.cs.ISO_8859_1$Encode.encodeArrayLoop() on x86
Hi Sherman,
Am 11.01.2013 06:47, schrieb Xueming Shen:
(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.
I've enhanced your suggestions, see more below...
Additionally, some part of encodeArrayLoop(...) maybe could be moved into a separate method, to be
reused by encode(char[] src, int sp, int len, byte[] dst).
Some of the re-engineering could be adapted to the Decoder methods.
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.
I think we should balance 4 cases in rating the performance:
a) few loops, small buf/array
b) few loops, big buf/array
c) many loops, small buf/array
d) many loops, big buf/array
In a), b) the loop surrounding code will not be JIT-compiled, so should be
optimized for interpreter.
In c) d) the loop surrounding code *may be* JIT-compiled and consequtively inline the inner loop,
should be verified.
In b), d) the small inner loop, as demonstrated below, will be JIT-compiled, regardless if moved
into separate method.
-Ulf
1) Check for (sp = sl) is superfluous.
==
private static int copyISOs(
char[] sa, int sp, byte[] da, int dp, int len) {
int i = 0;
for (; i len; i++) {
char c = sa[sp++];
// if (c '\uFF00' != 0) // needs bug 6935994 to be fixed, would be
fastest
if ((byte)(c 8) != 0) // temporary replacement, fast byte-length
operation on x86
break;
da[dp++] = (byte)c;
}
return i;
}
private CoderResult encodeArrayLoop(
CharBuffer src, ByteBuffer dst) {
char[] sa = src.array();
int soff = src.arrayOffset();
int sp = soff + src.position();
int sr = src.remaining();
int sl = sp + sr;
byte[] da = dst.array();
int doff = dst.arrayOffset();
int dp = doff + dst.position();
int dr = dst.remaining();
CoderResult cr;
if (dr sr) {
sr = dr;
cr = CoderResult.OVERFLOW;
} else
cr = CoderResult.UNDERFLOW;
try {
int ret = copyISOs(sa, sp, da, dp, sr);
sp = sp + ret;
dp = dp + ret;
if (ret != sr) {
if (sgp.parse(sa[sp], sa, sp, sl) 0)
return sgp.error();
return sgp.unmappableResult();
}
return cr;
} finally {
src.position(sp - soff);
dst.position(dp - doff);
}
}
2) Avoids sp, dp to be recalculated; shorter surrounding code - best chance to
become JIT-compiled.
==
// while inlinig, JIT will erase the surrounding int[] p
private static boolean copyISOs(
char[] sa, byte[] da, final int[] p, int sl) {
for (int sp = p[0], dp = p[1]; sp sl; sp++) {
char c = sa[sp];
// if (c '\uFF00' != 0) // needs bug 6935994 to be fixed, would be
fastest
if ((byte)(c 8) != 0) // temporary replacement, fast byte-length
operation on x86
return false;
da[dp++] = (byte)c;
}
return true;
}
private CoderResult encodeArrayLoop(
CharBuffer src, ByteBuffer dst) {
char[] sa = src.array();
int soff = src.arrayOffset();
int sp = soff + src.position();
int sr = src.remaining();
byte[] da = dst.array();
int doff = dst.arrayOffset();
int dp = doff + dst.position();
int dr = dst.remaining();
CoderResult cr;
if (dr sr) {
sr = dr;
cr = CoderResult.OVERFLOW;
} else
cr = CoderResult.UNDERFLOW;
try {
int sl = sp + sr;
final int[] p = { sp, dp };
if (!copyISOs(sa, da, p, sl)) {
if (sgp.parse(sa[sp], sa, sp, sl) 0)
return sgp.error();
return sgp.unmappableResult();
}
return cr;
} finally {
src.position(sp - soff);
dst.position(dp - doff);
}
}
3) No more needs try...finally block.
==
private CoderResult encodeArrayLoop(
CharBuffer src, ByteBuffer dst) {
char[] sa = src.array();
int soff = src.arrayOffset();
int sp = soff + src.position();
int sr = src.remaining();
byte[] da = dst.array();
int doff = dst.arrayOffset();
int dp = doff + dst.position();
int dr = dst.remaining();
CoderResult cr;
if (dr sr) {
sr = dr;
cr = CoderResult.OVERFLOW;
} else
cr = CoderResult.UNDERFLOW;
int sl = sp + sr;
for (; sp sl; sp++) {
char c = sa[sp];
// if (c '\uFF00' != 0) { // needs bug
Re: Review request JDK-8004729: Parameter Reflection API
Hi Eric,
Taking another look at the code, some extra logic / checking is needed
in cases where the number of source parameters (non-synthetic and
non-synthesized) disagrees with the number of actual parameters at a
class file level.
For example, if the single source parameter of an inner class
constructor is annotated, the annotation should be associated with the
*second* parameter since the first class file parameter is a synthesized
constructor added by the compiler. I think generally annotations should
not be associated with synthesized or synthetic parameters.
-Joe
On 1/11/2013 9:03 AM, Eric McCorkle wrote:
Update should be visible now.
On 01/11/13 11:54, Vitaly Davidovich wrote:
Yes that's exactly what I'm looking for as well.
Sent from my phone
On Jan 11, 2013 11:25 AM, Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com wrote:
On 01/11/2013 04:54 PM, Eric McCorkle wrote:
The webrev has been updated again.
The multiple writes to parameters have been removed, and the
tests have
been expanded to look at inner classes, and to test modifiers.
Please look over it again.
Hello Eric,
You still have 2 reads of volatile even in fast path. I would do it
this way:
private Parameter[] privateGetParameters() {
Parameter[] tmp = parameters; // one and only read
if (tmp != null)
return tmp;
// Otherwise, go to the JVM to get them
tmp = getParameters0();
// If we get back nothing, then synthesize parameters
if (tmp == null) {
final int num = getParameterCount();
tmp = new Parameter[num];
for (int i = 0; i num; i++)
// TODO: is there a way to synthetically derive the
// modifiers? Probably not in the general case, since
// we'd have no way of knowing about them, but there
// may be specific cases.
tmp[i] = new Parameter(arg + i, 0, this, i);
// This avoids possible races from seeing a
// half-initialized parameters cache.
}
parameters = tmp;
return tmp;
}
Regards, Peter
Test-wise, I've got a clean run on JPRT (there were some failures in
lambda stuff, but I've been seeing that for some time now).
On 01/10/13 21:47, Eric McCorkle 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
mailto:eric.mccorkle@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
Re: RFR: 8005582 - java/lang/Runtime/exec/WinCommand.java intermittent test failures
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. That was a fun one to track down... 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.) No, you have it :) The call to DeleteFile does not actually *delete* the file, it simply marks it for deletion at some point in the future. 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. I concur. -DrD-
Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)
Thanks Mandy! Good catch!
I've updated the webrev:
http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.06/
-- daniel
On 1/11/13 6:23 PM, Mandy Chung wrote:
On 1/11/2013 5:59 AM, Daniel Fuchs wrote:
Hi Mandy,
This is done. I have updated the webrev:
http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.05/
typo in:
242* If {@code factoryId} is javax.xml.stream.XMLIntputFactory,
Otherwise, looks good. Thanks for the update.
Mandy
Re: RFR: javax.xml.validation: Using ServiceLoader to load JAXP schema factories (7169894: JAXP Plugability Layer: using service loader)
On 1/11/2013 8:58 AM, Alan Bateman wrote: On 09/01/2013 14:28, Daniel Fuchs wrote: Hi, Here is a new webrev in the series that addresses using ServiceLoader in JAXP for JDK 8. 7169894: JAXP Plugability Layer: using service loader This changeset addresses modification in the javax.xml.validation package. It is a bit more complex than the changes required for the other packages because the newInstance methods takes an additional schemaLanguage parameter, and therefore we need to loop over the providers in order to find one that supports the language. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.validation/webrev.00/ Also this particular package did not have any specific configuration error to throw in case of misconfiguration (the xpath package in which the logic is very similar had one for instance), so we're adding a new SchemaFactoryConfigurationError for that purpose. I've taken an initial look at this and I'm wondering about SchemeFactory.newInstance throwing SchemaFactoryConfigurationError. Technically this is an incompatible change but in practical terms it may not be concern as this provider interface may not be used very much. Joe Wang - have you come across SchemaFactory implementations, I'm trying to get a feel for how much this is used, if ever. I don't have any data on how much the service mechanism may be used, Xerces would surely be the one most frequently used. I'm more concerned with the spec change that would require TCK change (the addition of SchemaFactoryConfigurationError related tests). Would that require MR? We probably need to run it with the JCK engineers. Joe -Alan
Re: RFR: javax.xml.validation: Using ServiceLoader to load JAXP schema factories (7169894: JAXP Plugability Layer: using service loader)
On 1/11/13 8:05 PM, Joe Wang wrote: I don't have any data on how much the service mechanism may be used, Xerces would surely be the one most frequently used. I'm more concerned with the spec change that would require TCK change (the addition of SchemaFactoryConfigurationError related tests). Although it seems cleaner to use a SchemaFactoryConfigurationError, we could wrap the ServiceConfigurationError in an IllegalArgumentException - which is what would have eventually been thrown in the old code. If that seems like a safer strategy I could update the changes in this sense. Best regards, -- daniel
Re: RFR: javax.xml.validation: Using ServiceLoader to load JAXP schema factories (7169894: JAXP Plugability Layer: using service loader)
On 1/11/2013 11:14 AM, Daniel Fuchs wrote: On 1/11/13 8:05 PM, Joe Wang wrote: I don't have any data on how much the service mechanism may be used, Xerces would surely be the one most frequently used. I'm more concerned with the spec change that would require TCK change (the addition of SchemaFactoryConfigurationError related tests). Although it seems cleaner to use a SchemaFactoryConfigurationError, we could wrap the ServiceConfigurationError in an IllegalArgumentException - which is what would have eventually been thrown in the old code. If that seems like a safer strategy I could update the changes in this sense. I agree. It's safer in that we could move forward with the change. FF for JDK8 is 1/23. Joe Best regards, -- daniel
Re: RFR: javax.xml.validation: Using ServiceLoader to load JAXP schema factories (7169894: JAXP Plugability Layer: using service loader)
On 11/01/2013 19:05, Joe Wang wrote: : I don't have any data on how much the service mechanism may be used, Xerces would surely be the one most frequently used. I'm more concerned with the spec change that would require TCK change (the addition of SchemaFactoryConfigurationError related tests). Would that require MR? Yes, there will be a MR of JSR-206 required for this work. -Alan.
hg: jdk8/tl/jdk: 8005978: shell tests need to use the $COMPILEJDK for javac, jar and other tools
Changeset: 7da291690aa0 Author:alanb Date: 2013-01-11 20:19 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7da291690aa0 8005978: shell tests need to use the $COMPILEJDK for javac, jar and other tools Reviewed-by: chegar ! test/ProblemList.txt ! test/com/sun/management/HotSpotDiagnosticMXBean/DumpHeap.sh ! test/com/sun/management/UnixOperatingSystemMXBean/GetMaxFileDescriptorCount.sh ! test/com/sun/management/UnixOperatingSystemMXBean/GetOpenFileDescriptorCount.sh ! test/java/io/FileOutputStream/FileOpen.sh ! test/java/io/Serializable/class/run.sh ! test/java/io/Serializable/evolution/RenamePackage/run.sh ! test/java/io/Serializable/maskSyntheticModifier/run.sh ! test/java/io/Serializable/packageAccess/run.sh ! test/java/io/Serializable/resolveClass/consTest/run.sh ! test/java/io/Serializable/resolveClass/deserializeButton/run.sh ! test/java/io/Serializable/superclassDataLoss/run.sh ! test/java/io/Serializable/unnamedPackageSwitch/run.sh ! test/java/lang/Class/getEnclosingClass/build.sh ! test/java/lang/ClassLoader/Assert.sh ! test/java/lang/ClassLoader/deadlock/TestCrossDelegate.sh ! test/java/lang/ClassLoader/deadlock/TestOneWayDelegate.sh ! test/java/lang/System/MacJNUEncoding/MacJNUEncoding.sh ! test/java/lang/Thread/UncaughtExceptions.sh ! test/java/lang/annotation/loaderLeak/LoaderLeak.sh ! test/java/lang/instrument/AppendToBootstrapClassPathSetUp.sh ! test/java/lang/instrument/AppendToClassPathSetUp.sh ! test/java/lang/instrument/BootClassPath/BootClassPathTest.sh ! test/java/lang/instrument/MakeJAR.sh ! test/java/lang/instrument/MakeJAR2.sh ! test/java/lang/instrument/MakeJAR3.sh ! test/java/lang/instrument/MakeJAR4.sh ! test/java/lang/instrument/ManifestTest.sh ! test/java/lang/instrument/ParallelTransformerLoader.sh ! test/java/lang/instrument/PremainClass/NoPremainAgent.sh ! test/java/lang/instrument/PremainClass/PremainClassTest.sh ! test/java/lang/instrument/PremainClass/ZeroArgPremainAgent.sh ! test/java/lang/instrument/RedefineBigClass.sh ! test/java/lang/instrument/RedefineClassWithNativeMethod.sh ! test/java/lang/instrument/RedefineMethodAddInvoke.sh ! test/java/lang/instrument/RedefineSetUp.sh ! test/java/lang/instrument/RetransformBigClass.sh ! test/java/lang/instrument/appendToClassLoaderSearch/CircularityErrorTest.sh ! test/java/lang/instrument/appendToClassLoaderSearch/ClassUnloadTest.sh ! test/java/lang/instrument/appendToClassLoaderSearch/CommonSetup.sh ! test/java/lang/instrument/appendToClassLoaderSearch/run_tests.sh ! test/java/net/Authenticator/B4933582.sh ! test/java/net/URL/B5086147.sh ! test/java/net/URL/runconstructor.sh ! test/java/net/URLClassLoader/B503.sh ! test/java/net/URLClassLoader/closetest/build.sh ! test/java/net/URLClassLoader/getresourceasstream/test.sh ! test/java/net/URLClassLoader/sealing/checksealed.sh ! test/java/net/URLConnection/6212146/test.sh ! test/java/net/URLConnection/UNCTest.sh ! test/java/nio/charset/spi/basic.sh ! test/java/rmi/activation/Activatable/extLoadedImpl/ext.sh ! test/java/rmi/registry/readTest/readTest.sh ! test/java/security/Security/ClassLoaderDeadlock/ClassLoaderDeadlock.sh ! test/java/security/Security/ClassLoaderDeadlock/Deadlock2.sh ! test/java/security/Security/signedfirst/Dyn.sh ! test/java/security/Security/signedfirst/Static.sh ! test/java/security/cert/CertificateFactory/slowstream.sh ! test/java/util/Formatter/Basic.sh ! test/java/util/Locale/LocaleProviders.sh ! test/java/util/PluggableLocale/ExecTest.sh ! test/java/util/ServiceLoader/basic.sh ! test/java/util/TimeZone/TimeZoneDatePermissionCheck.sh ! test/java/util/prefs/PrefsSpi.sh ! test/javax/crypto/SecretKeyFactory/FailOverTest.sh ! test/javax/script/CommonSetup.sh ! test/javax/script/ProviderTest.sh ! test/javax/security/auth/Subject/doAs/Test.sh ! test/lib/security/java.policy/Ext_AllPolicy.sh ! test/sun/management/jmxremote/bootstrap/PasswordFilePermissionTest.sh ! test/sun/management/jmxremote/bootstrap/SSLConfigFilePermissionTest.sh ! test/sun/management/jmxremote/startstop/JMXStartStopTest.sh ! test/sun/net/www/MarkResetTest.sh ! test/sun/net/www/http/HttpClient/RetryPost.sh ! test/sun/net/www/protocol/jar/B5105410.sh ! test/sun/net/www/protocol/jar/jarbug/run.sh ! test/sun/security/krb5/config/dns.sh ! test/sun/security/krb5/runNameEquals.sh ! test/sun/security/mscapi/IsSunMSCAPIAvailable.sh ! test/sun/security/pkcs11/KeyStore/Basic.sh ! test/sun/security/pkcs11/KeyStore/ClientAuth.sh ! test/sun/security/pkcs11/KeyStore/Solaris.sh ! test/sun/security/pkcs11/Provider/ConfigQuotedString.sh ! test/sun/security/pkcs11/Provider/Login.sh ! test/sun/security/provider/PolicyFile/GrantAllPermToExtWhenNoPolicy.sh ! test/sun/security/provider/PolicyFile/getinstance/getinstance.sh ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/EngineArgs/DebugReportsOneExtraByte.sh ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl/NotifyHandshakeTest.sh !
Re: RFR 8005962 : TEST_BUG: java/util/Properties/MacJNUEncoding can fail in certain environments
On 1/11/13 9:57 AM, Brent Christian wrote: On 1/11/13 9:54 AM, Chris Hegarty wrote: On 11/01/2013 17:50, Brent Christian wrote: I did an 'hg mv' of the directory containing the (two) test files. But I then had to 'hg export' to send the changeset for pushing. I'm guessing Yes, I noticed this before. 'hg export' and 'hg mv' are not friends. -Chris. I see that hg also has a 'bundle' and 'unbundle'. Has anyone used it? Would that have been better in this situation? Aha, so it was 'hg export' that was the culprit. I should have figured that since you (Brent) authored the changeset but Naoto had pushed it. In any case, hg export --git will export the changeset in a fashion that preserves the rename history. This should work fine with hg import. As far as I know, the --git option isn't the default because other patch-importing programs (such as patch itself) don't understand it. Bundles will also preserve rename information. I was a fan of bundles for a while. They're more difficult to work with, though. First, they're binary. Second, if you don't have the parent changeset of what's in the bundle, it's totally opaque; you can't see what's inside of it. I'd recommend using hg export --git. s'marks
Re: RFR: 8005582 - java/lang/Runtime/exec/WinCommand.java intermittent test failures
Thanks all for the entertaining horror story - glad I no longer need to fight with Windoze.
Re: Request for review: 6896617: Optimize sun.nio.cs.ISO_8859_1$Encode.encodeArrayLoop() on x86
On Jan 11, 2013, at 10:19 AM, Ulf Zibis ulf.zi...@cosoco.de wrote:
Hi Sherman,
Am 11.01.2013 06:47, schrieb Xueming Shen:
(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.
I've enhanced your suggestions, see more below...
Additionally, some part of encodeArrayLoop(...) maybe could be moved into a
separate method, to be reused by encode(char[] src, int sp, int len, byte[]
dst).
Some of the re-engineering could be adapted to the Decoder methods.
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.
I think we should balance 4 cases in rating the performance:
a) few loops, small buf/array
b) few loops, big buf/array
c) many loops, small buf/array
d) many loops, big buf/array
In a), b) the loop surrounding code will not be JIT-compiled, so should be
optimized for interpreter.
In c) d) the loop surrounding code *may be* JIT-compiled and consequtively
inline the inner loop, should be verified.
In b), d) the small inner loop, as demonstrated below, will be JIT-compiled,
regardless if moved into separate method.
But you guys noticed that sentence in the initial review request, right?
Move encoding loop into separate method for which VM will use intrinsic on
x86.
Just wanted to make sure ;-)
-- Chris
-Ulf
1) Check for (sp = sl) is superfluous.
==
private static int copyISOs(
char[] sa, int sp, byte[] da, int dp, int len) {
int i = 0;
for (; i len; i++) {
char c = sa[sp++];
// if (c '\uFF00' != 0) // needs bug 6935994 to be fixed, would be
fastest
if ((byte)(c 8) != 0) // temporary replacement, fast byte-length
operation on x86
break;
da[dp++] = (byte)c;
}
return i;
}
private CoderResult encodeArrayLoop(
CharBuffer src, ByteBuffer dst) {
char[] sa = src.array();
int soff = src.arrayOffset();
int sp = soff + src.position();
int sr = src.remaining();
int sl = sp + sr;
byte[] da = dst.array();
int doff = dst.arrayOffset();
int dp = doff + dst.position();
int dr = dst.remaining();
CoderResult cr;
if (dr sr) {
sr = dr;
cr = CoderResult.OVERFLOW;
} else
cr = CoderResult.UNDERFLOW;
try {
int ret = copyISOs(sa, sp, da, dp, sr);
sp = sp + ret;
dp = dp + ret;
if (ret != sr) {
if (sgp.parse(sa[sp], sa, sp, sl) 0)
return sgp.error();
return sgp.unmappableResult();
}
return cr;
} finally {
src.position(sp - soff);
dst.position(dp - doff);
}
}
2) Avoids sp, dp to be recalculated; shorter surrounding code - best chance
to become JIT-compiled.
==
// while inlinig, JIT will erase the surrounding int[] p
private static boolean copyISOs(
char[] sa, byte[] da, final int[] p, int sl) {
for (int sp = p[0], dp = p[1]; sp sl; sp++) {
char c = sa[sp];
// if (c '\uFF00' != 0) // needs bug 6935994 to be fixed, would be
fastest
if ((byte)(c 8) != 0) // temporary replacement, fast byte-length
operation on x86
return false;
da[dp++] = (byte)c;
}
return true;
}
private CoderResult encodeArrayLoop(
CharBuffer src, ByteBuffer dst) {
char[] sa = src.array();
int soff = src.arrayOffset();
int sp = soff + src.position();
int sr = src.remaining();
byte[] da = dst.array();
int doff = dst.arrayOffset();
int dp = doff + dst.position();
int dr = dst.remaining();
CoderResult cr;
if (dr sr) {
sr = dr;
cr = CoderResult.OVERFLOW;
} else
cr = CoderResult.UNDERFLOW;
try {
int sl = sp + sr;
final int[] p = { sp, dp };
if (!copyISOs(sa, da, p, sl)) {
if (sgp.parse(sa[sp], sa, sp, sl) 0)
return sgp.error();
return sgp.unmappableResult();
}
return cr;
} finally {
src.position(sp - soff);
dst.position(dp - doff);
}
}
3) No more needs try...finally block.
==
private CoderResult encodeArrayLoop(
CharBuffer src, ByteBuffer dst) {
char[] sa = src.array();
int soff = src.arrayOffset();
int sp = soff + src.position();
int sr = src.remaining();
byte[] da = dst.array();
int doff = dst.arrayOffset();
Re: Request for review: 6896617: Optimize sun.nio.cs.ISO_8859_1$Encode.encodeArrayLoop() on x86
Yes, I think I fully understand the purpose of the proposed change. This has been on the list for years. Just want to make sure we don't pay an unexpected price on use scenario that the intrinsic on x86 does not kick in. -Sherman On 01/11/2013 02:53 PM, Christian Thalinger wrote: But you guys noticed that sentence in the initial review request, right? Move encoding loop into separate method for which VM will use intrinsic on x86. Just wanted to make sure ;-) -- Chris
RFR (XS): CR 8003985: Support @Contended annotation
Hi guys, This is the JDK side of changes for supporting @Contended: http://cr.openjdk.java.net/~shade/8003985/webrev.jdk.02/ Status: - This change is ready to be pushed; due to licensing reasons with jsr166, we can only commit the stub; regular jsr166 sync will bring the updated annotation later. - JEP-142 is in Funded state (http://openjdk.java.net/jeps/142) - CCC is in place, and Approved. I need a sponsor to push this. Thanks, -Aleksey.
Re: RFR (XS): CR 8003985: Support @Contended annotation
On 01/11/2013 03:23 PM, Aleksey Shipilev wrote: Hi guys, This is the JDK side of changes for supporting @Contended: http://cr.openjdk.java.net/~shade/8003985/webrev.jdk.02/ Status: - This change is ready to be pushed; due to licensing reasons with jsr166, we can only commit the stub; regular jsr166 sync will bring the updated annotation later. - JEP-142 is in Funded state (http://openjdk.java.net/jeps/142) - CCC is in place, and Approved. I need a sponsor to push this. Thanks, -Aleksey. Looks fine; approved. Thanks, -Joe
Re: RFR: javax.xml.validation: Using ServiceLoader to load JAXP schema factories (7169894: JAXP Plugability Layer: using service loader)
On Jan 11, 2013, at 2:05 PM, Joe Wang wrote: On 1/11/2013 8:58 AM, Alan Bateman wrote: On 09/01/2013 14:28, Daniel Fuchs wrote: Hi, Here is a new webrev in the series that addresses using ServiceLoader in JAXP for JDK 8. 7169894: JAXP Plugability Layer: using service loader This changeset addresses modification in the javax.xml.validation package. It is a bit more complex than the changes required for the other packages because the newInstance methods takes an additional schemaLanguage parameter, and therefore we need to loop over the providers in order to find one that supports the language. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.validation/webrev.00/ Also this particular package did not have any specific configuration error to throw in case of misconfiguration (the xpath package in which the logic is very similar had one for instance), so we're adding a new SchemaFactoryConfigurationError for that purpose. I've taken an initial look at this and I'm wondering about SchemeFactory.newInstance throwing SchemaFactoryConfigurationError. Technically this is an incompatible change but in practical terms it may not be concern as this provider interface may not be used very much. Joe Wang - have you come across SchemaFactory implementations, I'm trying to get a feel for how much this is used, if ever. I don't have any data on how much the service mechanism may be used, Xerces would surely be the one most frequently used. I'm more concerned with the spec change that would require TCK change (the addition of SchemaFactoryConfigurationError related tests). Would that require MR? We probably need to run it with the JCK engineers. If you are changing the exception, then you would need an MR for this and want to update any TCK/JCK tests (or add tests) Joe -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
hg: jdk8/tl/jdk: 7131459: [Fmt-De] DecimalFormat produces wrong format() results when close to a tie
Changeset: bc1f16f5566f Author:darcy Date: 2013-01-11 15:39 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/bc1f16f5566f 7131459: [Fmt-De] DecimalFormat produces wrong format() results when close to a tie Reviewed-by: darcy Contributed-by: olivier.lagn...@oracle.com ! src/share/classes/java/text/DigitList.java ! src/share/classes/sun/misc/FloatingDecimal.java + test/java/text/Format/DecimalFormat/TieRoundingTest.java
Re: RFR 8005311: Add Scalable Updatable Variables, DoubleAccumulator, DoubleAdder, LongAccumulator, LongAdder
On 01/11/2013 09:18 AM, Chris Hegarty wrote: On 11/01/2013 16:57, Doug Lea wrote: On 01/11/13 11:35, Peter Levart wrote: On 01/11/2013 05:18 PM, Chris Hegarty wrote: Now with explicit disclaimer on DoubleA* The order of accumulation within or across threads is not guaranteed. Thus, this class may not be applicable if numerical stability is required when combining values of substantially different orders of magnitude. It doesn't have to be substantially different order of magnitude. Thanks. Chris, please add especially: Hello, I would prefer to cautionary note along the lines of if you want numerical accuracy, take care to use a summation algorithm with defined worst-case behavior. (Varying magnitude is not so much of a problem if you add things up in the right order.) -Joe Thanks Doug, done. -Chris. The order of accumulation within or across threads is not guaranteed. Thus, this class may not be applicable if numerical stability is required, especially when combining values of substantially different orders of magnitude. -Doug
Re: Review request JDK-8004729: Parameter Reflection API
I got halfway through implementing a change that would synthesize
Parameter's for the static links (this for the inner classes), when it
occurred to me: is there really a case for allowing reflection on those
parameters.
Put another way,
public class Foo {
public class Bar {
int baz(int qux) { }
}
}
Should baz.getParameters() return just qux, or should it expose
Foo.this? On second thought, I can't think of a good reason why you
*want* to reflect on Foo.this.
Also, the logic is much simpler. You just have to figure out how deep
you are in inner classes, store that information somewhere, and offset
by that much every time a Parameter calls back to its Executable to get
information. The logic for synthesizing the this parameters is much
more complex.
Thoughts?
On 01/11/13 13:27, Joe Darcy wrote:
Hi Eric,
Taking another look at the code, some extra logic / checking is needed
in cases where the number of source parameters (non-synthetic and
non-synthesized) disagrees with the number of actual parameters at a
class file level.
For example, if the single source parameter of an inner class
constructor is annotated, the annotation should be associated with the
*second* parameter since the first class file parameter is a synthesized
constructor added by the compiler. I think generally annotations should
not be associated with synthesized or synthetic parameters.
-Joe
On 1/11/2013 9:03 AM, Eric McCorkle wrote:
Update should be visible now.
On 01/11/13 11:54, Vitaly Davidovich wrote:
Yes that's exactly what I'm looking for as well.
Sent from my phone
On Jan 11, 2013 11:25 AM, Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com wrote:
On 01/11/2013 04:54 PM, Eric McCorkle wrote:
The webrev has been updated again.
The multiple writes to parameters have been removed, and the
tests have
been expanded to look at inner classes, and to test modifiers.
Please look over it again.
Hello Eric,
You still have 2 reads of volatile even in fast path. I would do it
this way:
private Parameter[] privateGetParameters() {
Parameter[] tmp = parameters; // one and only read
if (tmp != null)
return tmp;
// Otherwise, go to the JVM to get them
tmp = getParameters0();
// If we get back nothing, then synthesize parameters
if (tmp == null) {
final int num = getParameterCount();
tmp = new Parameter[num];
for (int i = 0; i num; i++)
// TODO: is there a way to synthetically derive the
// modifiers? Probably not in the general case, since
// we'd have no way of knowing about them, but there
// may be specific cases.
tmp[i] = new Parameter(arg + i, 0, this, i);
// This avoids possible races from seeing a
// half-initialized parameters cache.
}
parameters = tmp;
return tmp;
}
Regards, Peter
Test-wise, I've got a clean run on JPRT (there were some
failures in
lambda stuff, but I've been seeing that for some time now).
On 01/10/13 21:47, Eric McCorkle 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
hg: jdk8/tl/jdk: 2 new changesets
Changeset: 6f6246aced89 Author:sherman Date: 2013-01-11 22:43 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/6f6246aced89 8005466: JAR file entry hash table uses too much memory (zlib_util.c) Summary: realign the fields of jzcell struct Reviewed-by: sherman Contributed-by: ioi@oracle.com ! src/share/native/java/util/zip/zip_util.h Changeset: 8009c7e3899e Author:sherman Date: 2013-01-11 22:45 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8009c7e3899e Merge
