JDK 9 RFR of JDK-8075304: Remove duplicate test: FDTest
This RFR proposes to remove the duplicate FDTest from “jdk” repo.
FDTest exists in both “jdk” and “langtools”:
http://hg.openjdk.java.net/jdk9/dev/jdk/file/tip/test/jdk/lambda/FDTest.java
http://hg.openjdk.java.net/jdk9/dev/langtools/file/tip/test/tools/javac/lambdaShapes/org/openjdk/tests/javac/FDTest.java
So far the only diff between the two tests is that the version in
langtools has one more teardown method to well handle the resource close
@AfterSuite
static void teardown() throws IOException {
fm.close();
}
As this test is for JavaCompiler, it makes more sense to keep the test
in langtools and remove the duplicate one from jdk”.
bug: https://bugs.openjdk.java.net/browse/JDK-8075304
webrev: http://cr.openjdk.java.net/~amlu/8075304/webrev.00/
Thanks,
Amy
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 16/03/15 19:16, John Rose wrote: On Mar 16, 2015, at 2:29 AM, Andrew Haley a...@redhat.com wrote: Since the option provides control for product behavior, without an explicit opt-in, it should either be a product flag or a diagnostic flag. I suggest keeping the more direct name (Use* not Disable*) and making it a diagnostic flag. OK. Andrew.
Re: RFR JDK-8074678: JCK test api/java_util/regex/MatchResult/index.html starts failing after JDK-8071479
On Mar 16, 2015, at 11:00 PM, Xueming Shen xueming.s...@oracle.com wrote: Please help review the change for issue: https://bugs.openjdk.java.net/browse/JDK-8074678 webrev: http://cr.openjdk.java.net/~sherman/8074678 The proposed fix is to implement the same whether or not there is a match sanity check in ImmutableMatchResult. +1 Paul.
Re: RFR [9] 8071472: Add field access to support setting final fields in readObject
Peter, Alan, After further thought I think that requiring all final fields to be set is reasonable behaviour. Since there is no compile time checking, this is a reasonable runtime effort to catch what is likely to be developer errors. To address Alan’s comments, I beefed up the API docs and added examples to typical usage. Updated specdiff (and webrev): http://cr.openjdk.java.net/~chegar/8071472/01/ http://cr.openjdk.java.net/~chegar/8071472/01/ This version also includes a number of changes to readObject implementations in the base module, to replace unsafe usage with this field setter API. -Chris. On 13 Mar 2015, at 19:36, Chris Hegarty chris.hega...@oracle.com wrote: On 13 Mar 2015, at 17:58, Peter Levart peter.lev...@gmail.com wrote: On 03/12/2015 11:24 PM, Peter Levart wrote: So if a readObject calls fields() without calling defaultReadObject() then it has to set every final field. On one hand that ensures that every final field is set, on the other hand it is a bit odd because omitting the call to fields() means they all get their default value. If fields() is not called, we must be backwards-compatible. But yes, this constraint is questionable. On one hand it tries to mimic the assignment to final fields in constructors, but it can't go all the way, as read access to final fields in readObject() is not limited to just those that have already been assigned (like it is in constructor with definitive assignment rules). We could add get() methods to FieldAccess that would constraint the reads of final fields to those that have already been assigned, but that is just like advisory locking - we can only advise users to use FieldAccess to read fields in readObject() but we can't prevent them from reading them directly. So if this doesn't have much sense and brings confusion, it can go away. ...or it can stay in part where we check that a final field is not set more than once, which can help especially when use of FieldAccess API is combined with defaultReadObject(). Yes, that makes sense. The final check that all finals have been assigned can be made optional by an explicit call to a method (FieldAccess.checkAllFinalsSet() for example). If possible, I’d rather not have any additional methods exposed on FieldSetter, other than the overloaded sets. Let see how this works if we keep it as minimal as possible. I’m going to take a run over all readObjects in the base module that use unsafe or reflection, and rewrite to use this API. -Chris. Regards, Peter
Re: RFR [9] 8071472: Add field access to support setting final fields in readObject
On 17/03/2015 09:57, Chris Hegarty wrote: Peter, Alan, After further thought I think that requiring all final fields to be set is reasonable behaviour. Since there is no compile time checking, this is a reasonable runtime effort to catch what is likely to be developer errors. To address Alan’s comments, I beefed up the API docs and added examples to typical usage. Updated specdiff (and webrev): http://cr.openjdk.java.net/~chegar/8071472/01/ http://cr.openjdk.java.net/%7Echegar/8071472/01/ This version also includes a number of changes to readObject implementations in the base module, to replace unsafe usage with this field setter API. The rename to FieldSetter looks good, also good to have an example in the javadoc. It is good to catch cases where final fields aren't set but the issue is the surprising behavior that the check is tied to whether the FieldSetter has been obtained or not. The other thing is that fieldSetter's javadoc doesn't make this clear, you need to read FieldSetter's javadoc to learn about this enforcement. It's hard to know what the right thing to do here as ideally this enforcement would be enabled by default. If my class with final fields has an empty readObject, or it has code paths that don't call defaultReadObject for example, then it's a bug that I'd like to know about. I realize there is potential compatibility, maybe performance, impact of doing this but it does feel like something that I should be able to opt-in or get for free rather than it being a side effect. One other thing that I wonder about is the exception and whether it might be better to bend InvalidObjectException instead. -Alan
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On Mar 16, 2015, at 6:19 PM, Andrew Haley a...@redhat.com wrote: On 03/16/2015 10:03 AM, Paul Sandoz wrote: I am running this patch though our JPRT test system right now. The test looks good, two comments on it: - IMO it does not need the internal PRNG FastRandom, SplittableRandom can be used instead. - I suspect that test needs to be located in the hotspot test area, which likely means it gets run on more exotic platforms. Perhaps someone with more knowledge of the test configuration and infrastructure can comment? I have a patch with all of the changes that people have asked for except this last one. I don't know who to ask. Including Stefan, who may know more. Paul.
Re: RFR(XS): 8075071: [TEST_BUG] TimSortStackSize2.java: OOME: Java heap space: MaxHeap shrinked by MaxRAMFraction
Hi Lev, On 18/03/2015 9:07 AM, Lev Priima wrote: Possible, by determining of disabled UseCompressedOops option and forking vm again with required minimum heap value(-Xms). Please review update: http://cr.openjdk.java.net/~lpriima/8075071/1/webrev/ Yes that is the approach that is needed. But didn't you need to set Xmx not Xms to deal with the MaxRAMFraction problem? Thanks, David Lev On 03/17/2015 03:23 PM, David Holmes wrote: So this is a problem that needs to be resolved. The memory requirements of the test are platform dependent but can't be satisfied on all platforms. Though I am surprised that 770M is needed given that two incarnations ago we set -Xmx to 385M! But it is appearing to be impossible to run this test in a way that can work on all platforms. David On 17/03/2015 10:15 PM, Lev Priima wrote: Yes. Lev On 03/17/2015 02:56 PM, David Holmes wrote: On 17/03/2015 9:15 PM, Lev Priima wrote: Hi David, Explicit -Xmx does not allow explicit MaxRAMFraction to shrink MaxHeapSize down to -Xms(or even less down to default InitialHeapSize in case if -Xms also wasn't set explicitly). In other words explicit -Xmx has priority over explicit MaxRAMFraction if both are set and contradict with each other. Okay - that's good to know. However the -Xmx770m is a problem - our small devices only have 512MB of memory. Is the 770M only needed for 64-bit with -UseCompressedOops ? Thanks, David Lev On 03/17/2015 07:40 AM, David Holmes wrote: Hi Lev, On 17/03/2015 6:30 AM, Lev Priima wrote: Please reviewand push: Issue:https://bugs.openjdk.java.net/browse/JDK-8075071 Patch: http://cr.openjdk.java.net/~lpriima/8075071/0/webrev/ Tested locally: jtreg -vmoptions:-XX:MaxRAMFraction=9223372036854775807 -XX:-UseCompressedOops test/java/util/Arrays/TimSortStackSize2.java Test results: passed: 1 How does MaxRAMFraction interact with -Xmx? Thanks, David Lev
Re: RFR(XS): 8075071: [TEST_BUG] TimSortStackSize2.java: OOME: Java heap space: MaxHeap shrinked by MaxRAMFraction
Possible, by determining of disabled UseCompressedOops option and forking vm again with required minimum heap value(-Xms). Please review update: http://cr.openjdk.java.net/~lpriima/8075071/1/webrev/ Lev On 03/17/2015 03:23 PM, David Holmes wrote: So this is a problem that needs to be resolved. The memory requirements of the test are platform dependent but can't be satisfied on all platforms. Though I am surprised that 770M is needed given that two incarnations ago we set -Xmx to 385M! But it is appearing to be impossible to run this test in a way that can work on all platforms. David On 17/03/2015 10:15 PM, Lev Priima wrote: Yes. Lev On 03/17/2015 02:56 PM, David Holmes wrote: On 17/03/2015 9:15 PM, Lev Priima wrote: Hi David, Explicit -Xmx does not allow explicit MaxRAMFraction to shrink MaxHeapSize down to -Xms(or even less down to default InitialHeapSize in case if -Xms also wasn't set explicitly). In other words explicit -Xmx has priority over explicit MaxRAMFraction if both are set and contradict with each other. Okay - that's good to know. However the -Xmx770m is a problem - our small devices only have 512MB of memory. Is the 770M only needed for 64-bit with -UseCompressedOops ? Thanks, David Lev On 03/17/2015 07:40 AM, David Holmes wrote: Hi Lev, On 17/03/2015 6:30 AM, Lev Priima wrote: Please reviewand push: Issue:https://bugs.openjdk.java.net/browse/JDK-8075071 Patch: http://cr.openjdk.java.net/~lpriima/8075071/0/webrev/ Tested locally: jtreg -vmoptions:-XX:MaxRAMFraction=9223372036854775807 -XX:-UseCompressedOops test/java/util/Arrays/TimSortStackSize2.java Test results: passed: 1 How does MaxRAMFraction interact with -Xmx? Thanks, David Lev
[9] RFR of 8075362: j.u.Properties.load() methods have misaligned @throws clauses
Follow-on to correct some insufficiencies pointed out in
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-March/032289.html
Please review at your convenience.
Issue: https://bugs.openjdk.java.net/browse/JDK-8075362
Diff: See below (Properties.java change did not show up in webrev).
Thanks,
Brian
diff --git a/src/java.base/share/classes/java/util/Properties.java
b/src/java.base/share/classes/java/util/Properties.java
--- a/src/java.base/share/classes/java/util/Properties.java
+++ b/src/java.base/share/classes/java/util/Properties.java
@@ -309,7 +309,7 @@
* input stream.
* @throws IllegalArgumentException if a malformed Unicode escape
* appears in the input.
- * @throws NullPointerException if {@code reader} is null.
+ * @throws NullPointerException if {@code reader} is null.
* @since 1.6
*/
public synchronized void load(Reader reader) throws IOException {
@@ -335,7 +335,7 @@
* input stream.
* @throws IllegalArgumentException if the input stream contains a
* malformed Unicode escape sequence.
- * @throws NullPointerException if {@code inStream} is null.
+ * @throws NullPointerException if {@code inStream} is null.
* @since 1.2
*/
public synchronized void load(InputStream inStream) throws IOException {
diff --git a/test/java/util/Properties/Basic.java
b/test/java/util/Properties/LoadAndStoreNPE.java
rename from test/java/util/Properties/Basic.java
rename to test/java/util/Properties/LoadAndStoreNPE.java
--- a/test/java/util/Properties/Basic.java
+++ b/test/java/util/Properties/LoadAndStoreNPE.java
@@ -28,10 +28,10 @@
/*
* @test
- * @bug 8073214
- * @summary Basic tests of Properties methods.
+ * @bug 8073214 8075362
+ * @summary Tests to verify that load() and store() throw NPEs as advertised.
*/
-public class Basic
+public class LoadAndStoreNPE
{
public static void main(String[] args) throws Exception
{
@@ -68,7 +68,7 @@
}
if (failures != 0) {
-throw new RuntimeException(Basic failed with
+throw new RuntimeException(LoadAndStoreNPE failed with
+ failures + errors!);
}
}
Re: RFR(XS): 8075071: [TEST_BUG] TimSortStackSize2.java: OOME: Java heap space: MaxHeap shrinked by MaxRAMFraction
Hi David, Explicit -Xmx does not allow explicit MaxRAMFraction to shrink MaxHeapSize down to -Xms(or even less down to default InitialHeapSize in case if -Xms also wasn't set explicitly). In other words explicit -Xmx has priority over explicit MaxRAMFraction if both are set and contradict with each other. Lev On 03/17/2015 07:40 AM, David Holmes wrote: Hi Lev, On 17/03/2015 6:30 AM, Lev Priima wrote: Please reviewand push: Issue:https://bugs.openjdk.java.net/browse/JDK-8075071 Patch: http://cr.openjdk.java.net/~lpriima/8075071/0/webrev/ Tested locally: jtreg -vmoptions:-XX:MaxRAMFraction=9223372036854775807 -XX:-UseCompressedOops test/java/util/Arrays/TimSortStackSize2.java Test results: passed: 1 How does MaxRAMFraction interact with -Xmx? Thanks, David Lev
Re: RFR [9] 8071472: Add field access to support setting final fields in readObject
On 03/17/2015 11:39 AM, Alan Bateman wrote: On 17/03/2015 09:57, Chris Hegarty wrote: Peter, Alan, After further thought I think that requiring all final fields to be set is reasonable behaviour. Since there is no compile time checking, this is a reasonable runtime effort to catch what is likely to be developer errors. To address Alan’s comments, I beefed up the API docs and added examples to typical usage. Updated specdiff (and webrev): http://cr.openjdk.java.net/~chegar/8071472/01/ http://cr.openjdk.java.net/%7Echegar/8071472/01/ This version also includes a number of changes to readObject implementations in the base module, to replace unsafe usage with this field setter API. The rename to FieldSetter looks good, also good to have an example in the javadoc. It is good to catch cases where final fields aren't set but the issue is the surprising behavior that the check is tied to whether the FieldSetter has been obtained or not. The other thing is that fieldSetter's javadoc doesn't make this clear, you need to read FieldSetter's javadoc to learn about this enforcement. It's hard to know what the right thing to do here as ideally this enforcement would be enabled by default. If my class with final fields has an empty readObject, or it has code paths that don't call defaultReadObject for example, then it's a bug that I'd like to know about. I realize there is potential compatibility, maybe performance, impact of doing this but it does feel like something that I should be able to opt-in or get for free rather than it being a side effect. Hi Alan, I agree that not calling defaultReadObject() from readObject() and having a final field is potentially a bug. But need not be in case some other means of setting final fields was used (Unsafe or reflection). Some readObject() implementations in base module that Chris changed to use new API fall into this category. We can't track those usages, so to keep backwards compatibility, this checking has to be opt-in. Is there a more elegant way to opt-in? A @CheckFinalsAssignment annotation on the readObject() method? Regards, Peter One other thing that I wonder about is the exception and whether it might be better to bend InvalidObjectException instead. -Alan
Re: RFR(XS): 8075071: [TEST_BUG] TimSortStackSize2.java: OOME: Java heap space: MaxHeap shrinked by MaxRAMFraction
On 17/03/2015 9:15 PM, Lev Priima wrote: Hi David, Explicit -Xmx does not allow explicit MaxRAMFraction to shrink MaxHeapSize down to -Xms(or even less down to default InitialHeapSize in case if -Xms also wasn't set explicitly). In other words explicit -Xmx has priority over explicit MaxRAMFraction if both are set and contradict with each other. Okay - that's good to know. However the -Xmx770m is a problem - our small devices only have 512MB of memory. Is the 770M only needed for 64-bit with -UseCompressedOops ? Thanks, David Lev On 03/17/2015 07:40 AM, David Holmes wrote: Hi Lev, On 17/03/2015 6:30 AM, Lev Priima wrote: Please reviewand push: Issue:https://bugs.openjdk.java.net/browse/JDK-8075071 Patch: http://cr.openjdk.java.net/~lpriima/8075071/0/webrev/ Tested locally: jtreg -vmoptions:-XX:MaxRAMFraction=9223372036854775807 -XX:-UseCompressedOops test/java/util/Arrays/TimSortStackSize2.java Test results: passed: 1 How does MaxRAMFraction interact with -Xmx? Thanks, David Lev
Re: RFR [9] 8071472: Add field access to support setting final fields in readObject
On 03/17/2015 10:57 AM, Chris Hegarty wrote: Peter, Alan, After further thought I think that requiring all final fields to be set is reasonable behaviour. Since there is no compile time checking, this is a reasonable runtime effort to catch what is likely to be developer errors. To address Alan’s comments, I beefed up the API docs and added examples to typical usage. Updated specdiff (and webrev): http://cr.openjdk.java.net/~chegar/8071472/01/ http://cr.openjdk.java.net/~chegar/8071472/01/ This version also includes a number of changes to readObject implementations in the base module, to replace unsafe usage with this field setter API. Hi Chris, It's good that you included changes to some readObject() implementations in base module as these are the 1st real-world examples of API use. This way we can get some experience as to whether the API is adequate. This experience includes subtleties of assignment checking on finals. I think it would be desirable that if this API is included in JDK9, more migration from Unsafe to this API is gradually attempted before the API is frozen for JDK9. As for the changes, I went through them and I think they are OK. Just one nit: private static FieldSetterContext.finalInstanceFieldCount() helper method could be moved to inside the FieldSetterContext.FieldsMap nested class as it is only used from it's constructor. This way you avoid generating synthetic access methods. Also in ObjectInputStream.fieldSetter() javadoc, in the following statement: Returns the |FieldSetter| instance associated with current object and type being deserialized, which gives write access to the *types* declared instance fields, including final, during the |readObject| callback. I think it should be changed from: types - type's (or class' for that matter as only classes can have instance fields). Regards, Peter -Chris. On 13 Mar 2015, at 19:36, Chris Hegarty chris.hega...@oracle.com wrote: On 13 Mar 2015, at 17:58, Peter Levart peter.lev...@gmail.com wrote: On 03/12/2015 11:24 PM, Peter Levart wrote: So if a readObject calls fields() without calling defaultReadObject() then it has to set every final field. On one hand that ensures that every final field is set, on the other hand it is a bit odd because omitting the call to fields() means they all get their default value. If fields() is not called, we must be backwards-compatible. But yes, this constraint is questionable. On one hand it tries to mimic the assignment to final fields in constructors, but it can't go all the way, as read access to final fields in readObject() is not limited to just those that have already been assigned (like it is in constructor with definitive assignment rules). We could add get() methods to FieldAccess that would constraint the reads of final fields to those that have already been assigned, but that is just like advisory locking - we can only advise users to use FieldAccess to read fields in readObject() but we can't prevent them from reading them directly. So if this doesn't have much sense and brings confusion, it can go away. ...or it can stay in part where we check that a final field is not set more than once, which can help especially when use of FieldAccess API is combined with defaultReadObject(). Yes, that makes sense. The final check that all finals have been assigned can be made optional by an explicit call to a method (FieldAccess.checkAllFinalsSet() for example). If possible, I’d rather not have any additional methods exposed on FieldSetter, other than the overloaded sets. Let see how this works if we keep it as minimal as possible. I’m going to take a run over all readObjects in the base module that use unsafe or reflection, and rewrite to use this API. -Chris. Regards, Peter
Re: RFR(XS): 8075071: [TEST_BUG] TimSortStackSize2.java: OOME: Java heap space: MaxHeap shrinked by MaxRAMFraction
Yes. Lev On 03/17/2015 02:56 PM, David Holmes wrote: On 17/03/2015 9:15 PM, Lev Priima wrote: Hi David, Explicit -Xmx does not allow explicit MaxRAMFraction to shrink MaxHeapSize down to -Xms(or even less down to default InitialHeapSize in case if -Xms also wasn't set explicitly). In other words explicit -Xmx has priority over explicit MaxRAMFraction if both are set and contradict with each other. Okay - that's good to know. However the -Xmx770m is a problem - our small devices only have 512MB of memory. Is the 770M only needed for 64-bit with -UseCompressedOops ? Thanks, David Lev On 03/17/2015 07:40 AM, David Holmes wrote: Hi Lev, On 17/03/2015 6:30 AM, Lev Priima wrote: Please reviewand push: Issue:https://bugs.openjdk.java.net/browse/JDK-8075071 Patch: http://cr.openjdk.java.net/~lpriima/8075071/0/webrev/ Tested locally: jtreg -vmoptions:-XX:MaxRAMFraction=9223372036854775807 -XX:-UseCompressedOops test/java/util/Arrays/TimSortStackSize2.java Test results: passed: 1 How does MaxRAMFraction interact with -Xmx? Thanks, David Lev
Re: RFR(XS): 8075071: [TEST_BUG] TimSortStackSize2.java: OOME: Java heap space: MaxHeap shrinked by MaxRAMFraction
So this is a problem that needs to be resolved. The memory requirements of the test are platform dependent but can't be satisfied on all platforms. Though I am surprised that 770M is needed given that two incarnations ago we set -Xmx to 385M! But it is appearing to be impossible to run this test in a way that can work on all platforms. David On 17/03/2015 10:15 PM, Lev Priima wrote: Yes. Lev On 03/17/2015 02:56 PM, David Holmes wrote: On 17/03/2015 9:15 PM, Lev Priima wrote: Hi David, Explicit -Xmx does not allow explicit MaxRAMFraction to shrink MaxHeapSize down to -Xms(or even less down to default InitialHeapSize in case if -Xms also wasn't set explicitly). In other words explicit -Xmx has priority over explicit MaxRAMFraction if both are set and contradict with each other. Okay - that's good to know. However the -Xmx770m is a problem - our small devices only have 512MB of memory. Is the 770M only needed for 64-bit with -UseCompressedOops ? Thanks, David Lev On 03/17/2015 07:40 AM, David Holmes wrote: Hi Lev, On 17/03/2015 6:30 AM, Lev Priima wrote: Please reviewand push: Issue:https://bugs.openjdk.java.net/browse/JDK-8075071 Patch: http://cr.openjdk.java.net/~lpriima/8075071/0/webrev/ Tested locally: jtreg -vmoptions:-XX:MaxRAMFraction=9223372036854775807 -XX:-UseCompressedOops test/java/util/Arrays/TimSortStackSize2.java Test results: passed: 1 How does MaxRAMFraction interact with -Xmx? Thanks, David Lev
Re: RFR [9] 8071472: Add field access to support setting final fields in readObject
On 17/03/2015 12:21, Peter Levart wrote: Hi Alan, I agree that not calling defaultReadObject() from readObject() and having a final field is potentially a bug. But need not be in case some other means of setting final fields was used (Unsafe or reflection). Some readObject() implementations in base module that Chris changed to use new API fall into this category. We can't track those usages, so to keep backwards compatibility, this checking has to be opt-in. Is there a more elegant way to opt-in? A @CheckFinalsAssignment annotation on the readObject() method? I'm not sure that an annotation is right here. Instead then it might work as a method on FieldSetter to enable strict checking. -Alan
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/17/2015 10:38 AM, Paul Sandoz wrote: On Mar 16, 2015, at 6:19 PM, Andrew Haley a...@redhat.com wrote: On 03/16/2015 10:03 AM, Paul Sandoz wrote: I am running this patch though our JPRT test system right now. The test looks good, two comments on it: - IMO it does not need the internal PRNG FastRandom, SplittableRandom can be used instead. - I suspect that test needs to be located in the hotspot test area, which likely means it gets run on more exotic platforms. Perhaps someone with more knowledge of the test configuration and infrastructure can comment? I have a patch with all of the changes that people have asked for except this last one. I don't know who to ask. Including Stefan, who may know more. Righto. I will post a new webrev once we decide. Thanks, Andrew.
