Re: RFR: JDK-8032050: TEST_BUG: java/rmi/activation/Activatable/shutdownGracefully/ShutdownGracefully.java fails intermittently
Thank you for fixing JDK-8023541. Then I leave ActivationLibrary.java for now. I still did some clean up following your suggestion. 1. I changed waitFor(long timeout) method, this method is going to use code like Process.waitFor(timeout, unit). This can be backported to JDK7. Also exitValue is kept as a return value. For making sure there is no Pipe leak, a cleanup thread will start when timeout happens. 2. Change in ShutdownGracefully is a little tricky. I think we should just destroy JVM once exception is thrown. So I move the wait logic into try block instead keep them in finally block. Can you receive it again. http://cr.openjdk.java.net/~tyan/JDK-8032050/webrev.02/ Thank you Tristan On 01/29/2014 03:16 PM, Stuart Marks wrote: Hi Tristan, I don't want to put the workaround into ActivationLibrary.rmidRunning() for a null return from the lookup, because this is only a workaround for an actual bug in rmid initialization. See the review I just posted for JDK-8023541. Adding JavaVM.waitFor(timeout) is something that would be useful in general, but it needs to be handled carefully. It uses the new Process.waitFor(timeout, unit) which is new in Java SE 8; this makes backporting to 7 more complicated. Not clear whether we'll do so, but I don't want to forclose the opportunity without discussion. It's also not clear how one can get the vm's exit status after JavaVM.waitFor() has returned true. With the Process API it's possible simply to call waitFor() or exitValue(). With JavaVM, a new API needs to be created, or the rule has to be established that one must call JavaVM.waitFor() to collect the exit status as well as to close the pipes from the subprocess. If JavaVM.waitFor(timeout, unit) is called without subsequently calling JavaVM.waitFor(), the pipes are leaked. In ShutdownGracefully.java, the finally-block needs to check to see if rmid is still running, and if it is, to shut it down. Simply calling waitFor(timeout, unit) isn't sufficient, because if the rmid process is still running, it will be left running. The straightforward approach would be to call ActivationLibrary.rmidRunning() to test if it's still running. Unfortunately this isn't quite right, because rmidRunning() has a timeout loop in it -- which should probably be removed. (I think there's a bug for this.) Another approach would be simply to call rmid.destroy(). This calls rmid's shutdown() method first, which is reasonable, but I'm not sure it kills the process if that fails. In any case, this already has a timeout loop waiting for the process to die, so ShutdownGracefully.java needn't use a new waitFor(timeout, unit) call. Removing the commented-out code that starts with no longer needed is good, and removing the ShutdownDetectThread is also good, since that's unnecessary. There are some more cleanups I have in mind here but I'd like to see a revised webrev before proceeding. Thanks, s'marks On 1/25/14 8:57 PM, Tristan Yan wrote: Hi Stuart Thank you for your review and suggestion. Yes, since this failure mode is very hard to be reproduced. I guess it's make sense to do some hack. And I also noticed in ActivationLibrary.rmidRunning. It does try to look up ActivationSystem but doesn't check if it's null. So I add the logic to make sure we will look up the non-null ActivationSystem. Also I did some cleanup if you don't mind. Add a waitFor(long timeout, TimeUnit unit) for JavaVM. Which we can have a better waitFor control. I appreciate you can review the code again. http://cr.openjdk.java.net/~tyan/JDK-8032050/webrev.01/ Thank you Tristan On 01/25/2014 10:20 AM, Stuart Marks wrote: On 1/23/14 10:34 PM, Tristan Yan wrote: Hi All Could you review the bug fix for JDK-8032050. http://cr.openjdk.java.net/~tyan/JDK-8032050/webrev.00/ Description: This rare happened failure caused because when RMID starts. It don't guarantee sun.rmi.server.Activation.startActivation finishes. Fix by adding a iterative getSystem with a 5 seconds timeout. Hi Tristan, Adding a timing/retry loop into this test isn't the correct approach for fixing this test. The timing loop isn't necessary because there is already a timing loop in RMID.start() in the RMI test library. (There's another timing loop in ActivationLibrary.rmidRunning() which should probably be removed.) So the intent of this library call is that it start rmid and wait for it to become ready. That logic doesn't need to be added to the test. In the bug report JDK-8032050 you had mentioned that the NullPointerException was suspicious. You're right! I took a look and it seemed like it was related to JDK-8023541, and I added a note to this effect to the bug report. The problem here is that rmid can come up and transiently return null instead of the stub of the activation system. That's what JDK-8023541 covers. I think that rmid itself needs to be fixed, though modifying the timing loop in the RMI test library to wait for rmid
Re: [8] RFR (S): 8033278: Missed access checks for Lookup.unreflect* after 8032585
On Jan 31, 2014, at 1:58 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: http://cr.openjdk.java.net/~vlivanov/8033278/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8033278 The fix for 8032585 [1] introduced a regression: in some cases access check on a reference class is omitted. The fix is to ensure that access check on a reference class is always performed. 104 case PROTECTED: 105 if ((allowedModes PROTECTED_OR_PACKAGE_ALLOWED) != 0 106 isSamePackage(defc, lookupClass)) 107 return true; 108 if ((allowedModes PROTECTED) == 0) 109 return false; 110 if ((mods STATIC) != 0 111 !isRelatedClass(refc, lookupClass)) 112 return false; 113 if ((allowedModes PROTECTED) != 0 114 isSuperClass(defc, lookupClass)) 115 return true; 116 return false; Can lines 113 to 116 be reduced to: return isSuperClass(defc, lookupClass)); ? The shuffling of the code looks correct (and simpler), but i am fuzzy on the nuances of access control. Paul. Testing: regression test, jdk/test/java/lang/invoke/, jdk/test/java/util/stream, vm.defmeth.testlist, vm.mlvm.testlist, nashorn (unit tests, octane), groovy Thanks! Best regards, Vladimir Ivanov [1] http://cr.openjdk.java.net/~vlivanov/8032585/webrev.00/
hg: jdk8/tl/jdk: 4 new changesets
Changeset: 9f098aed44c0 Author:anazarov Date: 2014-01-31 12:01 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/9f098aed44c0 8032025: Update repeating annotations demo Reviewed-by: jfranck + src/share/sample/annotations/DependencyChecker/PluginChecker/src/checker/Device.java + src/share/sample/annotations/DependencyChecker/PluginChecker/src/checker/Kettle.xml + src/share/sample/annotations/DependencyChecker/PluginChecker/src/checker/Module.java + src/share/sample/annotations/DependencyChecker/PluginChecker/src/checker/PluginChecker.java + src/share/sample/annotations/DependencyChecker/PluginChecker/src/checker/Require.java + src/share/sample/annotations/DependencyChecker/PluginChecker/src/checker/RequireContainer.java + src/share/sample/annotations/DependencyChecker/Plugins/src/plugins/BoilerPlugin.java + src/share/sample/annotations/DependencyChecker/Plugins/src/plugins/ExtendedBoilerPlugin.java + src/share/sample/annotations/DependencyChecker/Plugins/src/plugins/TimerPlugin.java + src/share/sample/annotations/Validator/src/PositiveIntegerSupplier.java + src/share/sample/annotations/Validator/src/SupplierValidator.java + src/share/sample/annotations/Validator/src/Validate.java + src/share/sample/annotations/Validator/src/Validator.java + src/share/sample/annotations/index.html Changeset: f72a8df6a2ed Author:anazarov Date: 2014-01-31 12:01 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f72a8df6a2ed 8031650: Update bulk operation demo Reviewed-by: psandoz, mduigou + src/share/sample/lambda/BulkDataOperations/index.html + src/share/sample/lambda/BulkDataOperations/src/CSVProcessor.java + src/share/sample/lambda/BulkDataOperations/src/Grep.java + src/share/sample/lambda/BulkDataOperations/src/PasswordGenerator.java + src/share/sample/lambda/BulkDataOperations/src/WC.java Changeset: 4574011c1689 Author:anazarov Date: 2014-01-31 12:01 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4574011c1689 8032020: Update try-with-resources demo Reviewed-by: darcy, alanb, smarks + src/share/sample/try-with-resources/index.html + src/share/sample/try-with-resources/src/CustomAutoCloseableSample.java + src/share/sample/try-with-resources/src/Unzip.java + src/share/sample/try-with-resources/src/ZipCat.java Changeset: a4f68fc5f353 Author:psandoz Date: 2014-01-31 12:01 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a4f68fc5f353 8032056: Create demo to illustrate new practices of the default methods usage Reviewed-by: briangoetz, rfield, psandoz Contributed-by: taras.led...@oracle.com + src/share/sample/lambda/DefaultMethods/ArrayIterator.java + src/share/sample/lambda/DefaultMethods/DiamondInheritance.java + src/share/sample/lambda/DefaultMethods/Inheritance.java + src/share/sample/lambda/DefaultMethods/MixIn.java + src/share/sample/lambda/DefaultMethods/Reflection.java + src/share/sample/lambda/DefaultMethods/SimplestUsage.java
Re: StringJoiner: detect or fix delimiter collision?
Hi Philip,
Am 27.01.2014 02:12, schrieb Philip Hodges:
Please please please drop StringJoiner from Java 1.8 before it is too late.
I have not seen any technical justifications whatsoever so far, just
inexplicable enthusiasm.
It is like giving young drivers a faster car with no seat belts.
+++1
I'm also with you with all your arguments against this API.
I'm really sorry I couldn't carry on arguing the case before August. As a
minority, I only have one person's quota of energy. I will try to get some more
people to look at it.
You are not alone.
Although I missed the delimiter/escaping problem those days, it was April 18 I posted my different
concerns about StringJoiner in this list:
I'm wondering, that StringJoiner has some logic for pre/suffix, but nothing to loop the elements
themselves
To me, StringJoiner is a useless complicated box around StringBuilder, and imagine, someone needs
thread-safety.
It also slows down performance, as it needs additional instances and additional class to be loaded
(critical at VM startup).
Instead please add to StringBuilder and StringBuffer:
append(CharSequence... elements);
append(char delimiter, CharSequence... elements);
append(char delimiter, Iterable? extends CharSequence elements);
cut(int len);// removes len chars at the end of the sequence
optional:
append(CharSequence delimiter, CharSequence... elements);
append(CharSequence delimiter, Iterable? extends CharSequence elements);
For performance reasons, append() should always append the trailing delimiter, which could be cut at
the end.
It's questionable, if class string needs a static (=no relation to an existing string in contrast to
non-static split()) join method, as it seduces to
[ + String.join(...) + ]
which needs some effort from javac side to optimize to a single StringBuilder
task.
IMO we better had StringBuilder.join(...), so javac could easily optimize to:
new StringBuilder().append('[').append(',',
someStrings).cut(1).append(']').toString()
THe current proposed implementation has:
1 class with 7 methods
2 additional methods in String
To cover the same functionality, above approach basically only needs 2 additional methods in
StringBuilder, has better performance, so what is complicated on that?
Am 27.01.2014 18:44, schrieb Mike Duigou:
The reception we've seen thus far for StringJoiner has been otherwise
exclusively enthusiastic and positive.
Where are those people, they don't speak up in this list, seem to don't know about the performance
issues and the traps which are mentioned here. We don't know how they will deal with the problems in
practical if they occur in real world.
On the other hand, the doomsayers also could refer to others out there which
see no win in this API.
-Ulf
Re: RFR(s): 8023541 Race condition in rmid initialization
On Jan 30, 2014, at 11:02 PM, Stuart Marks stuart.ma...@oracle.com wrote: Maybe. I'd guess that the new JMM will stick to covering well-behaved programs (e.g. ones that adhere to safe publication). The difficulty with issues like this one is that once publication has occurred unsafely, we have to figure out how to drag it back into the safe area. There are probably too many ways to write unsafe programs for the JMM to cover them in a simple fashion. For your delectation: http://www.cliffc.org/blog/2011/10/17/writing-to-final-fields-via-reflection/ http://www.cliffc.org/blog/2011/10/27/final-fields-part-2/ Paul.
Re: RFR(s): 8023541 Race condition in rmid initialization
On 01/31/2014 08:32 AM, Paul Sandoz wrote: On Jan 30, 2014, at 11:02 PM, Stuart Marks stuart.ma...@oracle.com wrote: Maybe. I'd guess that the new JMM will stick to covering well-behaved programs (e.g. ones that adhere to safe publication). The difficulty with issues like this one is that once publication has occurred unsafely, we have to figure out how to drag it back into the safe area. There are probably too many ways to write unsafe programs for the JMM to cover them in a simple fashion. For your delectation: http://www.cliffc.org/blog/2011/10/17/writing-to-final-fields-via-reflection/ http://www.cliffc.org/blog/2011/10/27/final-fields-part-2/ Simplifying final field rules is definitely on the agenda for JMM9 revisions. My guess is that the JMM per se will specify only the memory ordering effects, and for the most part leave the question of when reloads are suppressed as a JVM quality of implementation issue. While I'm at it, I think Stuart's current approach seems fine. Whenever you have no choice except to leak/publish in a constructor, use volatiles to track initialization. -Doug
Re: [8] RFR (S): 8033278: Missed access checks for Lookup.unreflect* after 8032585
Paul, The transformation you suggest is equivalent, but I reluctant to apply it. IMO, it doesn't add much value and current version is easier to read. Considering the current level of complexity in VA.isMemberAccessible I don't want to increase it even further :-) Best regards, Vladimir Ivanov PS: thanks for looking into the fix. On 1/31/14 1:31 PM, Paul Sandoz wrote: On Jan 31, 2014, at 1:58 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: http://cr.openjdk.java.net/~vlivanov/8033278/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8033278 The fix for 8032585 [1] introduced a regression: in some cases access check on a reference class is omitted. The fix is to ensure that access check on a reference class is always performed. 104 case PROTECTED: 105 if ((allowedModes PROTECTED_OR_PACKAGE_ALLOWED) != 0 106 isSamePackage(defc, lookupClass)) 107 return true; 108 if ((allowedModes PROTECTED) == 0) 109 return false; 110 if ((mods STATIC) != 0 111 !isRelatedClass(refc, lookupClass)) 112 return false; 113 if ((allowedModes PROTECTED) != 0 114 isSuperClass(defc, lookupClass)) 115 return true; 116 return false; Can lines 113 to 116 be reduced to: return isSuperClass(defc, lookupClass)); ? The shuffling of the code looks correct (and simpler), but i am fuzzy on the nuances of access control. Paul. Testing: regression test, jdk/test/java/lang/invoke/, jdk/test/java/util/stream, vm.defmeth.testlist, vm.mlvm.testlist, nashorn (unit tests, octane), groovy Thanks! Best regards, Vladimir Ivanov [1] http://cr.openjdk.java.net/~vlivanov/8032585/webrev.00/
Re: [8] RFR (S): 8033278: Missed access checks for Lookup.unreflect* after 8032585
On Jan 31, 2014, at 3:21 PM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Paul, The transformation you suggest is equivalent, but I reluctant to apply it. IMO, it doesn't add much value and current version is easier to read. OK, i guess we will just have to agree to disagree on that small point as i think the opposite :-) but I don't wanna block the review. Paul. Considering the current level of complexity in VA.isMemberAccessible I don't want to increase it even further :-) Best regards, Vladimir Ivanov PS: thanks for looking into the fix.
RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
Trivial change to CopyOnWriteArrayList.COWSubList.subList to catch the case where the fromIndex is greater that the toIndex. http://cr.openjdk.java.net/~chegar/8011645/webrev.00/webrev/ -Chris.
Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
On Jan 31, 2014, at 5:23 PM, Chris Hegarty chris.hega...@oracle.com wrote:
Trivial change to CopyOnWriteArrayList.COWSubList.subList to catch the case
where the fromIndex is greater that the toIndex.
http://cr.openjdk.java.net/~chegar/8011645/webrev.00/webrev/
Looks ok to me.
--
Shame it's awkward to share this code from ArrayList:
static void subListRangeCheck(int fromIndex, int toIndex, int size) {
if (fromIndex 0)
throw new IndexOutOfBoundsException(fromIndex = + fromIndex);
if (toIndex size)
throw new IndexOutOfBoundsException(toIndex = + toIndex);
if (fromIndex toIndex)
throw new IllegalArgumentException(fromIndex( + fromIndex +
) toIndex( + toIndex + ));
}
Paul.
hg: jdk8/tl/jdk: 8033278: Missed access checks for Lookup.unreflect* after 8032585
Changeset: f684c9773858 Author:vlivanov Date: 2014-01-31 21:07 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f684c9773858 8033278: Missed access checks for Lookup.unreflect* after 8032585 Reviewed-by: jrose, twisti ! src/share/classes/sun/invoke/util/VerifyAccess.java ! test/java/lang/invoke/ProtectedMemberDifferentPackage/Test.java ! test/java/lang/invoke/ProtectedMemberDifferentPackage/p1/T2.java ! test/java/lang/invoke/ProtectedMemberDifferentPackage/p2/T3.java
Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
Thanks Martin and Chris!
-Doug
On 01/31/2014 01:01 PM, Martin Buchholz wrote:
jsr166 CVS is now updated with this fix, and also adds some missing tck tests.
Index: src/jdk7/java/util/concurrent/CopyOnWriteArrayList.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/jdk7/java/util/concurrent/CopyOnWriteArrayList.java,v
retrieving revision 1.6
diff -u -r1.6 CopyOnWriteArrayList.java
--- src/jdk7/java/util/concurrent/CopyOnWriteArrayList.java4 Nov 2013 00:00:39
-1.6
+++ src/jdk7/java/util/concurrent/CopyOnWriteArrayList.java31 Jan 2014 17:50:14
-
@@ -1232,7 +1232,7 @@
lock.lock();
try {
checkForComodification();
-if (fromIndex 0 || toIndex size)
+if (fromIndex 0 || toIndex size || fromIndex toIndex)
throw new IndexOutOfBoundsException();
return new COWSubListE(l, fromIndex + offset,
toIndex + offset);
Index: src/main/java/util/concurrent/CopyOnWriteArrayList.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/CopyOnWriteArrayList.java,v
retrieving revision 1.114
diff -u -r1.114 CopyOnWriteArrayList.java
--- src/main/java/util/concurrent/CopyOnWriteArrayList.java4 Nov 2013 00:00:39
-1.114
+++ src/main/java/util/concurrent/CopyOnWriteArrayList.java31 Jan 2014 17:50:15
-
@@ -1371,7 +1371,7 @@
lock.lock();
try {
checkForComodification();
-if (fromIndex 0 || toIndex size)
+if (fromIndex 0 || toIndex size || fromIndex toIndex)
throw new IndexOutOfBoundsException();
return new COWSubListE(l, fromIndex + offset,
toIndex + offset);
Index: src/test/tck/CopyOnWriteArrayListTest.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/test/tck/CopyOnWriteArrayListTest.java,v
retrieving revision 1.29
diff -u -r1.29 CopyOnWriteArrayListTest.java
--- src/test/tck/CopyOnWriteArrayListTest.java30 May 2013 03:28:55 -1.29
+++ src/test/tck/CopyOnWriteArrayListTest.java31 Jan 2014 17:50:15 -
@@ -500,10 +500,10 @@
* can not store the objects inside the list
*/
public void testToArray_ArrayStoreException() {
+CopyOnWriteArrayList c = new CopyOnWriteArrayList();
+c.add(zfasdfsdf);
+c.add(asdadasd);
try {
-CopyOnWriteArrayList c = new CopyOnWriteArrayList();
-c.add(zfasdfsdf);
-c.add(asdadasd);
c.toArray(new Long[5]);
shouldThrow();
} catch (ArrayStoreException success) {}
@@ -513,167 +513,196 @@
* get throws an IndexOutOfBoundsException on a negative index
*/
public void testGet1_IndexOutOfBoundsException() {
-try {
-CopyOnWriteArrayList c = new CopyOnWriteArrayList();
-c.get(-1);
-shouldThrow();
-} catch (IndexOutOfBoundsException success) {}
+CopyOnWriteArrayList c = populatedArray(5);
+List[] lists = { c, c.subList(1, c.size() - 1) };
+for (List list : lists) {
+try {
+list.get(-1);
+shouldThrow();
+} catch (IndexOutOfBoundsException success) {}
+}
}
/**
* get throws an IndexOutOfBoundsException on a too high index
*/
public void testGet2_IndexOutOfBoundsException() {
-try {
-CopyOnWriteArrayList c = new CopyOnWriteArrayList();
-c.add(asdasd);
-c.add(asdad);
-c.get(100);
-shouldThrow();
-} catch (IndexOutOfBoundsException success) {}
+CopyOnWriteArrayList c = populatedArray(5);
+List[] lists = { c, c.subList(1, c.size() - 1) };
+for (List list : lists) {
+try {
+list.get(list.size());
+shouldThrow();
+} catch (IndexOutOfBoundsException success) {}
+}
}
/**
* set throws an IndexOutOfBoundsException on a negative index
*/
public void testSet1_IndexOutOfBoundsException() {
-try {
-CopyOnWriteArrayList c = new CopyOnWriteArrayList();
-c.set(-1,qwerty);
-shouldThrow();
-} catch (IndexOutOfBoundsException success) {}
+CopyOnWriteArrayList c = populatedArray(5);
+List[] lists = { c, c.subList(1, c.size() - 1) };
+for (List list : lists) {
+try {
+list.set(-1, qwerty);
+shouldThrow();
+} catch (IndexOutOfBoundsException success) {}
+}
}
/**
* set throws an IndexOutOfBoundsException on a too
Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
Thanks I’ll update the test before pushing. I know you have tests in the TCK for this now, but I’ll add the trivial jtreg test to OpenJDK to ensure this doesn’t creep back. -Chris. On 31 Jan 2014, at 18:07, Martin Buchholz marti...@google.com wrote: The jtreg test is fine, but: s/IOBE/IOOBE/ When I created MOAT.java many years ago, I intended tests such as this to get added to that, so that all of the List implementations could share the same test code. jsr166 does not have the same concern, since it only has one List implementation at the moment. Today, there are other choices, like sharing test infrastructure with Guava e.g. ListTestSuiteBuilder. More generally, openjdk core libraries can benefit from all the great testing work that guava folk have done. On Fri, Jan 31, 2014 at 8:23 AM, Chris Hegarty chris.hega...@oracle.com wrote: Trivial change to CopyOnWriteArrayList.COWSubList.subList to catch the case where the fromIndex is greater that the toIndex. http://cr.openjdk.java.net/~chegar/8011645/webrev.00/webrev/ -Chris.
RFR 8032221 Typo in java.util.date
Please review a typo and javadoc cleanup for java.util.Date webrev: http://cr.openjdk.java.net/~rriggs/webrev-date-typo-8032221/ Thanks, Roger
Re: RFR 8032221 Typo in java.util.date
On 01/31/2014 10:33 AM, roger riggs wrote: Please review a typo and javadoc cleanup for java.util.Date webrev: http://cr.openjdk.java.net/~rriggs/webrev-date-typo-8032221/ Thanks, Roger Looks good Roger. Cheers, -Joe
Re: RFR 8032221 Typo in java.util.date
trancate - truncate . Other cleanups look good too. -Chris. On 31 Jan 2014, at 18:33, roger riggs roger.ri...@oracle.com wrote: Please review a typo and javadoc cleanup for java.util.Date webrev: http://cr.openjdk.java.net/~rriggs/webrev-date-typo-8032221/ Thanks, Roger
Re: RFR 8032221 Typo in java.util.date
looks fine. getting rid of tt and code, is something I guess we should look to do throughout all of our code? On Jan 31, 2014, at 1:33 PM, roger riggs wrote: Please review a typo and javadoc cleanup for java.util.Date webrev: http://cr.openjdk.java.net/~rriggs/webrev-date-typo-8032221/ Thanks, Roger Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: ObjectIn/OutputStream improvements
Forwarding to correct core-libs-dev list.
-Chris.
On 31 Jan 2014, at 14:22, Chris Hegarty chris.hega...@oracle.com wrote:
Hi Robert,
I think your patch can be split into two logical, independent, parts. The
first is the use of unsafe to access the String UTF length. The seconds is to
reuse, where possible, the existing StringBuilder instances, skipping of
primitive/object reading/writing where applicable, and general cleanup.
Since this is a very sensitive area I would like to consider these
separately. To that end, I have taken the changes that are applicable to the
latter, removed any subjective stylistic changes, and made some additional
cleanup improvements.
http://cr.openjdk.java.net/~chegar/serial_stupp.00/webrev/
Specifically,
* I think for clarify and readability SerialCallbackContext
checkAndSetUsed() should be invoked directly. It makes it very
clear what the intent is.
* Skip primitive/object reading/writing if no primitives/objects in
the stream/class. ( I personally don't see any benefit of rounding
up the size of the array, it just seems to add unnecessary
complexity )
* Move primitive types into getPrimitiveSignature for better reuse
of code. This retains your change to not create the additional
StringBuilder when it is not necessary.
I am currently running tests on this change.
-Chris.
On 07/01/14 13:03, Robert Stupp wrote:
Hi,
I've attached the diff to the original email - it has been stripped.
Here's a second try (inline).
I've already signed the OCA and it has been approved :)
Robert
# HG changeset patch
# User snazy
# Date 1387101091 -3600
# Sun Dec 15 10:51:31 2013 +0100
# Node ID 6d46d99212453017c88417678d08dc8f10da9606
# Parent 9e1be800420265e5dcf264a7ed4abb6f230dd19d
Removed some unnecessary variable assignments.
ObjectInputStream:
- skip primitive/object reading if no primitive/objects in class
- use shared StringBuilder for string reading (prevent superfluous
object allocations)
ObjectOutputStream:
- skip primitive/object writing if no primitive/objects in class
- use unsafe access to calculate UTF-length
- use unsafe access in readBytes() and writeChars() methods to access
String value field
- removed cbuf field
ObjectStreamClass/ObjectStreamField:
- minor improvement in getClassSignature ; share method code with
ObjectStreamField
diff --git a/src/share/classes/java/io/ObjectInputStream.java
b/src/share/classes/java/io/ObjectInputStream.java
--- a/src/share/classes/java/io/ObjectInputStream.java
+++ b/src/share/classes/java/io/ObjectInputStream.java
@@ -39,8 +39,8 @@
import java.util.HashMap;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.atomic.AtomicBoolean;
import static java.io.ObjectStreamClass.processQueue;
+
import sun.reflect.misc.ReflectUtil;
/**
@@ -534,7 +534,7 @@
if (ctx == null) {
throw new NotActiveException(not in call to readObject);
}
-Object curObj = ctx.getObj();
+ctx.getObj();
ObjectStreamClass curDesc = ctx.getDesc();
bin.setBlockDataMode(false);
GetFieldImpl getField = new GetFieldImpl(curDesc);
@@ -1597,7 +1597,7 @@
int descHandle = handles.assign(unshared ? unsharedMarker : desc);
passHandle = NULL_HANDLE;
-ObjectStreamClass readDesc = null;
+ObjectStreamClass readDesc;
try {
readDesc = readClassDescriptor();
} catch (ClassNotFoundException ex) {
@@ -1976,29 +1976,34 @@
}
int primDataSize = desc.getPrimDataSize();
-if (primVals == null || primVals.length primDataSize) {
-primVals = new byte[primDataSize];
-}
-bin.readFully(primVals, 0, primDataSize, false);
-if (obj != null) {
-desc.setPrimFieldValues(obj, primVals);
-}
-
-int objHandle = passHandle;
-ObjectStreamField[] fields = desc.getFields(false);
-Object[] objVals = new Object[desc.getNumObjFields()];
-int numPrimFields = fields.length - objVals.length;
-for (int i = 0; i objVals.length; i++) {
-ObjectStreamField f = fields[numPrimFields + i];
-objVals[i] = readObject0(f.isUnshared());
-if (f.getField() != null) {
-handles.markDependency(objHandle, passHandle);
+if (primDataSize 0) {
+if (primVals == null || primVals.length primDataSize) {
+primVals = new byte[ ((primDataSize4)+1)4 ];
+}
+bin.readFully(primVals, 0, primDataSize, false);
+if (obj != null) {
+desc.setPrimFieldValues(obj, primVals);
}
}
-if (obj != null) {
-desc.setObjFieldValues(obj, objVals);
+
+int numObjFields = desc.getNumObjFields();
+if
Re: Re: ObjectIn/OutputStream improvements
Forwarding for correct core-libs-dev list.
-Chris.
On 31 Jan 2014, at 15:26, Robert Stupp sn...@gmx.de wrote:
Hi Chris,
fine. I'm a bit proud that my 5ct help to improve JDK9 :)
The primitive buffer array size rounding was there to reduce the number of
re-allocations when a class requires a slightly bigger primitive buffer than
another. Maybe we can introduce some minimum buffer size - e.g. 64 or 128
bytes? This should be enough for most classes. Classes that require a bigger
buffer will always force an extend of the buffer - rounded or not.
Robert
Gesendet: Freitag, 31. Januar 2014 um 15:22 Uhr
Von: Chris Hegarty chris.hega...@oracle.com
An: Robert Stupp sn...@gmx.de, core-libs-dev-request
core-libs-dev-requ...@openjdk.java.net
Betreff: Re: Aw: Re: ObjectIn/OutputStream improvements
Hi Robert,
I think your patch can be split into two logical, independent, parts.
The first is the use of unsafe to access the String UTF length. The
seconds is to reuse, where possible, the existing StringBuilder
instances, skipping of primitive/object reading/writing where
applicable, and general cleanup.
Since this is a very sensitive area I would like to consider these
separately. To that end, I have taken the changes that are applicable to
the latter, removed any subjective stylistic changes, and made some
additional cleanup improvements.
http://cr.openjdk.java.net/~chegar/serial_stupp.00/webrev/
Specifically,
* I think for clarify and readability SerialCallbackContext
checkAndSetUsed() should be invoked directly. It makes it very
clear what the intent is.
* Skip primitive/object reading/writing if no primitives/objects in
the stream/class. ( I personally don't see any benefit of rounding
up the size of the array, it just seems to add unnecessary
complexity )
* Move primitive types into getPrimitiveSignature for better reuse
of code. This retains your change to not create the additional
StringBuilder when it is not necessary.
I am currently running tests on this change.
-Chris.
On 07/01/14 13:03, Robert Stupp wrote:
Hi,
I've attached the diff to the original email - it has been stripped.
Here's a second try (inline).
I've already signed the OCA and it has been approved :)
Robert
# HG changeset patch
# User snazy
# Date 1387101091 -3600
# Sun Dec 15 10:51:31 2013 +0100
# Node ID 6d46d99212453017c88417678d08dc8f10da9606
# Parent 9e1be800420265e5dcf264a7ed4abb6f230dd19d
Removed some unnecessary variable assignments.
ObjectInputStream:
- skip primitive/object reading if no primitive/objects in class
- use shared StringBuilder for string reading (prevent superfluous
object allocations)
ObjectOutputStream:
- skip primitive/object writing if no primitive/objects in class
- use unsafe access to calculate UTF-length
- use unsafe access in readBytes() and writeChars() methods to access
String value field
- removed cbuf field
ObjectStreamClass/ObjectStreamField:
- minor improvement in getClassSignature ; share method code with
ObjectStreamField
diff --git a/src/share/classes/java/io/ObjectInputStream.java
b/src/share/classes/java/io/ObjectInputStream.java
--- a/src/share/classes/java/io/ObjectInputStream.java
+++ b/src/share/classes/java/io/ObjectInputStream.java
@@ -39,8 +39,8 @@
import java.util.HashMap;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.atomic.AtomicBoolean;
import static java.io.ObjectStreamClass.processQueue;
+
import sun.reflect.misc.ReflectUtil;
/**
@@ -534,7 +534,7 @@
if (ctx == null) {
throw new NotActiveException(not in call to readObject);
}
- Object curObj = ctx.getObj();
+ ctx.getObj();
ObjectStreamClass curDesc = ctx.getDesc();
bin.setBlockDataMode(false);
GetFieldImpl getField = new GetFieldImpl(curDesc);
@@ -1597,7 +1597,7 @@
int descHandle = handles.assign(unshared ? unsharedMarker : desc);
passHandle = NULL_HANDLE;
- ObjectStreamClass readDesc = null;
+ ObjectStreamClass readDesc;
try {
readDesc = readClassDescriptor();
} catch (ClassNotFoundException ex) {
@@ -1976,29 +1976,34 @@
}
int primDataSize = desc.getPrimDataSize();
- if (primVals == null || primVals.length primDataSize) {
- primVals = new byte[primDataSize];
- }
- bin.readFully(primVals, 0, primDataSize, false);
- if (obj != null) {
- desc.setPrimFieldValues(obj, primVals);
- }
-
- int objHandle = passHandle;
- ObjectStreamField[] fields = desc.getFields(false);
- Object[] objVals = new Object[desc.getNumObjFields()];
- int numPrimFields = fields.length - objVals.length;
- for (int i = 0; i objVals.length; i++) {
- ObjectStreamField f = fields[numPrimFields + i];
- objVals[i] = readObject0(f.isUnshared());
- if (f.getField() != null) {
- handles.markDependency(objHandle, passHandle);
+ if (primDataSize 0) {
+ if (primVals == null ||
Re: ObjectIn/OutputStream improvements
On 1/31/14 10:46 AM, Chris Hegarty wrote: I think your patch can be split into two logical, independent, parts. The first is the use of unsafe to access the String UTF length. The seconds is to reuse, where possible, the existing StringBuilder instances, skipping of primitive/object reading/writing where applicable, and general cleanup. Since this is a very sensitive area I would like to consider these separately. To that end, I have taken the changes that are applicable to the latter, removed any subjective stylistic changes, and made some additional cleanup improvements. http://cr.openjdk.java.net/~chegar/serial_stupp.00/webrev/ I agree with splitting the Unsafe usages so they can be reviewed separately. New and changed usage of Unsafe will require exacting scrutiny. In general, the cleanups and refactorings look fine. I have a question about the change to reuse StringBuilder instances. This replaces freshly allocated StringBuilders stored in local variables with reuse of a StringBuilder stored in a field of BlockDataInputStream, which in turn is stored in a field of ObjectInputStream. Thus, instead of creating of lots of temporaries that become gc-eligible very quickly, this creates a single long-lived object whose size is the high-water mark of the longest string that's been read. The change does reduce allocation pressure, but the point of generational GC is to make allocation and collection of temporaries quite inexpensive. Is this the right tradeoff? s'marks
Re: [8] RFR (S): 8033278: Missed access checks for Lookup.unreflect* after 8032585
Looks good. On Jan 30, 2014, at 4:58 PM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: http://cr.openjdk.java.net/~vlivanov/8033278/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8033278 The fix for 8032585 [1] introduced a regression: in some cases access check on a reference class is omitted. The fix is to ensure that access check on a reference class is always performed. Testing: regression test, jdk/test/java/lang/invoke/, jdk/test/java/util/stream, vm.defmeth.testlist, vm.mlvm.testlist, nashorn (unit tests, octane), groovy Thanks! Best regards, Vladimir Ivanov [1] http://cr.openjdk.java.net/~vlivanov/8032585/webrev.00/ ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: [8] RFR (S): 8033278: Missed access checks for Lookup.unreflect* after 8032585
Chris, thank you. Best regards, Vladimir Ivanov On 1/31/14 11:22 PM, Christian Thalinger wrote: Looks good. On Jan 30, 2014, at 4:58 PM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: http://cr.openjdk.java.net/~vlivanov/8033278/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8033278 The fix for 8032585 [1] introduced a regression: in some cases access check on a reference class is omitted. The fix is to ensure that access check on a reference class is always performed. Testing: regression test, jdk/test/java/lang/invoke/, jdk/test/java/util/stream, vm.defmeth.testlist, vm.mlvm.testlist, nashorn (unit tests, octane), groovy Thanks! Best regards, Vladimir Ivanov [1] http://cr.openjdk.java.net/~vlivanov/8032585/webrev.00/ ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: ObjectIn/OutputStream improvements
On 31 Jan 2014, at 19:07, Stuart Marks stuart.ma...@oracle.com wrote: On 1/31/14 10:46 AM, Chris Hegarty wrote: I think your patch can be split into two logical, independent, parts. The first is the use of unsafe to access the String UTF length. The seconds is to reuse, where possible, the existing StringBuilder instances, skipping of primitive/object reading/writing where applicable, and general cleanup. Since this is a very sensitive area I would like to consider these separately. To that end, I have taken the changes that are applicable to the latter, removed any subjective stylistic changes, and made some additional cleanup improvements. http://cr.openjdk.java.net/~chegar/serial_stupp.00/webrev/ I agree with splitting the Unsafe usages so they can be reviewed separately. New and changed usage of Unsafe will require exacting scrutiny. In general, the cleanups and refactorings look fine. I have a question about the change to reuse StringBuilder instances. This replaces freshly allocated StringBuilders stored in local variables with reuse of a StringBuilder stored in a field of BlockDataInputStream, which in turn is stored in a field of ObjectInputStream. Thus, instead of creating of lots of temporaries that become gc-eligible very quickly, this creates a single long-lived object whose size is the high-water mark of the longest string that's been read. The change does reduce allocation pressure, but the point of generational GC is to make allocation and collection of temporaries quite inexpensive. Is this the right tradeoff? Good point Stuart. I can’t imagine that reusing is a problem, provided we can release. It may be that we should look at clearing the reference, and others, in close(). -Chris. s'marks
RE: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
Martin, Unifying the List testing code might be kind of tricky with https://bugs.openjdk.java.net/browse/JDK-4506427 as unresolved. http://docs.oracle.com/javase/7/docs/api/java/util/List.html http://docs.oracle.com/javase/7/docs/api/java/util/AbstractList.html The patch looks good though. Cheers, Jason Date: Fri, 31 Jan 2014 10:07:31 -0800 Subject: Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does notvalidate range properly From: marti...@google.com To: chris.hega...@oracle.com CC: core-libs-dev@openjdk.java.net The jtreg test is fine, but: s/IOBE/IOOBE/ When I created MOAT.java many years ago, I intended tests such as this to get added to that, so that all of the List implementations could share the same test code. jsr166 does not have the same concern, since it only has one List implementation at the moment. Today, there are other choices, like sharing test infrastructure with Guava e.g. ListTestSuiteBuilder. More generally, openjdk core libraries can benefit from all the great testing work that guava folk have done. On Fri, Jan 31, 2014 at 8:23 AM, Chris Hegarty chris.hega...@oracle.comwrote: Trivial change to CopyOnWriteArrayList.COWSubList.subList to catch the case where the fromIndex is greater that the toIndex. http://cr.openjdk.java.net/~chegar/8011645/webrev.00/webrev/ -Chris.
Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
On Jan 31 2014, at 11:50 , Martin Buchholz marti...@google.com wrote: Jason, Thanks for pointing that out. I'm sure I have seen those bugs before (when I owned them!) but had suppressed the memory. I'm currently the assignee for this bug. I probably didn't try fixing them because there is no clean way out, and I was afraid of getting bogged down in compatibility hell for what is a non-issue for real-world users. Indeed. That's exactly why they still haven't been addressed. Suggestions are, of course, always welcome. Mike On Fri, Jan 31, 2014 at 11:43 AM, Jason Mehrens jason_mehr...@hotmail.comwrote: Martin, Unifying the List testing code might be kind of tricky with https://bugs.openjdk.java.net/browse/JDK-4506427 as unresolved. http://docs.oracle.com/javase/7/docs/api/java/util/List.html http://docs.oracle.com/javase/7/docs/api/java/util/AbstractList.html The patch looks good though. Cheers, Jason Date: Fri, 31 Jan 2014 10:07:31 -0800 Subject: Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly From: marti...@google.com To: chris.hega...@oracle.com CC: core-libs-dev@openjdk.java.net The jtreg test is fine, but: s/IOBE/IOOBE/ When I created MOAT.java many years ago, I intended tests such as this to get added to that, so that all of the List implementations could share the same test code. jsr166 does not have the same concern, since it only has one List implementation at the moment. Today, there are other choices, like sharing test infrastructure with Guava e.g. ListTestSuiteBuilder. More generally, openjdk core libraries can benefit from all the great testing work that guava folk have done. On Fri, Jan 31, 2014 at 8:23 AM, Chris Hegarty chris.hega...@oracle.com wrote: Trivial change to CopyOnWriteArrayList.COWSubList.subList to catch the case where the fromIndex is greater that the toIndex. http://cr.openjdk.java.net/~chegar/8011645/webrev.00/webrev/ -Chris.
Re: ObjectIn/OutputStream improvements
Seems like good changes.
ObjectStreamClass.java::
- Why not have getClassSignature() return an interned string? (that's if
interning is actually essential. Are we sure it's not just overhead?)
On Jan 31 2014, at 10:46 , Chris Hegarty chris.hega...@oracle.com wrote:
Forwarding to correct core-libs-dev list.
-Chris.
On 31 Jan 2014, at 14:22, Chris Hegarty chris.hega...@oracle.com wrote:
Hi Robert,
I think your patch can be split into two logical, independent, parts. The
first is the use of unsafe to access the String UTF length. The seconds is
to reuse, where possible, the existing StringBuilder instances, skipping of
primitive/object reading/writing where applicable, and general cleanup.
Since this is a very sensitive area I would like to consider these
separately. To that end, I have taken the changes that are applicable to the
latter, removed any subjective stylistic changes, and made some additional
cleanup improvements.
http://cr.openjdk.java.net/~chegar/serial_stupp.00/webrev/
Specifically,
* I think for clarify and readability SerialCallbackContext
checkAndSetUsed() should be invoked directly. It makes it very
clear what the intent is.
* Skip primitive/object reading/writing if no primitives/objects in
the stream/class. ( I personally don't see any benefit of rounding
up the size of the array, it just seems to add unnecessary
complexity )
* Move primitive types into getPrimitiveSignature for better reuse
of code. This retains your change to not create the additional
StringBuilder when it is not necessary.
I am currently running tests on this change.
-Chris.
On 07/01/14 13:03, Robert Stupp wrote:
Hi,
I've attached the diff to the original email - it has been stripped.
Here's a second try (inline).
I've already signed the OCA and it has been approved :)
Robert
# HG changeset patch
# User snazy
# Date 1387101091 -3600
# Sun Dec 15 10:51:31 2013 +0100
# Node ID 6d46d99212453017c88417678d08dc8f10da9606
# Parent 9e1be800420265e5dcf264a7ed4abb6f230dd19d
Removed some unnecessary variable assignments.
ObjectInputStream:
- skip primitive/object reading if no primitive/objects in class
- use shared StringBuilder for string reading (prevent superfluous
object allocations)
ObjectOutputStream:
- skip primitive/object writing if no primitive/objects in class
- use unsafe access to calculate UTF-length
- use unsafe access in readBytes() and writeChars() methods to access
String value field
- removed cbuf field
ObjectStreamClass/ObjectStreamField:
- minor improvement in getClassSignature ; share method code with
ObjectStreamField
diff --git a/src/share/classes/java/io/ObjectInputStream.java
b/src/share/classes/java/io/ObjectInputStream.java
--- a/src/share/classes/java/io/ObjectInputStream.java
+++ b/src/share/classes/java/io/ObjectInputStream.java
@@ -39,8 +39,8 @@
import java.util.HashMap;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.atomic.AtomicBoolean;
import static java.io.ObjectStreamClass.processQueue;
+
import sun.reflect.misc.ReflectUtil;
/**
@@ -534,7 +534,7 @@
if (ctx == null) {
throw new NotActiveException(not in call to readObject);
}
-Object curObj = ctx.getObj();
+ctx.getObj();
ObjectStreamClass curDesc = ctx.getDesc();
bin.setBlockDataMode(false);
GetFieldImpl getField = new GetFieldImpl(curDesc);
@@ -1597,7 +1597,7 @@
int descHandle = handles.assign(unshared ? unsharedMarker : desc);
passHandle = NULL_HANDLE;
-ObjectStreamClass readDesc = null;
+ObjectStreamClass readDesc;
try {
readDesc = readClassDescriptor();
} catch (ClassNotFoundException ex) {
@@ -1976,29 +1976,34 @@
}
int primDataSize = desc.getPrimDataSize();
-if (primVals == null || primVals.length primDataSize) {
-primVals = new byte[primDataSize];
-}
-bin.readFully(primVals, 0, primDataSize, false);
-if (obj != null) {
-desc.setPrimFieldValues(obj, primVals);
-}
-
-int objHandle = passHandle;
-ObjectStreamField[] fields = desc.getFields(false);
-Object[] objVals = new Object[desc.getNumObjFields()];
-int numPrimFields = fields.length - objVals.length;
-for (int i = 0; i objVals.length; i++) {
-ObjectStreamField f = fields[numPrimFields + i];
-objVals[i] = readObject0(f.isUnshared());
-if (f.getField() != null) {
-handles.markDependency(objHandle, passHandle);
+if (primDataSize 0) {
+if (primVals == null || primVals.length primDataSize) {
+primVals = new byte[ ((primDataSize4)+1)4 ];
+}
+bin.readFully(primVals, 0, primDataSize, false);
+if
Re: RFR 8032221 Typo in java.util.date
On 01/31/2014 10:44 AM, Lance Andersen - Oracle wrote:
looks fine. getting rid of tt and code, is something I guess we should
look to do throughout all of our code?
Yes :-)
In the core libraries, Jason did some bulk conversions along those lines
in JDK 8:
JDK-8017324 Cleanup of the javadoc code tag in java.security.*
etc.
For JDK 9, normalizing to {@code} has been proposed earlier:
http://mail.openjdk.java.net/pipermail/jdk9-dev/2013-December/000141.html
Thanks,
-Joe
On Jan 31, 2014, at 1:33 PM, roger riggs wrote:
Please review a typo and javadoc cleanup for java.util.Date
webrev:
http://cr.openjdk.java.net/~rriggs/webrev-date-typo-8032221/
Thanks, Roger
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
On Jan 31 2014, at 15:09 , Jason Mehrens jason_mehr...@hotmail.com wrote: Martin, Mike, Totally agree with everything that has been said on this. Leaving it 'unresolved' or 'closed as will not fix' won't bother me. Mike has it listed as a 'doc clarification only' so my suggestion toward a resolution would be to modify AbstractList.subList documentation with a spec implementation note. Might be worth adding to the bug report that AbstractList.removeRange doesn't detect or specify that exceptions are thrown when 'to' is less than 'from' but, ArrayList.removeRange overrides AbstactList.removeRange with an implementation that detects and throws IOOBE. Might want to add an optional IOOBE exception to AbstractList.removeRange documentation when this patch is attempted. Added to the bug so that it doesn't get forgotten. Mike Jason Subject: Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly From: mike.dui...@oracle.com Date: Fri, 31 Jan 2014 12:06:16 -0800 CC: jason_mehr...@hotmail.com; core-libs-dev@openjdk.java.net To: marti...@google.com On Jan 31 2014, at 11:50 , Martin Buchholz marti...@google.com wrote: Jason, Thanks for pointing that out. I'm sure I have seen those bugs before (when I owned them!) but had suppressed the memory. I'm currently the assignee for this bug. I probably didn't try fixing them because there is no clean way out, and I was afraid of getting bogged down in compatibility hell for what is a non-issue for real-world users. Indeed. That's exactly why they still haven't been addressed. Suggestions are, of course, always welcome. Mike
Re: ObjectIn/OutputStream improvements
One would have to measure of course, but intuitively, it seems like a good change to reuse the stringbuilder. There's also the issue that the local stringbuilder before was default-sized, and for large content, it would generate even more garbage as the underlying array is expanded. The temporary short lived allocations are cheap is, unfortunately, a semi-myth, or at least somewhat misguided. It's true that individually they may be cheap, but they have macro effects. The higher the allocation rate, the more young GCs we get. Every young GC (on hotspot collectors, at least) is a STW pause. Bringing threads to a safepoint has some cost, especially if there are many of them on large many-core machines. With larger heaps these days, young gens tend to be larger as well. When GC runs, it trashes the cpu caches for the mutators, so when they resume, they may get cache misses. At each young GC, some objects may get promoted prematurely to tenured. So no, I wouldn't say it's quite inexpensive :). When you have no option but to allocate, it's nice to have collectors that can handle those necessary allocations well. Otherwise, if it's a perf sensitive area and avoiding allocations doesn't obfuscate or make the code significantly less maintainable, it's usually better to avoid allocations. Just my $.02 Sent from my phone On Jan 31, 2014 2:06 PM, Stuart Marks stuart.ma...@oracle.com wrote: On 1/31/14 10:46 AM, Chris Hegarty wrote: I think your patch can be split into two logical, independent, parts. The first is the use of unsafe to access the String UTF length. The seconds is to reuse, where possible, the existing StringBuilder instances, skipping of primitive/object reading/writing where applicable, and general cleanup. Since this is a very sensitive area I would like to consider these separately. To that end, I have taken the changes that are applicable to the latter, removed any subjective stylistic changes, and made some additional cleanup improvements. http://cr.openjdk.java.net/~chegar/serial_stupp.00/webrev/ I agree with splitting the Unsafe usages so they can be reviewed separately. New and changed usage of Unsafe will require exacting scrutiny. In general, the cleanups and refactorings look fine. I have a question about the change to reuse StringBuilder instances. This replaces freshly allocated StringBuilders stored in local variables with reuse of a StringBuilder stored in a field of BlockDataInputStream, which in turn is stored in a field of ObjectInputStream. Thus, instead of creating of lots of temporaries that become gc-eligible very quickly, this creates a single long-lived object whose size is the high-water mark of the longest string that's been read. The change does reduce allocation pressure, but the point of generational GC is to make allocation and collection of temporaries quite inexpensive. Is this the right tradeoff? s'marks
