Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs
Alan, I checked the httpsever and sctp changes. All look good to me. -Chris. On 10/06/2013 09:03 PM, Alan Bateman wrote: As a follow-up to Joe Darcy's rename of jdk.Supported to jdk.Exported, I'd like to have another attempt at adding the annotation to a number of JDK specific APIs that are long standing exported, documented and supported APIs. Specifically, the following APIs: - Java Debug Interface (com.sun.jdi) - Attach API (com.sun.tools.attach) - SCTP API (com.sun.nio.sctp) - HTTP server API (com.sun.net.httpserver) - Management extensions (com.sun.management) - JConsole Plugin API (com.sun.tools.jconsole) - JDK-specific API to JAAS (com.sun.security.auth) - JDK-specific JGSS API (com.sun.security.jgss) (The javadoc for each of these APIs is currently generated in the build) The webrev with the proposed update is here: http://cr.openjdk.java.net/~alanb/8008662/webrev.02/ As per the original webrev, I've added package-info.java to a number of packages that didn't have any description. In a few cases, I've had to rename the legacy package.html to package-info.java. For the review then the intention is that @jdk.Exported be added to the package-info and all public/protected types in these APIs. The only exceptions are two cases where I've added @jdk.Exported(false), specifically: - com.sun.management.OSMBeanFactory as it clearly documents to stay away - com.sun.security.auth.callback.DialogCallbackHandler as it for the chop (see JEP 162) Thanks, Alan.
Review Request for 8025799: Restore sun.reflect.Reflection.getCallerClass(int)
JDK 8 was feature complete in June and there just isn't sufficient time remaining to get agreement and feedback on an API to examine the caller frames. To that end, I propose to restore the old unsupported Reflection.getCallerClass(int) and that we will look to define a standard API for JDK 9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8025799/ It remains to be an unsupported API and JDK should not use this method and it's not annotated with @CallerSensitive. I considered detecting if this method is called by a system class (loaded by null loader) and throw an error. I decided to minimize the compatibility risk in case if there is any existing code added to the bootclasspath depending on this private API. Thanks Mandy
Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs
The JAAS and JGSS changes look fine too. On 7 Oct 2013, at 09:23, Chris Hegarty wrote: Alan, I checked the httpsever and sctp changes. All look good to me. -Chris. On 10/06/2013 09:03 PM, Alan Bateman wrote: As a follow-up to Joe Darcy's rename of jdk.Supported to jdk.Exported, I'd like to have another attempt at adding the annotation to a number of JDK specific APIs that are long standing exported, documented and supported APIs. Specifically, the following APIs: - Java Debug Interface (com.sun.jdi) - Attach API (com.sun.tools.attach) - SCTP API (com.sun.nio.sctp) - HTTP server API (com.sun.net.httpserver) - Management extensions (com.sun.management) - JConsole Plugin API (com.sun.tools.jconsole) - JDK-specific API to JAAS (com.sun.security.auth) - JDK-specific JGSS API (com.sun.security.jgss) (The javadoc for each of these APIs is currently generated in the build) The webrev with the proposed update is here: http://cr.openjdk.java.net/~alanb/8008662/webrev.02/ As per the original webrev, I've added package-info.java to a number of packages that didn't have any description. In a few cases, I've had to rename the legacy package.html to package-info.java. For the review then the intention is that @jdk.Exported be added to the package-info and all public/protected types in these APIs. The only exceptions are two cases where I've added @jdk.Exported(false), specifically: - com.sun.management.OSMBeanFactory as it clearly documents to stay away - com.sun.security.auth.callback.DialogCallbackHandler as it for the chop (see JEP 162) Thanks, Alan.
Re: Review Request for 8025799: Restore sun.reflect.Reflection.getCallerClass(int)
On 10/07/2013 10:24 AM, Mandy Chung wrote: JDK 8 was feature complete in June and there just isn't sufficient time remaining to get agreement and feedback on an API to examine the caller frames. To that end, I propose to restore the old unsupported Reflection.getCallerClass(int) and that we will look to define a standard API for JDK 9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8025799/ It remains to be an unsupported API and JDK should not use this method and it's not annotated with @CallerSensitive. I considered detecting if this method is called by a system class (loaded by null loader) and throw an error. I decided to minimize the compatibility risk in case if there is any existing code added to the bootclasspath depending on this private API. Thanks Mandy The Java side is Ok for me. cheers, Rémi
Re: RFR: 8025968: Integrate test improvements made in lambda repo
On Oct 5, 2013, at 1:13 AM, Henry Jen henry@oracle.com wrote: Hi, Please review a port from lambda repo to tl, it contains a refactoring of OpTestCase and a small addition to fill test gap. http://cr.openjdk.java.net/~henryjen/tl/8025968/0/webrev/ Looks good. Paul.
Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs
Hi Alan, The com.sun.management changes look good. -- daniel On 10/6/13 10:03 PM, Alan Bateman wrote: As a follow-up to Joe Darcy's rename of jdk.Supported to jdk.Exported, I'd like to have another attempt at adding the annotation to a number of JDK specific APIs that are long standing exported, documented and supported APIs. Specifically, the following APIs: - Java Debug Interface (com.sun.jdi) - Attach API (com.sun.tools.attach) - SCTP API (com.sun.nio.sctp) - HTTP server API (com.sun.net.httpserver) - Management extensions (com.sun.management) - JConsole Plugin API (com.sun.tools.jconsole) - JDK-specific API to JAAS (com.sun.security.auth) - JDK-specific JGSS API (com.sun.security.jgss) (The javadoc for each of these APIs is currently generated in the build) The webrev with the proposed update is here: http://cr.openjdk.java.net/~alanb/8008662/webrev.02/ As per the original webrev, I've added package-info.java to a number of packages that didn't have any description. In a few cases, I've had to rename the legacy package.html to package-info.java. For the review then the intention is that @jdk.Exported be added to the package-info and all public/protected types in these APIs. The only exceptions are two cases where I've added @jdk.Exported(false), specifically: - com.sun.management.OSMBeanFactory as it clearly documents to stay away - com.sun.security.auth.callback.DialogCallbackHandler as it for the chop (see JEP 162) Thanks, Alan.
hg: jdk8/tl/jdk: 8025983: Typo in Javadoc of Files.isRegularFile()
Changeset: 0ac9dc315071 Author:alanb Date: 2013-10-07 11:48 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0ac9dc315071 8025983: Typo in Javadoc of Files.isRegularFile() Reviewed-by: mchung, chegar ! src/share/classes/java/nio/file/Files.java ! src/share/classes/java/nio/file/Path.java
Re: Review Request for 8025799: Restore sun.reflect.Reflection.getCallerClass(int)
On 07/10/2013 09:24, Mandy Chung wrote: JDK 8 was feature complete in June and there just isn't sufficient time remaining to get agreement and feedback on an API to examine the caller frames. To that end, I propose to restore the old unsupported Reflection.getCallerClass(int) and that we will look to define a standard API for JDK 9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8025799/ It remains to be an unsupported API and JDK should not use this method and it's not annotated with @CallerSensitive. I considered detecting if this method is called by a system class (loaded by null loader) and throw an error. I decided to minimize the compatibility risk in case if there is any existing code added to the bootclasspath depending on this private API. I liked the direction this was going with the walkStack and firstCaller proposal but it is way too late in jdk8 to have any time to get feedback. So I agree that temporarily restoring the unsupported Reflection.getCallerClass(int) is the right thing for now. As the standard API is likely to be significant then it probably warrants a JEP. As regards the webrev then the changes looks okay. I guess you could rename one of the native methods to avoid the non-obvious name mangling but since it is only temporary then I could live with what you have. -Alan
Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs
On 07/10/2013 11:31, Mandy Chung wrote: : For the review then the intention is that @jdk.Exported be added to the package-info and all public/protected types in these APIs. The only exceptions are two cases where I've added @jdk.Exported(false), specifically: - com.sun.management.OSMBeanFactory as it clearly documents to stay away It's a bug. We should fix it and remove this public class (I have filed JDK-8025985 for it). Thanks for creating the bug, I agree it should be removed. For now, I'll leave @jdk.Exported(false), unless that issue is resolved before we push this (I hope that is okay with you). -Alan.
Re: RFR (S) CR 6857566: (bf) DirectByteBuffer garbage creation can outpace reclamation
More test data... Running the previously mentioned measurement (allocating direct buffers randomly sized between 256KB and 1MB for 60 seconds) with a single allocating tread that presents allocation pressure which is still acceptable for original code (two threads are already too much an lead to OOME), the results, using 4-core i7 CPU and -XX:MaxDirectMemorySize=128m are: - 482403 allocations satisfied without helping ReferenceHandler - 75 allocations satisfied while helping ReferenceHandler but before System.gc() - 2373 allocations satisfied while helping ReferenceHandler, after System.gc() but before any sleeps - no sleeps avg. allocation: 0.12 ms/op (original code: 0.6 ms/op) This test may be regarded as an edge test. No current real-world application exhibits substantialy higher allocation pressure. If it did, it would throw OOME, so it would be unusable. Above numbers show that majority of allocations (99.5%) happen without helping ReferenceHandler thread. Only every 200th disturbs the ReferenceHandler thread, and it does exactly what ReferenceHandler does asynchronously and because the ReferenceHandler thread is asleep. So it actually increases the promptness of Reference enqueue-ing - not something that could hurt. We can reasonably expect that in current real-world applications helping ReferenceHandler would happen even less frequently, and could not negatively impact application behaviour. What applications will see is up to 5x improvement in throughput of allocation (a result of using atomic operations for reservation and less sleeping). For comparison, here are also the results for 2 allocating threads (higher allocation pressure than any current real-world application): - 734916 allocations satisfied without helping ReferenceHandler - 3112 allocations satisfied while helping ReferenceHandler but before System.gc() - 3817 allocations satisfied while helping ReferenceHandler, after System.gc() but before any sleeps - no sleeps avg. allocation: 0.16 ms/op (per thread) This is still 99.1% allocations without disturbing the peaceful flow of ReferenceHandler thread. Regards, Peter On 10/07/2013 12:56 AM, Peter Levart wrote: Hi Again, The result of my experimentation is as follows: Letting ReferenceHandler thread alone to en-queue References and execute Cleaners is not enough to prevent OOMEs when allocation is performed in large number of threads even if I let Cleaners do only synchronous announcing of what will be freed (very fast), delegate the actual de-allocation to a background thread and base reservation waiting on announced free space (still wait that space is deallocated and unreserved before satisfying reservation request, but wait as long as it takes if the announced free space is enough for reservation request). ReferenceHandler thread, when it finds that it has no more pending References, parks and waits for notification from VM. The VM promptly process references (hooks them on the pending list), but with saturated CPUs, waking-up the ReferenceHandler thread and re-gaining the lock takes too much time. During that time allocating threads can reserve the whole permitted space and OOME must be thrown. So I'm back to strategy #1 - helping ReferenceHandler thread. It's not so much about helping to achieve better throughput (as I noted deallocating can not be effectively parallelized) but to overcome the latency of waking-up the ReferenceHandler thread. Here's my attempt at doing this: http://cr.openjdk.java.net/~plevart/jdk8-tl/DyrectBufferAlloc/webrev.01/ This is much simplified from my 1st submission of similar strategy. I tried to be as undisruptive to current logic of Reference processing as possible, but of course you decide if this is still too risky for inclusion into JDK8. Cleaner is unchanged - it processes it's thunk synchronously and ReferenceHandler thread invokes it directly. ReferenceHandler logic is the same - I just factored-out the content of the loop into a private method to be able to call it from nio Bits where the bulk of change lies. The (un)reservation logic is re-implemented with atomic operations - no locks. When large number of threads are competing for reservation, locking overhead can be huge and can slow-down unreservation (which must use the same lock as reservation). The reservation re-try logic 1st tries to satisfy the reservation request while helping ReferenceHandler thread in en-queue-ing References and executing Cleaners until the list of pending references is exhausted. If this does not succeed, it triggers VM to process references (System.gc()) and then enters similar re-try loop but introducing exponentially increasing back-off delay every time the chain of pending references is exhausted, starting with 1ms sleep and doubling. This gives VM time to process the references. Maximum number of sleeps is 9, giving max. accumulated sleep time of 0.5 s. This means that a
Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs
7 classes in com.sun.security.auth have been deprecated for several major releases now. Should they still have this annotation? --Sean On 10/06/2013 04:03 PM, Alan Bateman wrote: As a follow-up to Joe Darcy's rename of jdk.Supported to jdk.Exported, I'd like to have another attempt at adding the annotation to a number of JDK specific APIs that are long standing exported, documented and supported APIs. Specifically, the following APIs: - Java Debug Interface (com.sun.jdi) - Attach API (com.sun.tools.attach) - SCTP API (com.sun.nio.sctp) - HTTP server API (com.sun.net.httpserver) - Management extensions (com.sun.management) - JConsole Plugin API (com.sun.tools.jconsole) - JDK-specific API to JAAS (com.sun.security.auth) - JDK-specific JGSS API (com.sun.security.jgss) (The javadoc for each of these APIs is currently generated in the build) The webrev with the proposed update is here: http://cr.openjdk.java.net/~alanb/8008662/webrev.02/ As per the original webrev, I've added package-info.java to a number of packages that didn't have any description. In a few cases, I've had to rename the legacy package.html to package-info.java. For the review then the intention is that @jdk.Exported be added to the package-info and all public/protected types in these APIs. The only exceptions are two cases where I've added @jdk.Exported(false), specifically: - com.sun.management.OSMBeanFactory as it clearly documents to stay away - com.sun.security.auth.callback.DialogCallbackHandler as it for the chop (see JEP 162) Thanks, Alan.
Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs
On 07/10/2013 13:26, Sean Mullan wrote: 7 classes in com.sun.security.auth have been deprecated for several major releases now. Should they still have this annotation? --Sean I know but aren't they still exported and supported? DialogCallbackHandler is the only one with its name on a bullet. -Alan.
Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs
On 10/07/2013 08:28 AM, Alan Bateman wrote: On 07/10/2013 13:26, Sean Mullan wrote: 7 classes in com.sun.security.auth have been deprecated for several major releases now. Should they still have this annotation? --Sean I know but aren't they still exported and supported? They have all had API replacements since JDK 1.4 or 1.5. There has been plenty of time to transition. DialogCallbackHandler is the only one with its name on a bullet. We have only started removing some deprecated things in JDK, so it just was never thought about until now. Marking these as supported going forward strikes me as a bit strange, since we don't really want anyone using these anymore. --Sean
Re: RFR (S) CR 6857566: (bf) DirectByteBuffer garbage creation can outpace reclamation
Hi Peter, On 10/07/2013 02:56 AM, Peter Levart wrote: http://cr.openjdk.java.net/~plevart/jdk8-tl/DyrectBufferAlloc/webrev.01/ The 1 hour run of this on my 1x2x2 i5, directMem=16m, original DirectByteBufferTest yields no failures! Oh my. Good job! Comments on the code: Bits.java: - Thread.currentThread().interrupt() only sets the interruption flag, it does not break the control flow. You can call it directly in the exception handler, sparing some boiler-plate code. - Given that you about to retry, do you think catching the interrupt during sleep should immediately return? Or, try the last time to reserve some memory, and either return or throw OOME? - Please avoid inline assignments. - I'd like to embed the comment why 9 is the magic number. Reference.java: - Please move the static initializer block back, that will be cleaner change. - Please keep the (r instanceof Cleaner) block as the separate if; makes it visually different from the common code path. DirectBufferAllocTest.java: - for(;;) - while(true) - Can we turn the 60*1000 into the constant? - I think 10 seconds is enough for regression test. - Can we fold the relevant into Integer.getInteger(..., #const) to make them tunable, while still claiming it to be the regtest in the default mode? Put the comment which settings should be used to turn this test into the stress test. Total of 909960 allocations were performed: - 247993 were satisfied before attempting to help ReferenceHandler thread - 660184 were satisfied while helping ReferenceHandler thread but before triggering System.gc() - 1783 were satisfied after triggering System.gc() but before doing any sleep - no sleeping has been performed The same test, just changing -XX:MaxDirectMemorySize=128m (that means 1MB per thread each allocating direct buffers randomly sized between 256KB and 1MB): Total of 579943 allocations were performed: - 131547 were satisfied before attempting to help ReferenceHandler thread - 438345 were satisfied while helping ReferenceHandler thread but before triggering System.gc() - 10016 were satisfied after triggering System.gc() but before doing any sleep - 34 were satisfied after sleep(1) - 1 was satisfied after sleep(1) followed by sleep(2) Thank you! I was meant to do this for my original patch. (In fact I did some debug printing on the recovery paths, only to see we almost never hit them). So what do you think? Is this still too risky for JDK8? I think that the patch is great. We are near the JDK 8 endgame though. As much as I will be happy to have that in 8u0, this is probably something for early JDK 9, with a backport to 8u2? -Aleksey.
Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs
On 07/10/2013 13:36, Sean Mullan wrote: We have only started removing some deprecated things in JDK, so it just was never thought about until now. Marking these as supported going forward strikes me as a bit strange, since we don't really want anyone using these anymore. As a guide, I think we should plan to address these anomalies for 9 so that we can export APIs on a per API package basis. I'm okay to change these to @jdk.Exported(false) if we can flag them for removal (like we did for DialogCallbackHandler). -Alan.
Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs
On 10/07/2013 08:58 AM, Alan Bateman wrote: On 07/10/2013 13:36, Sean Mullan wrote: We have only started removing some deprecated things in JDK, so it just was never thought about until now. Marking these as supported going forward strikes me as a bit strange, since we don't really want anyone using these anymore. As a guide, I think we should plan to address these anomalies for 9 so that we can export APIs on a per API package basis. I'm okay to change these to @jdk.Exported(false) if we can flag them for removal (like we did for DialogCallbackHandler). Sure. I'll file a bug to have these deprecated classes removed in 9. --Sean
Re: SplittableRandom update
While having one last look at this Doug and I found some oops how the heck did that happen miscommits with the mix64/mix32/nextGamma methods that contained test code for measuring the impact of shifts: For corrections see: http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/SplittableRandom.java?r1=1.23r2=1.24 Updated patch: http://cr.openjdk.java.net/~psandoz/tl/JDK-8025136-SR-enhancements/webrev/ I will commit this tomorrow unless there are objections. Paul. On Sep 20, 2013, at 4:11 PM, Doug Lea d...@cs.oswego.edu wrote: In the course of writing up a report (coming soon) that includes discussion of SplittableRandom, we had a chance to further analyze and test things, resulting in a few small improvements. Plus some internal renamings to better reflect intent. Plus now with the same initial seed mechanics discussed a few days ago for ThreadLocalRandom. Thanks to Paul Sandoz for creating webrevs: https://bugs.openjdk.java.net/browse/JDK-8025136 http://cr.openjdk.java.net/~psandoz/tl/JDK-8025136-SR-enhancements/webrev/ I think we need one more reviewer for it. -Doug
Re: RFR 8024660: TEST_BUG: java/lang/ProcessBuilder/*IOHandle.java leaving hotspot.log open in fastdebug builds
On 04/10/2013 16:10, Rob McKenna wrote: Hi Pavel, Thanks for sorting this out. I'm not a reviewer but hopefully Alan will have a look when he gets a chance. Based on the bug description this looks good to me though. -Rob I looked over the weekend and it's mostly okay (thanks Pavel for taking one, we don't do enough execution of these tests with fastdebug builds so I'm sure this isn't the only issue that we have). A minor comment is that it might be a bit cleaner to throw RuntimeException rather than Error but it's not a big deal in this test. The only real comment/question is whether performB should fail if process.waitFor is interrupted, this shouldn't happen. Rob - do you plan to sponsor this for Pavel?
Re: RFR 8024660: TEST_BUG: java/lang/ProcessBuilder/*IOHandle.java leaving hotspot.log open in fastdebug builds
Yep. -Rob On 07/10/13 14:24, Alan Bateman wrote: On 04/10/2013 16:10, Rob McKenna wrote: Hi Pavel, Thanks for sorting this out. I'm not a reviewer but hopefully Alan will have a look when he gets a chance. Based on the bug description this looks good to me though. -Rob I looked over the weekend and it's mostly okay (thanks Pavel for taking one, we don't do enough execution of these tests with fastdebug builds so I'm sure this isn't the only issue that we have). A minor comment is that it might be a bit cleaner to throw RuntimeException rather than Error but it's not a big deal in this test. The only real comment/question is whether performB should fail if process.waitFor is interrupted, this shouldn't happen. Rob - do you plan to sponsor this for Pavel?
Re: RFR (S) CR 6857566: (bf) DirectByteBuffer garbage creation can outpace reclamation
On 06/10/2013 23:56, Peter Levart wrote: : It's not so much about helping to achieve better throughput (as I noted deallocating can not be effectively parallelized) but to overcome the latency of waking-up the ReferenceHandler thread. Here's my attempt at doing this: http://cr.openjdk.java.net/~plevart/jdk8-tl/DyrectBufferAlloc/webrev.01/ This is much simplified from my 1st submission of similar strategy. I tried to be as undisruptive to current logic of Reference processing as possible, but of course you decide if this is still too risky for inclusion into JDK8. Cleaner is unchanged - it processes it's thunk synchronously and ReferenceHandler thread invokes it directly. ReferenceHandler logic is the same - I just factored-out the content of the loop into a private method to be able to call it from nio Bits where the bulk of change lies. : So what do you think? Is this still too risky for JDK8? I looked at the latest webrev and I think the approach looks good. I should explain that I did look into this issue about 3-4 years ago and at the time I experimented with the allocating threads waiting until the reference handler had drained the pending list. I didn't think of doing the assist at the time, hence I was interested to see the # allocations where it helped. On the patch then I agree with Aleksey that moving the static initializer makes it less obvious that the only change is registering the shared secret (it's not a big deal of course). The back-off before retrying looks good, I just wonder if 1ms is too low to start with. On the interrupt then I think it's okay to just set the interrupt status as you are doing. I see you switched the tracking for the management interface to use AtomicLong. Are you looking to improve the concurrency or is there another reason? A minor coding convention but the break before else and finally is inconsistent in these areas. Another consistency point is that maxsleeps is a constant and so should probably be in uppercase. A related piece of work is the FileChannel map implementation where there is a gc + retry if mmap fails. This could be changed to have a similar back-off/retry. On the test then the copyright date is 2001-2007 so I assume this was copied from somewhere :-) I agree with Aleksey on the test duration, especially if you can provoke OOME in less than 10 or 20 seconds on some machines. As regards whether this should go into JDK 8 then the updated proposal is significantly less risky that the original proposal that changed the implementation to use weak references. That said, this is a 13 year old issue that hasn't come up very often (to my knowledge anyway, perhaps because those making heavy use of direct buffers are pooling buffers rather than allocating and unreferencing). In additional we are close to the end of JDK 8 (ZBB is in 2.5 weeks time) and technically we have been in ramp down (meaning P1-P3 only) since mid-July. -Alan.
Debug builds
Hi, Where did the old debug builds of rt.jar go (meaning, rt.jar with full debug symbols, even for local variables)? I scanned the mailing list for a related discussion but couldn't find anything. It looks like somewhere down the line a decision was made to remove these builds, but it's not clear why that was. Also, out of curiosity, what is the overhead of the full debug symbols (versus what we ship now)? Couldn't we ship a full-debug rt.jar with the JDK and ship the smaller rt.jar with the JRE? Thanks, Gili
Re: RFR (S) CR 6857566: (bf) DirectByteBuffer garbage creation can outpace reclamation
On 10/07/2013 02:43 PM, Aleksey Shipilev wrote:
Hi Peter,
On 10/07/2013 02:56 AM, Peter Levart wrote:
http://cr.openjdk.java.net/~plevart/jdk8-tl/DyrectBufferAlloc/webrev.01/
The 1 hour run of this on my 1x2x2 i5, directMem=16m, original
DirectByteBufferTest yields no failures! Oh my. Good job!
Thanks!
Comments on the code:
Bits.java:
- Thread.currentThread().interrupt() only sets the interruption flag,
it does not break the control flow. You can call it directly in the
exception handler, sparing some boiler-plate code.
...then the next possible sleep() in loop will immediately throw
InterruptedException again. That's why I set the flag and set the
interrupted status just before returning in a finally block where all
exit paths converge...
- Given that you about to retry, do you think catching the interrupt
during sleep should immediately return? Or, try the last time to reserve
some memory, and either return or throw OOME?
I have a rule that when InterruptedException is caught in a method (or
method invokes Thread.interrupted() which returns true), then this
method must do one of the following 3 things:
- throw InterruptedException or
- complete normally or throw any other exception, but make sure current
Thread's interrupted status is set before completing/throwing or
- never complete
That's the only way to not swallow interrupts.
Now in this concrete example, I must not loop indefinitely, I can not
throw InterruptedException, neither can I complete normally, since that
would mean the memory has been reserved (which it hasn't). I could throw
OOME *and* set the interrupted status of current thread, but as we have
seen from the DirectBufferTest I ran, some allocations did need
occasional sleep(1) or even sleep(2) before completing normally. If
thread had interrupted status set in such situations, allocation would
throw OOME, which feels wrong, since it would not be thrown if the
thread was not interrupted.
So it seems best to continue processing and pretend that
InterruptedException never happened, but make sure interrupted status is
set before completing.
What I should correct is, that when the InterruptedException is caught
during sleep, 'sleeps' counter is not incremented and 'sleepTime' is not
doubled, since the wake-up was premature and should not count - 9
consecutive interrupts could theoretically provoke OOME because the loop
would exit after 9 fake sleeps.
- Please avoid inline assignments.
Doug's school ;-)
You mean the following:
long totalCap;
while (cap = maxMemory - (totalCap = totalCapacity.get())) {
if (totalCapacity.compareAndSet(totalCap, totalCap + cap)) {
...
return true;
}
}
So how would you write this? Alternatives:
a)
while (true) {
long totalCap = totalCapacity.get();
if (cap maxMemory - totalCap) {
break;
}
if (totalCapacity.compareAndSet(totalCap, totalCap + cap)) {
...
return true;
}
}
b)
long totalCap = totalCapacity.get();
while (cap = maxMemory - totalCap) {
if (totalCapacity.compareAndSet(totalCap, totalCap + cap)) {
...
return true;
}
totalCap = totalCapacity.get();
}
c)
for (long totalCap = totalCapacity.get();
cap = maxMemory - totalCap;
totalCap = totalCapacity.get()) {
if (totalCapacity.compareAndSet(totalCap, totalCap + cap)) {
...
return true;
}
}
d) ???
- I'd like to embed the comment why 9 is the magic number.
Ok. WIll do.
Reference.java:
- Please move the static initializer block back, that will be cleaner
change.
I actually haven't moved the static initializer block. It's still
immediately after the ReferenceHandler class (at least syntactically). I
moved the content of the for loop in ReferenceHandler#run() method out
into a new method inserted after the static initializer. Sdiffs tool
matches by similarity of content when aligning left and right panes. I
can insert the new method between ReferenceHandler class and static
initializer, thus moving the static initializer, and Sdiffs will show
less blue text, but the hg changeset might be even more confusing to
read (or merge) that way. I will try and see what happens.
- Please keep the (r instanceof Cleaner) block as the separate if;
makes it visually different from the common code path.
Right.
DirectBufferAllocTest.java:
- for(;;) - while(true)
Ok.
- Can we turn the 60*1000 into the constant?
Ok.
- I think 10 seconds is enough for regression test.
I think too. It usually fails after few seconds.
- Can we fold the relevant into Integer.getInteger(..., #const) to
make them tunable, while still claiming it to be the
Re: RFR 8024660: TEST_BUG: java/lang/ProcessBuilder/*IOHandle.java leaving hotspot.log open in fastdebug builds
Hi Alan, On Monday 07 October 2013 14:24:40 you wrote: On 04/10/2013 16:10, Rob McKenna wrote: Hi Pavel, Thanks for sorting this out. I'm not a reviewer but hopefully Alan will have a look when he gets a chance. Based on the bug description this looks good to me though. -Rob I looked over the weekend and it's mostly okay (thanks Pavel for taking one, we don't do enough execution of these tests with fastdebug builds so I'm sure this isn't the only issue that we have). Thanks for looking on this A minor comment is that it might be a bit cleaner to throw RuntimeException rather than Error but it's not a big deal in this test. Error throwing on InterruptedException was added to not break code style in the code that throws only Errors all over the test. If you want to I can change this Error (and other throws too) to RuntimeException. The only real comment/question is whether performB should fail if process.waitFor is interrupted, this shouldn't happen. Printing B was interrupted while waiting for C on InterruptedException could help if we had a regression and performC were looped. When this happens Jtreg hits timeout and kills/ends all processes. Messages of each processes that waited for another one will be printed to stderr. I think it will ease failure analysis in some situations like hanging, host or VM slowness. Thanks, Pavel
Re: Debug builds
On Oct 7, 2013, at 8:30 AM, cowwoc cow...@bbs.darktech.org wrote: Hi, Where did the old debug builds of rt.jar go (meaning, rt.jar with full debug symbols, even for local variables)? I scanned the mailing list for a related discussion but couldn't find anything. It looks like somewhere down the line a decision was made to remove these builds, but it's not clear why that was. Also, out of curiosity, what is the overhead of the full debug symbols (versus what we ship now)? Couldn't we ship a full-debug rt.jar with the JDK and ship the smaller rt.jar with the JRE? +1 on shipping debug builds with the JDK, that would be very helpful for those of us who are stupid^Wcurious enough to step-debug into JDK classes.
Re: Debug builds
On 07/10/2013 1:35 PM, Steven Schlansker wrote: On Oct 7, 2013, at 8:30 AM, cowwoc cow...@bbs.darktech.org wrote: Hi, Where did the old debug builds of rt.jar go (meaning, rt.jar with full debug symbols, even for local variables)? I scanned the mailing list for a related discussion but couldn't find anything. It looks like somewhere down the line a decision was made to remove these builds, but it's not clear why that was. Also, out of curiosity, what is the overhead of the full debug symbols (versus what we ship now)? Couldn't we ship a full-debug rt.jar with the JDK and ship the smaller rt.jar with the JRE? +1 on shipping debug builds with the JDK, that would be very helpful for those of us who are stupid^Wcurious enough to step-debug into JDK classes. I have personally wasted countless days/weeks/months debugging problems that would have been solved much quicker by having full debug symbols. Furthermore, as you can see at http://mail.openjdk.java.net/pipermail/build-dev/2012-October/006892.html the GNU/Linux world also ships with full debug symbols by default. I'd like to take this opportunity to follow-up on http://mail.openjdk.java.net/pipermail/build-dev/2012-October/006881.html. I am confused because this post seems to be saying that Oracle JDK 7 ships full debug symbols, but in my experience this is not the case. It would be good to get some answers from someone in the know. Thanks, Gili
Re: Code Review Request for 8025967 addition of -Werror broke the old build
Thanks Vinnie for the review~ Forwarding this request to core-libs-dev per Sean's suggestion. Changes are for getting rid of compiler warnings in order for JCE provider build (still using the old build process) to complete successfully - either add the annotation to suppress rawtype warnings or change local declarations from Class[] to Class?[]. If there are concerns, please let me know by EOB today as I have to get JCE changes integrated before leaving for vacation. Thanks, Valerie On 10/07/13 08:41, Sean Mullan wrote: I would also send this to core-libs-dev for someone to look over the non-security component changes. --Sean On 10/04/2013 06:24 PM, Valerie (Yu-Ching) Peng wrote: Well, can someone please review the following trivial fix today or early Monday? 8025967: addition of -Werror broke the old build JCE is still using the legacy build and as a result, I have to fix build warnings in other components in order for the whole build to succeed. The changes are all about addressing the javac raw type warnings. I want to keep changes to a minimum, thus I only changed the private/implementation related ones and used annotation to disable the warnings in all public APIs and some private ones where there are dependencies, e.g. JNI. Webrev can be found at: http://cr.openjdk.java.net/~valeriep/8025967/ Thanks! Valerie
hg: jdk8/tl/jdk: 8025968: Integrate test improvements made in lambda repo
Changeset: f0ad3e2918b4 Author:henryjen Date: 2013-10-07 11:25 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f0ad3e2918b4 8025968: Integrate test improvements made in lambda repo Reviewed-by: psandoz ! test/java/util/stream/bootlib/java/util/stream/OpTestCase.java ! test/java/util/stream/test/org/openjdk/tests/java/util/stream/ExplodeOpTest.java ! test/java/util/stream/test/org/openjdk/tests/java/util/stream/TabulatorsTest.java
Re: JDK 8 RFR 8016252: More defensive HashSet.readObject
On 07/10/2013 18:40, Brian Burkhalter wrote: : I have posted an updated webrev here: http://cr.openjdk.java.net/~bpb/8016252.2/ I don't know whether it is the correct way to go, but this version attempts to use the capacity, load factor, and size as read in, insofar as they appear reasonable. This looks better. The checks on loadFactor look okay. I'm not so sure about capacity, a simple check if it is negative should be sufficient. For size then I don't think the Math.max has any real effect because the loop don't do anything if size is negative You could just throw IllegalObjectException if it is negative (as per the first patch) if you really wanted to. -Alan.
Re: JDK 8 RFR 8016252: More defensive HashSet.readObject
On Oct 7, 2013, at 1:31 PM, Alan Bateman wrote: I'm not so sure about capacity, a simple check if it is negative should be sufficient. The idea was to try to use the value if it appears reasonable, which is assured by the clamping. For size then I don't think the Math.max has any real effect because the loop don't do anything if size is negative You could just throw IllegalObjectException if it is negative (as per the first patch) if you really wanted to. It has an effect as size is used on clamping the capacity value. Brian
Re: RFR (S) CR 6857566: (bf) DirectByteBuffer garbage creation can outpace reclamation
On 10/07/2013 04:10 PM, Alan Bateman wrote:
On 06/10/2013 23:56, Peter Levart wrote:
:
It's not so much about helping to achieve better throughput (as I
noted deallocating can not be effectively parallelized) but to
overcome the latency of waking-up the ReferenceHandler thread. Here's
my attempt at doing this:
http://cr.openjdk.java.net/~plevart/jdk8-tl/DyrectBufferAlloc/webrev.01/
This is much simplified from my 1st submission of similar strategy. I
tried to be as undisruptive to current logic of Reference processing
as possible, but of course you decide if this is still too risky for
inclusion into JDK8. Cleaner is unchanged - it processes it's thunk
synchronously and ReferenceHandler thread invokes it directly.
ReferenceHandler logic is the same - I just factored-out the content
of the loop into a private method to be able to call it from nio Bits
where the bulk of change lies.
:
So what do you think? Is this still too risky for JDK8?
I looked at the latest webrev and I think the approach looks good.
I should explain that I did look into this issue about 3-4 years ago
and at the time I experimented with the allocating threads waiting
until the reference handler had drained the pending list. I didn't
think of doing the assist at the time, hence I was interested to see
the # allocations where it helped.
Ask not what reference handler can do for you, ask what you can do for
reference handler! ;-)
I first saw the idea in Clif Click's lock-free hash table. Then
recently, in the new Doug Lea's ConcurrentHashMap. This last one was
fresh and got me thinking...
On the patch then I agree with Aleksey that moving the static
initializer makes it less obvious that the only change is registering
the shared secret (it's not a big deal of course).
I'll try to make Sdiff not move it. (See the answer to Aleksey)
The back-off before retrying looks good, I just wonder if 1ms is too
low to start with.
The tests show that in majority of calls (at least on fast CPUs), thread
does not sleep at all and if it really must sleep, a single sleep(1) is
usually enough. So why sleep more? We have exponential back-off, so on
slower CPUs, where longer sleeps might be needed, a couple of iterations
more will be enough to reach the right point in time. I'll try the
measurement on Raspberry PI. I wonder how it behaves there...
Somewhere I read that the resolution of Thread.sleep() is not 1 ms, but
much more. In that case Thread.sleep(1) sleeps much more. That must have
been some time ago or on some other platform, because if I run this on
current JDK8/Linux:
for (long d = 1; d 100; d++) {
long t0 = System.nanoTime();
Thread.sleep(d);
long t1 = System.nanoTime();
System.out.println(sleep( + d + ) takes + (t1-t0) +
ns);
}
I get:
sleep(1) takes 1079078 ns
sleep(2) takes 2058245 ns
sleep(3) takes 3060258 ns
sleep(4) takes 4060121 ns
sleep(5) takes 5061263 ns
sleep(6) takes 6063189 ns
sleep(7) takes 7075132 ns
sleep(8) takes 8071381 ns
sleep(9) takes 9062244 ns
...
...which seems pretty accurate.
On the interrupt then I think it's okay to just set the interrupt
status as you are doing.
I think too.
I see you switched the tracking for the management interface to use
AtomicLong. Are you looking to improve the concurrency or is there
another reason?
With looping over tryReserveMemory and helping ReferenceHandler which
calls unreserveMemory from other threads too, the number of monitor
acquire/releases per allocation request would increase significantly if
we kept synchronized blocks. And with multiple contended threads the
overhead would increase too. So I got rid of locks, because it could be
done: There's a single accumulator used for regulating reserve/unreserve
(totalCapacity) and this can be maintained with READ-CAS-RETRY on
reserve and ATOMIC ADD on unreserve. Other values are just for
management interface, which doesn't require a snapshot view, so they
can each be maintained with ATOMIC ADD independently.
A minor coding convention but the break before else and finally is
inconsistent in these areas. Another consistency point is that
maxsleeps is a constant and so should probably be in uppercase.
I should've set-up the IDEA's Code Style correctly. Will correct this.
A related piece of work is the FileChannel map implementation where
there is a gc + retry if mmap fails. This could be changed to have a
similar back-off/retry.
I see. Would it make sense to do it in same patch or separately. This
too, will need JavaLangRefAccess.tryHandlePendingReference(), I think,
since it similarly ties unmap0 to a Cleaner referencing
MappedByteBuffer. The tryMap0 would just be a call to map0 with catching
of OOME and returning true/false, right?
Do you happen to know what defines the limit of how many bytes or blocks
can be mapped at one time? Is this some parameter for VM or is
Re: JDK 8 RFR 8016252: More defensive HashSet.readObject
On Oct 7, 2013, at 1:31 PM, Alan Bateman wrote: For size then I don't think the Math.max has any real effect because the loop don't do anything if size is negative You could just throw IllegalObjectException if it is negative (as per the first patch) if you really wanted to. On second thought an exception really should be thrown on negative size; will update. Brian
Re: JDK 8 RFR 8016252: More defensive HashSet.readObject
On Oct 7, 2013, at 1:43 PM, Brian Burkhalter wrote: On second thought an exception really should be thrown on negative size; will update. http://cr.openjdk.java.net/~bpb/8016252.2/ updated including a not-very-exciting and perhaps unnecessary test. Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 4 2013, at 13:58 , Brian Burkhalter wrote:
On Oct 3, 2013, at 5:38 PM, Brian Burkhalter wrote:
On Oct 3, 2013, at 5:35 PM, Alan Bateman wrote:
On 03/10/2013 16:10, Brian Burkhalter wrote:
Please review and comment at your convenience.
Issue: https://bugs.openjdk.java.net/browse/JDK-7179567
Webrev:http://cr.openjdk.java.net/~bpb/7179567/
An updated webrev which I hope adequately addresses the expressed concerns
may be found at:
http://cr.openjdk.java.net/~bpb/7179567.2/
Looks good to me.
Does the addition of If {@code codesource} is {@code null} the returned {@code
PermissionCollection} is empty. constitute a spec change or just a
clarification? I see the URClassLoader change @@ -625,10 +661,14 @@ but am
unsure.
Mike
Will you be adding tests for these cases to the webrev?
As needed once the concept in general is accepted.
The foregoing webrev includes a test of the affected public methods.
Thanks,
Brian
Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs
Hello, I skimmed the patch and it looked fine. More generally, we want every package and top-level class in the com.sun.* namespace to be either explicitly exported or not. Cheers, -Joe On 10/6/2013 1:03 PM, Alan Bateman wrote: As a follow-up to Joe Darcy's rename of jdk.Supported to jdk.Exported, I'd like to have another attempt at adding the annotation to a number of JDK specific APIs that are long standing exported, documented and supported APIs. Specifically, the following APIs: - Java Debug Interface (com.sun.jdi) - Attach API (com.sun.tools.attach) - SCTP API (com.sun.nio.sctp) - HTTP server API (com.sun.net.httpserver) - Management extensions (com.sun.management) - JConsole Plugin API (com.sun.tools.jconsole) - JDK-specific API to JAAS (com.sun.security.auth) - JDK-specific JGSS API (com.sun.security.jgss) (The javadoc for each of these APIs is currently generated in the build) The webrev with the proposed update is here: http://cr.openjdk.java.net/~alanb/8008662/webrev.02/ As per the original webrev, I've added package-info.java to a number of packages that didn't have any description. In a few cases, I've had to rename the legacy package.html to package-info.java. For the review then the intention is that @jdk.Exported be added to the package-info and all public/protected types in these APIs. The only exceptions are two cases where I've added @jdk.Exported(false), specifically: - com.sun.management.OSMBeanFactory as it clearly documents to stay away - com.sun.security.auth.callback.DialogCallbackHandler as it for the chop (see JEP 162) Thanks, Alan.
RFR: 8026009: Changes for 8025968 break all stream tests
Hi,
May I have a quick review on this left-out change to fix broken test?
Apology for the inconvenience.
http://cr.openjdk.java.net/~henryjen/tl/8026009/0/webrev/
Following is all it is,
diff --git a/test/java/util/stream/bootlib/java/util/stream/OpTestCase.java
b/test/java/util/stream/bootlib/java/util/stream/OpTestCase.java
--- a/test/java/util/stream/bootlib/java/util/stream/OpTestCase.java
+++ b/test/java/util/stream/bootlib/java/util/stream/OpTestCase.java
@@ -591,10 +591,10 @@
// Test data
-private class ShortCircuitOpT implements StatelessTestOpT,T {
+static class ShortCircuitOpT implements StatelessTestOpT,T {
private final StreamShape shape;
-private ShortCircuitOp(StreamShape shape) {
+ShortCircuitOp(StreamShape shape) {
this.shape = shape;
}
Cheers,
Henry
Re: RFR: 8026009: Changes for 8025968 break all stream tests
Looks good to me but I haven't completed a full build/test with this change
(yet).
Mike
On Oct 7 2013, at 14:56 , Henry Jen wrote:
Hi,
May I have a quick review on this left-out change to fix broken test?
Apology for the inconvenience.
http://cr.openjdk.java.net/~henryjen/tl/8026009/0/webrev/
Following is all it is,
diff --git a/test/java/util/stream/bootlib/java/util/stream/OpTestCase.java
b/test/java/util/stream/bootlib/java/util/stream/OpTestCase.java
--- a/test/java/util/stream/bootlib/java/util/stream/OpTestCase.java
+++ b/test/java/util/stream/bootlib/java/util/stream/OpTestCase.java
@@ -591,10 +591,10 @@
// Test data
-private class ShortCircuitOpT implements StatelessTestOpT,T {
+static class ShortCircuitOpT implements StatelessTestOpT,T {
private final StreamShape shape;
-private ShortCircuitOp(StreamShape shape) {
+ShortCircuitOp(StreamShape shape) {
this.shape = shape;
}
Cheers,
Henry
Re: RFC 6910473: BigInteger negative bit length, value range, and future prospects
Without comments on the contents of the patch, a change in documented
behavior would require a ccc request.
Cheers,
-Joe
On 10/3/2013 5:58 PM, Brian Burkhalter wrote:
I have reviewed this proposed change a couple of times in its current form and
it looks good to me.
It would be good to see some comments about the general concept from BigInt
cognoscenti, and from (a) Reviewer(s) as concerns the @implNote addition as
well as the new ArithmeticExceptions added at several points in the javadoc.
With respect to these latter, in the event the patch were to be approved, would
a CCC request be in order?
Brian
On Oct 1, 2013, at 7:50 PM, cowwoc wrote:
Sounds good. Thanks for the clarification.
Gili
On 01/10/2013 9:25 PM, Dmitry Nadezhin wrote:
I see that I misused the word to clamp in this discussion.
I guess that addition with clumping means:
return x + y MIN_VALUE ? MIN_VALUE : x + y MAX_VALUE ? MAX_VALUE : x +
y;
The patch actually throws ArithmeticException on overflow:
if (x + y MIN_VALUE || x + y MAX_VALUE) throw new ArithmetiException();
else return x + y;
The word clamp appeared in the discussion only.
Comments in the patch don't contain it. They say:
BigInteger constructors and operations throw {@code
ArithmeticException} whenthe result is out of the supported range.
On Tue, Oct 1, 2013 at 11:45 PM, cowwoc cow...@bbs.darktech.org wrote:
I prefer throwing exceptions on unusual conditions (e.g. overflow) and
letting the user clamp the value if they so wish. Clamping will lead to
unexpected behavior once values fall outside this range. Yes, it will be
documented, but I daresay most applications won't ever check for it and
produce incorrect results as a consequence.
Gili
On 01/10/2013 2:19 PM, Dmitry Nadezhin wrote:
Dear BigInteger experts,
Do you have comments to my previous message ?
http://mail.openjdk.java.net/**pipermail/core-libs-dev/2013-**
September/021264.htmlhttp://mail.openjdk.java.net/pipermail/core-libs-dev/2013-September/021264.html
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 7, 2013, at 2:47 PM, Mike Duigou wrote:
An updated webrev which I hope adequately addresses the expressed concerns
may be found at:
http://cr.openjdk.java.net/~bpb/7179567.2/
Looks good to me.
Does the addition of If {@code codesource} is {@code null} the returned
{@code PermissionCollection} is empty. constitute a spec change or just a
clarification? I see the URClassLoader change @@ -625,10 +661,14 @@ but am
unsure.
To me it's a clarification but it would be helpful for someone else to weight
in on the question.
Thanks,
Brian
Re: RFC 6910473: BigInteger negative bit length, value range, and future prospects
I would expect as much, but that's an exercise I'd prefer to avoid at this time if the content does not appear likely to be acceptable. Thanks, Brian On Oct 7, 2013, at 3:26 PM, Joseph Darcy wrote: Without comments on the contents of the patch, a change in documented behavior would require a ccc request.
Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown
Resuming this discussion … Thanks for the previous comments. My feeling at this point is to do one of two things: A) defer to something after JDK 8, or B) on EAI_AGAIN do not retry but set the cause of the UAE to new SomeException(gai_strerror(error)) where SomeException could be for example Exception, RuntimeException, or IOException. Comments? Thanks, Brian
hg: jdk8/tl/langtools: 8026017: Make history of AnnotatedConstruct methods in jx.l.m.e.Element clearer
Changeset: 4dd7ffbf01fb Author:darcy Date: 2013-10-07 16:51 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/4dd7ffbf01fb 8026017: Make history of AnnotatedConstruct methods in jx.l.m.e.Element clearer Reviewed-by: jjg ! src/share/classes/javax/lang/model/element/Element.java
hg: jdk8/tl/jdk: 8026009: Changes for 8025968 break all stream tests
Changeset: 0cffe1dab0bf Author:henryjen Date: 2013-10-07 15:18 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0cffe1dab0bf 8026009: Changes for 8025968 break all stream tests Reviewed-by: mduigou ! test/java/util/stream/bootlib/java/util/stream/OpTestCase.java
Re: Review Request for 8025799: Restore sun.reflect.Reflection.getCallerClass(int)
On 10/7/2013 11:34 AM, Christian Thalinger wrote: Unfortunate but I understand. It might be a good idea to add a getCallerClass(-1) call to the test case. The VM should throw an exception if JVM_GetCallerClass is called by any method other than Reflection.getCallerClass(). It's a good idea to add such test case. thanks Mandy On Oct 7, 2013, at 1:24 AM, Mandy Chung mandy.ch...@oracle.com wrote: JDK 8 was feature complete in June and there just isn't sufficient time remaining to get agreement and feedback on an API to examine the caller frames. To that end, I propose to restore the old unsupported Reflection.getCallerClass(int) and that we will look to define a standard API for JDK 9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8025799/ It remains to be an unsupported API and JDK should not use this method and it's not annotated with @CallerSensitive. I considered detecting if this method is called by a system class (loaded by null loader) and throw an error. I decided to minimize the compatibility risk in case if there is any existing code added to the bootclasspath depending on this private API. Thanks Mandy
Re: RFR: 8024688: j.u.Map.merge doesn't work as specified if contains key:null pair
On Oct 3 2013, at 22:15 , David Holmes wrote: On 4/10/2013 1:35 PM, Mike Duigou wrote: Hello all; This is a changeset which improves the consistency of several Map.merge implementations for handling of null values. It isn't at all clear to me what specification you are using to define the expected behaviour here. Most of the new Map methods provide only limited support for null values. This is not due to null-hostility but generally a consequence of trying to avoid the additional overhead of disambiguating between value absent and null value. This includes the result of get() and null returns are also used from the functions to provide special behaviour (usually removal). The merge() spec is typical in that it promises to treat null and absent values the same. Some might suggest this is a cunning plot by Doug to discourage use of null values. Perhaps. ;-) The utility of being able to use null values as sentinels, whether from get() or the use function is primary reason for this behaviour. Making the behaviour of the Map defaults and core collections not horrible and not astonishing in the presence null values has been a fun challenge. :-) It's sometimes necessitated providing alternative defaults for ConcurrentMap. I believe that the defaults in Map and ConcurrentMap could further diverge over time if it turns out to be a problem that the current Map defaults don't look aggressively enough for concurrent modification. I would have thought that anyone supplying a remapping function needs to be aware of whether the target map supports nulls or not, and that the remapping function should then do the right thing if null is encountered. Instead you are making the decision to bypass the remapping function if you encounter a null. Bypassing the function is what merge() does when there's no existing values. This behaviour is not specific to my changes. Here's the logic table I used to construct the tests: current value merged put/rem returned === = == === absent nulln/a remove null absent newval n/a newval newval nullnulln/a remove null nullnewval n/a newval newval oldval nullnullremove null oldval nullresult result result oldval newval nullremove null oldval newval result result result Alternatively, compute() offers the more general behaviour of always calling the remapping function though it does not offer the opportunity to directly provide a newValue (capture by the lambda is the alternative). If I had designed the API I would have had only a single method that provided newValue and unconditionally called the function. This is probably would have been naive--I don't know Doug's reasons behind choosing this behaviour for merge() and compute() though I am certainly curious about why it is done the way it is. Mike David The existing unit tests hadn't considered several cases where the result of the remapper was not the same as the value. I've restructured the merge tests to be more thorough and systematic this revealed a couple of problems. http://cr.openjdk.java.net/~mduigou/JDK-8024688/0/webrev/ Like several of the previous patches, this one introduces an alternative default for ConcurrentMap to work around issues involving null values where the handling in the general Map default would be incorrect. Mike
hg: jdk8/tl/jdk: 6956398: make ephemeral DH key match the length of the certificate key
Changeset: 0d5f4f1782e8 Author:xuelei Date: 2013-10-07 18:46 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0d5f4f1782e8 6956398: make ephemeral DH key match the length of the certificate key Reviewed-by: weijun ! src/share/classes/sun/security/ssl/ServerHandshaker.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/DHKeyExchange/DHEKeySizing.java
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
Hi Brian,
Aside: I'm confused about the relationship of this bug and JDK-6445180 -
are they not duplicates? Seems to me this one should have been closed as
a dup when it was reported.
On 5/10/2013 6:58 AM, Brian Burkhalter wrote:
On Oct 3, 2013, at 5:38 PM, Brian Burkhalter wrote:
On Oct 3, 2013, at 5:35 PM, Alan Bateman wrote:
On 03/10/2013 16:10, Brian Burkhalter wrote:
Please review and comment at your convenience.
Issue: https://bugs.openjdk.java.net/browse/JDK-7179567
Webrev: http://cr.openjdk.java.net/~bpb/7179567/
An updated webrev which I hope adequately addresses the expressed concerns may
be found at:
http://cr.openjdk.java.net/~bpb/7179567.2/
URLClassLoader.java: please delete the commented out line:
+ //Objects.requireNonNull(urls);
SecureClassLoader.java:
186 * @throws SecurityException if the {@code ClassLoader} instance
is not
187 * initialized.
should read this classloader instance as it refers to the current
instance. Also this may be bringing the spec into line with the
implementation but initialization here is purely an implementation
detail not part of the specification for this class so it should not be
mentioned explicitly - and this change still needs a CCC (which might
help determine exactly what should be said here - I'm unclear how you
can try to call getPermissions if this is uninitialized as it would need
'this' to escape from the constructor - which perhaps it can do via a
third-party security manager?).
NoCallStackClassLoader.java: the comment is far too verbose, we don't
write such explanatory kinds of comments for this kind of thing (else
the code would be overrun with commentary!).
Aside: if you are a JDK Author you don't need a Contributed-by line in
the commit comments.
David
-
Will you be adding tests for these cases to the webrev?
As needed once the concept in general is accepted.
The foregoing webrev includes a test of the affected public methods.
Thanks,
Brian
Re: Review Request for 8025799: Restore sun.reflect.Reflection.getCallerClass(int)
Hi Mandy, Note that unless you push both hotspot and jdk changes through the same forest you will need separate bugs for each part. You will also need a HSX committer to do the hotspot push. David On 7/10/2013 6:24 PM, Mandy Chung wrote: JDK 8 was feature complete in June and there just isn't sufficient time remaining to get agreement and feedback on an API to examine the caller frames. To that end, I propose to restore the old unsupported Reflection.getCallerClass(int) and that we will look to define a standard API for JDK 9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8025799/ It remains to be an unsupported API and JDK should not use this method and it's not annotated with @CallerSensitive. I considered detecting if this method is called by a system class (loaded by null loader) and throw an error. I decided to minimize the compatibility risk in case if there is any existing code added to the bootclasspath depending on this private API. Thanks Mandy
Re: Review Request for 8025799: Restore sun.reflect.Reflection.getCallerClass(int)
On 10/7/2013 9:45 PM, David Holmes wrote: Hi Mandy, Note that unless you push both hotspot and jdk changes through the same forest you will need separate bugs for each part. You will also need a HSX committer to do the hotspot push. I do plan to push them separately meaning that the jdk change will wait until the hotspot change is integrated. I think the hotspot integration is weekly. I am not a HSX committer :( Since I have your attention, can you sponsor my hotspot change? Mandy David On 7/10/2013 6:24 PM, Mandy Chung wrote: JDK 8 was feature complete in June and there just isn't sufficient time remaining to get agreement and feedback on an API to examine the caller frames. To that end, I propose to restore the old unsupported Reflection.getCallerClass(int) and that we will look to define a standard API for JDK 9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8025799/ It remains to be an unsupported API and JDK should not use this method and it's not annotated with @CallerSensitive. I considered detecting if this method is called by a system class (loaded by null loader) and throw an error. I decided to minimize the compatibility risk in case if there is any existing code added to the bootclasspath depending on this private API. Thanks Mandy
Re: RFR (S) CR 6857566: (bf) DirectByteBuffer garbage creation can outpace reclamation
On 8/10/2013 6:36 AM, Peter Levart wrote:
Somewhere I read that the resolution of Thread.sleep() is not 1 ms, but
much more. In that case Thread.sleep(1) sleeps much more. That must have
been some time ago or on some other platform, because if I run this on
current JDK8/Linux:
for (long d = 1; d 100; d++) {
long t0 = System.nanoTime();
Thread.sleep(d);
long t1 = System.nanoTime();
System.out.println(sleep( + d + ) takes + (t1-t0) +
ns);
}
I get:
sleep(1) takes 1079078 ns
sleep(2) takes 2058245 ns
sleep(3) takes 3060258 ns
sleep(4) takes 4060121 ns
sleep(5) takes 5061263 ns
sleep(6) takes 6063189 ns
sleep(7) takes 7075132 ns
sleep(8) takes 8071381 ns
sleep(9) takes 9062244 ns
...
...which seems pretty accurate.
It is platform specific. Solaris 10 will give 10ms by default. Windows
will give anywhere from 5 to 60ms on my laptop :) and is Windows version
specific. Even Linux may depend on system config and what clocksource is
used.
David
On the interrupt then I think it's okay to just set the interrupt
status as you are doing.
I think too.
I see you switched the tracking for the management interface to use
AtomicLong. Are you looking to improve the concurrency or is there
another reason?
With looping over tryReserveMemory and helping ReferenceHandler which
calls unreserveMemory from other threads too, the number of monitor
acquire/releases per allocation request would increase significantly if
we kept synchronized blocks. And with multiple contended threads the
overhead would increase too. So I got rid of locks, because it could be
done: There's a single accumulator used for regulating reserve/unreserve
(totalCapacity) and this can be maintained with READ-CAS-RETRY on
reserve and ATOMIC ADD on unreserve. Other values are just for
management interface, which doesn't require a snapshot view, so they
can each be maintained with ATOMIC ADD independently.
A minor coding convention but the break before else and finally is
inconsistent in these areas. Another consistency point is that
maxsleeps is a constant and so should probably be in uppercase.
I should've set-up the IDEA's Code Style correctly. Will correct this.
A related piece of work is the FileChannel map implementation where
there is a gc + retry if mmap fails. This could be changed to have a
similar back-off/retry.
I see. Would it make sense to do it in same patch or separately. This
too, will need JavaLangRefAccess.tryHandlePendingReference(), I think,
since it similarly ties unmap0 to a Cleaner referencing
MappedByteBuffer. The tryMap0 would just be a call to map0 with catching
of OOME and returning true/false, right?
Do you happen to know what defines the limit of how many bytes or blocks
can be mapped at one time? Is this some parameter for VM or is this just
plain OS limit?
On the test then the copyright date is 2001-2007 so I assume this was
copied from somewhere :-) I agree with Aleksey on the test duration,
especially if you can provoke OOME in less than 10 or 20 seconds on
some machines.
Right.
As regards whether this should go into JDK 8 then the updated proposal
is significantly less risky that the original proposal that changed
the implementation to use weak references.
That said, this is a 13 year old issue that hasn't come up very often
(to my knowledge anyway, perhaps because those making heavy use of
direct buffers are pooling buffers rather than allocating and
unreferencing). In additional we are close to the end of JDK 8 (ZBB is
in 2.5 weeks time) and technically we have been in ramp down (meaning
P1-P3 only) since mid-July.
Ok then, I'll finish this nevertheless and then it can sit and wait for
JDK9.
Regards, Peter
-Alan.
