RE: Should indexOfLatin1Unsafe be private instead of public in java\lang\StringUTF16.java?
OK, thanks Roger, I am looking for the opportunity of speeding up indexOf, and would fix it by the way once I have a patch there. Regards Patrick -Original Message- From: core-libs-dev On Behalf Of Roger Riggs Sent: Friday, February 14, 2020 12:12 AM To: core-libs-dev@openjdk.java.net Subject: Re: Should indexOfLatin1Unsafe be private instead of public in java\lang\StringUTF16.java? Hi Patrick, Private would be good, but since they are in a package-private class they are not visibile outside the package. If there is some other change to the class then fix it but otherwise not worth the overhead. Roger On 2/13/20 10:34 AM, Patrick Zhang OS wrote: > Hi, > > A quick question, > I read the code snippets of indexOf(String str), found indexOfUnsafe [1] and > indexOfLatin1Unsafe [2] have different access control, but it looks both > should be private. Did I miss any outer caller or any other restriction? > Thanks for your comment. > > [1] private static int indexOfUnsafe(byte[] value, int valueCount, > byte[] str, int strCount, int fromIndex) > http://hg.openjdk.java.net/jdk/jdk/file/cf6409153216/src/java.base/sha > re/classes/java/lang/StringUTF16.java#l392 > [2] public static int indexOfLatin1Unsafe(byte[] src, int srcCount, > byte[] tgt, int tgtCount, int fromIndex) > http://hg.openjdk.java.net/jdk/jdk/file/cf6409153216/src/java.base/sha > re/classes/java/lang/StringUTF16.java#l440 > > Regards > Patrick >
Should indexOfLatin1Unsafe be private instead of public in java\lang\StringUTF16.java?
Hi, A quick question, I read the code snippets of indexOf(String str), found indexOfUnsafe [1] and indexOfLatin1Unsafe [2] have different access control, but it looks both should be private. Did I miss any outer caller or any other restriction? Thanks for your comment. [1] private static int indexOfUnsafe(byte[] value, int valueCount, byte[] str, int strCount, int fromIndex) http://hg.openjdk.java.net/jdk/jdk/file/cf6409153216/src/java.base/share/classes/java/lang/StringUTF16.java#l392 [2] public static int indexOfLatin1Unsafe(byte[] src, int srcCount, byte[] tgt, int tgtCount, int fromIndex) http://hg.openjdk.java.net/jdk/jdk/file/cf6409153216/src/java.base/share/classes/java/lang/StringUTF16.java#l440 Regards Patrick
RE: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10
Thank you all. Hi Thomas, please help push this: http://cr.openjdk.java.net/~qpzhang/8238380/webrev.03, I updated the reviewer list accordingly. Thanks. Regards Patrick -Original Message- From: core-libs-dev On Behalf Of Roger Riggs Sent: Thursday, February 6, 2020 10:52 PM To: core-libs-dev@openjdk.java.net Subject: Re: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10 +1, One is enough, but additional eyes are good too. On 2/5/20 10:35 PM, Patrick Zhang OS wrote: > Does this require one more “yes” from any other reviewer? > > And may I ask for any committer’s help to sponsor and push it (once > approved)? Thanks in advance. > > Regards > Patrick > > From: Thomas Stüfe > Sent: Wednesday, February 5, 2020 11:16 PM > To: Patrick Zhang OS > Cc: core-libs-dev > Subject: Re: RFR: JDK-8238380: > java.base/unix/native/libjava/childproc.c "multiple definition" link > errors with GCC10 > > All still good. > > ...Thomas > > On Wed, Feb 5, 2020 at 3:42 PM Patrick Zhang OS > mailto:patr...@os.amperecomputing.com>> wrote: > Thanks for your comments, Thomas. > > I have no accurate knowledge regarding why parentPathv was initially written > as a global var instead of a member in ChildStuff struct initialized in > jspawnhelper.c. However the suggested change might not be very > straight-forward as there are other references such as: > Java_java_lang_ProcessImpl_init(), spawnChild() in libjava\ProcessImpl_md.c, > etc. And this goes beyond my original intention to fix the build errors > purely, and try best to avoid any side effect. So I would leave the further > improvement to others, and keep it as-is. Thanks. > > Updated the change a bit (the header comments): > http://cr.openjdk.java.net/~qpzhang/8238380/webrev.02/ > > Regards > Patrick > > From: Thomas Stüfe > mailto:thomas.stu...@gmail.com>> > Sent: Wednesday, February 5, 2020 2:30 PM > To: Patrick Zhang OS > mailto:patr...@os.amperecomputing.com> > > > Cc: core-libs-dev > mailto:core-libs-dev@openjdk.java.net> > > > Subject: Re: RFR: JDK-8238380: > java.base/unix/native/libjava/childproc.c "multiple definition" link > errors with GCC10 > > Hi Patrick, > > You patch looks alright. > > But I wonder whether a cleaner solution would not be to add a parentPathv > pointer to the ChildStuff structure and pass it down to JDK_execvpe that way, > like we do for all other input arguments of that function. If it is an input > argument, seems strange to pass it as global variable. I leave that for > others to decide though, your patch certainly works. > > Cheers, Thomas > > On Tue, Feb 4, 2020 at 3:39 PM Patrick Zhang OS > mailto:patr...@os.amperecomputing.com>> wrote: > Hi > > I split the original patch into parts for corresponding function review, here > is the very simple change to java.base/unix/native/libjava/childproc.c and .h. > Could core-libs-dev group help review and sponsor this? thanks in advance. > > JBS: https://bugs.openjdk.java.net/browse/JDK-8238380 (a sub-task of > JDK-8235903) > Webrev: http://cr.openjdk.java.net/~qpzhang/8238380/webrev.01/ > Test: ran jtreg tier1, jdk built with GCC4.8.5, GCC9, and GCC10, no > regression found. I don’t have any specific function tests, so this is just a > smoke test. > > Regards > Patrick > > From: Martin Buchholz > mailto:marti...@google.com><mailto:martinrb@googl > e.com<mailto:marti...@google.com>>> > Sent: Monday, December 16, 2019 10:44 AM > To: Patrick Zhang OS > mailto:patr...@os.amperecomputing.com> > <mailto:patr...@os.amperecomputing.com<mailto:patrick@os.amperecomputi > ng.com>>>; net-dev > mailto:net-...@openjdk.java.net><mailto:net- > d...@openjdk.java.net<mailto:net-...@openjdk.java.net>>>; OpenJDK > mailto:security-...@openjdk.java.net> ailto:security-...@openjdk.java.net<mailto:security-...@openjdk.java.n > et>>> > Cc: core-libs-dev > mailto:core-libs-dev@openjdk.java.net> > <mailto:core-libs-dev@openjdk.java.net<mailto:core-libs-...@openjdk.ja > va.net>>> > Subject: Re: RFR: JDK-8235903: GCC default -fno-common exposes > "multiple definition" link errors > > forwarded to other teams for review. > > On Fri, Dec 13, 2019 at 3:14 AM Patrick Zhang OS > mailto:patr...@os.amperecomputing.com><mailto:patr...@os.amperecomputing.com<mailto:patr...@os.amperecomputing.com>>> > wrote: > Hi > > Please review this patch, if it should be reviewed by any group other than > core-libs, please help forward it. Thanks.
RE: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10
Does this require one more “yes” from any other reviewer? And may I ask for any committer’s help to sponsor and push it (once approved)? Thanks in advance. Regards Patrick From: Thomas Stüfe Sent: Wednesday, February 5, 2020 11:16 PM To: Patrick Zhang OS Cc: core-libs-dev Subject: Re: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10 All still good. ...Thomas On Wed, Feb 5, 2020 at 3:42 PM Patrick Zhang OS mailto:patr...@os.amperecomputing.com>> wrote: Thanks for your comments, Thomas. I have no accurate knowledge regarding why parentPathv was initially written as a global var instead of a member in ChildStuff struct initialized in jspawnhelper.c. However the suggested change might not be very straight-forward as there are other references such as: Java_java_lang_ProcessImpl_init(), spawnChild() in libjava\ProcessImpl_md.c, etc. And this goes beyond my original intention to fix the build errors purely, and try best to avoid any side effect. So I would leave the further improvement to others, and keep it as-is. Thanks. Updated the change a bit (the header comments): http://cr.openjdk.java.net/~qpzhang/8238380/webrev.02/ Regards Patrick From: Thomas Stüfe mailto:thomas.stu...@gmail.com>> Sent: Wednesday, February 5, 2020 2:30 PM To: Patrick Zhang OS mailto:patr...@os.amperecomputing.com>> Cc: core-libs-dev mailto:core-libs-dev@openjdk.java.net>> Subject: Re: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10 Hi Patrick, You patch looks alright. But I wonder whether a cleaner solution would not be to add a parentPathv pointer to the ChildStuff structure and pass it down to JDK_execvpe that way, like we do for all other input arguments of that function. If it is an input argument, seems strange to pass it as global variable. I leave that for others to decide though, your patch certainly works. Cheers, Thomas On Tue, Feb 4, 2020 at 3:39 PM Patrick Zhang OS mailto:patr...@os.amperecomputing.com>> wrote: Hi I split the original patch into parts for corresponding function review, here is the very simple change to java.base/unix/native/libjava/childproc.c and .h. Could core-libs-dev group help review and sponsor this? thanks in advance. JBS: https://bugs.openjdk.java.net/browse/JDK-8238380 (a sub-task of JDK-8235903) Webrev: http://cr.openjdk.java.net/~qpzhang/8238380/webrev.01/ Test: ran jtreg tier1, jdk built with GCC4.8.5, GCC9, and GCC10, no regression found. I don’t have any specific function tests, so this is just a smoke test. Regards Patrick From: Martin Buchholz mailto:marti...@google.com><mailto:marti...@google.com<mailto:marti...@google.com>>> Sent: Monday, December 16, 2019 10:44 AM To: Patrick Zhang OS mailto:patr...@os.amperecomputing.com><mailto:patr...@os.amperecomputing.com<mailto:patr...@os.amperecomputing.com>>>; net-dev mailto:net-...@openjdk.java.net><mailto:net-...@openjdk.java.net<mailto:net-...@openjdk.java.net>>>; OpenJDK mailto:security-...@openjdk.java.net><mailto:security-...@openjdk.java.net<mailto:security-...@openjdk.java.net>>> Cc: core-libs-dev mailto:core-libs-dev@openjdk.java.net><mailto:core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net>>> Subject: Re: RFR: JDK-8235903: GCC default -fno-common exposes "multiple definition" link errors forwarded to other teams for review. On Fri, Dec 13, 2019 at 3:14 AM Patrick Zhang OS mailto:patr...@os.amperecomputing.com><mailto:patr...@os.amperecomputing.com<mailto:patr...@os.amperecomputing.com>>> wrote: Hi Please review this patch, if it should be reviewed by any group other than core-libs, please help forward it. Thanks. JBS: https://bugs.openjdk.java.net/browse/JDK-8235903 Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/ A recent GCC patch (supposed to be in GCC 10) exposes a couple of "multiple definition" link errors when building the jdk tip. [PATCH] PR85678: Change default to -fno-common https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01847.html For example, the error message looks like: * For target support_native_java.base_libjava_BUILD_LIBJAVA_link: build/support/native/java.base/libjava/childproc.o:(.bss+0x0): multiple definition of `parentPathv' build/support/native/java.base/libjava/ProcessImpl_md.o:(.bss+0x0): first defined here collect2: error: ld returned 1 exit status This was not an issue because the original default -fcommon allowed "global variables defined without an initializer" be handled as COMMON symbols, so it would not warn the problem like "same variable is accidentally defined in more than one compilation unit". About -fcommon vs -fno-cmmon: https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-common
RE: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10
Thanks for your comments, Thomas. I have no accurate knowledge regarding why parentPathv was initially written as a global var instead of a member in ChildStuff struct initialized in jspawnhelper.c. However the suggested change might not be very straight-forward as there are other references such as: Java_java_lang_ProcessImpl_init(), spawnChild() in libjava\ProcessImpl_md.c, etc. And this goes beyond my original intention to fix the build errors purely, and try best to avoid any side effect. So I would leave the further improvement to others, and keep it as-is. Thanks. Updated the change a bit (the header comments): http://cr.openjdk.java.net/~qpzhang/8238380/webrev.02/ Regards Patrick From: Thomas Stüfe Sent: Wednesday, February 5, 2020 2:30 PM To: Patrick Zhang OS Cc: core-libs-dev Subject: Re: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10 Hi Patrick, You patch looks alright. But I wonder whether a cleaner solution would not be to add a parentPathv pointer to the ChildStuff structure and pass it down to JDK_execvpe that way, like we do for all other input arguments of that function. If it is an input argument, seems strange to pass it as global variable. I leave that for others to decide though, your patch certainly works. Cheers, Thomas On Tue, Feb 4, 2020 at 3:39 PM Patrick Zhang OS mailto:patr...@os.amperecomputing.com>> wrote: Hi I split the original patch into parts for corresponding function review, here is the very simple change to java.base/unix/native/libjava/childproc.c and .h. Could core-libs-dev group help review and sponsor this? thanks in advance. JBS: https://bugs.openjdk.java.net/browse/JDK-8238380 (a sub-task of JDK-8235903) Webrev: http://cr.openjdk.java.net/~qpzhang/8238380/webrev.01/ Test: ran jtreg tier1, jdk built with GCC4.8.5, GCC9, and GCC10, no regression found. I don’t have any specific function tests, so this is just a smoke test. Regards Patrick From: Martin Buchholz mailto:marti...@google.com><mailto:marti...@google.com<mailto:marti...@google.com>>> Sent: Monday, December 16, 2019 10:44 AM To: Patrick Zhang OS mailto:patr...@os.amperecomputing.com><mailto:patr...@os.amperecomputing.com<mailto:patr...@os.amperecomputing.com>>>; net-dev mailto:net-...@openjdk.java.net><mailto:net-...@openjdk.java.net<mailto:net-...@openjdk.java.net>>>; OpenJDK mailto:security-...@openjdk.java.net><mailto:security-...@openjdk.java.net<mailto:security-...@openjdk.java.net>>> Cc: core-libs-dev mailto:core-libs-dev@openjdk.java.net><mailto:core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net>>> Subject: Re: RFR: JDK-8235903: GCC default -fno-common exposes "multiple definition" link errors forwarded to other teams for review. On Fri, Dec 13, 2019 at 3:14 AM Patrick Zhang OS mailto:patr...@os.amperecomputing.com><mailto:patr...@os.amperecomputing.com<mailto:patr...@os.amperecomputing.com>>> wrote: Hi Please review this patch, if it should be reviewed by any group other than core-libs, please help forward it. Thanks. JBS: https://bugs.openjdk.java.net/browse/JDK-8235903 Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/ A recent GCC patch (supposed to be in GCC 10) exposes a couple of "multiple definition" link errors when building the jdk tip. [PATCH] PR85678: Change default to -fno-common https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01847.html For example, the error message looks like: * For target support_native_java.base_libjava_BUILD_LIBJAVA_link: build/support/native/java.base/libjava/childproc.o:(.bss+0x0): multiple definition of `parentPathv' build/support/native/java.base/libjava/ProcessImpl_md.o:(.bss+0x0): first defined here collect2: error: ld returned 1 exit status This was not an issue because the original default -fcommon allowed "global variables defined without an initializer" be handled as COMMON symbols, so it would not warn the problem like "same variable is accidentally defined in more than one compilation unit". About -fcommon vs -fno-cmmon: https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-common Moving forward, building jdk with latest versions of GCC will trigger this error. Specifying "--with-extra-cflags='-fcommon'" can make it work, but it just got things hidden again. In addition, -fcommon's behavior "is inconsistent with C++, and on many targets implies a speed and code size penalty on global variable references. It is mainly useful to enable legacy code to link without errors." Last, in case that other jdk developers would revisit this problem once again, I suggest fixing the error explicitly instead of using "-fcommon" Regards Patrick
RE: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10
Hi I split the original patch into parts for corresponding function review, here is the very simple change to java.base/unix/native/libjava/childproc.c and .h. Could core-libs-dev group help review and sponsor this? thanks in advance. JBS: https://bugs.openjdk.java.net/browse/JDK-8238380 (a sub-task of JDK-8235903) Webrev: http://cr.openjdk.java.net/~qpzhang/8238380/webrev.01/ Test: ran jtreg tier1, jdk built with GCC4.8.5, GCC9, and GCC10, no regression found. I don’t have any specific function tests, so this is just a smoke test. Regards Patrick From: Martin Buchholz mailto:marti...@google.com>> Sent: Monday, December 16, 2019 10:44 AM To: Patrick Zhang OS mailto:patr...@os.amperecomputing.com>>; net-dev mailto:net-...@openjdk.java.net>>; OpenJDK mailto:security-...@openjdk.java.net>> Cc: core-libs-dev mailto:core-libs-dev@openjdk.java.net>> Subject: Re: RFR: JDK-8235903: GCC default -fno-common exposes "multiple definition" link errors forwarded to other teams for review. On Fri, Dec 13, 2019 at 3:14 AM Patrick Zhang OS mailto:patr...@os.amperecomputing.com>> wrote: Hi Please review this patch, if it should be reviewed by any group other than core-libs, please help forward it. Thanks. JBS: https://bugs.openjdk.java.net/browse/JDK-8235903 Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/ A recent GCC patch (supposed to be in GCC 10) exposes a couple of "multiple definition" link errors when building the jdk tip. [PATCH] PR85678: Change default to -fno-common https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01847.html For example, the error message looks like: * For target support_native_java.base_libjava_BUILD_LIBJAVA_link: build/support/native/java.base/libjava/childproc.o:(.bss+0x0): multiple definition of `parentPathv' build/support/native/java.base/libjava/ProcessImpl_md.o:(.bss+0x0): first defined here collect2: error: ld returned 1 exit status This was not an issue because the original default -fcommon allowed "global variables defined without an initializer" be handled as COMMON symbols, so it would not warn the problem like "same variable is accidentally defined in more than one compilation unit". About -fcommon vs -fno-cmmon: https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-common Moving forward, building jdk with latest versions of GCC will trigger this error. Specifying "--with-extra-cflags='-fcommon'" can make it work, but it just got things hidden again. In addition, -fcommon's behavior "is inconsistent with C++, and on many targets implies a speed and code size penalty on global variable references. It is mainly useful to enable legacy code to link without errors." Last, in case that other jdk developers would revisit this problem once again, I suggest fixing the error explicitly instead of using "-fcommon" Regards Patrick
RE: RFR: JDK-8235903: GCC default -fno-common exposes "multiple definition" link errors
Thanks Martin. Hi net-dev, and/or security-dev Reviewers, Please help review and sponsor this patch if acceptable. It does not tend to bring any functionality changes, instead to propose an early fix, for the build (linking) errors with further toolchain (GCC 10). JBS: https://bugs.openjdk.java.net/browse/JDK-8235903 Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/ Regards Patrick From: Martin Buchholz Sent: Monday, December 16, 2019 10:44 AM To: Patrick Zhang OS ; net-dev ; OpenJDK Cc: core-libs-dev Subject: Re: RFR: JDK-8235903: GCC default -fno-common exposes "multiple definition" link errors forwarded to other teams for review. On Fri, Dec 13, 2019 at 3:14 AM Patrick Zhang OS mailto:patr...@os.amperecomputing.com>> wrote: Hi Please review this patch, if it should be reviewed by any group other than core-libs, please help forward it. Thanks. JBS: https://bugs.openjdk.java.net/browse/JDK-8235903 Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/ A recent GCC patch (supposed to be in GCC 10) exposes a couple of "multiple definition" link errors when building the jdk tip. [PATCH] PR85678: Change default to -fno-common https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01847.html For example, the error message looks like: * For target support_native_java.base_libjava_BUILD_LIBJAVA_link: build/support/native/java.base/libjava/childproc.o:(.bss+0x0): multiple definition of `parentPathv' build/support/native/java.base/libjava/ProcessImpl_md.o:(.bss+0x0): first defined here collect2: error: ld returned 1 exit status This was not an issue because the original default -fcommon allowed "global variables defined without an initializer" be handled as COMMON symbols, so it would not warn the problem like "same variable is accidentally defined in more than one compilation unit". About -fcommon vs -fno-cmmon: https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-common Moving forward, building jdk with latest versions of GCC will trigger this error. Specifying "--with-extra-cflags='-fcommon'" can make it work, but it just got things hidden again. In addition, -fcommon's behavior "is inconsistent with C++, and on many targets implies a speed and code size penalty on global variable references. It is mainly useful to enable legacy code to link without errors." Last, in case that other jdk developers would revisit this problem once again, I suggest fixing the error explicitly instead of using "-fcommon" Regards Patrick
RFR: JDK-8235903: GCC default -fno-common exposes "multiple definition" link errors
Hi Please review this patch, if it should be reviewed by any group other than core-libs, please help forward it. Thanks. JBS: https://bugs.openjdk.java.net/browse/JDK-8235903 Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/ A recent GCC patch (supposed to be in GCC 10) exposes a couple of "multiple definition" link errors when building the jdk tip. [PATCH] PR85678: Change default to -fno-common https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01847.html For example, the error message looks like: * For target support_native_java.base_libjava_BUILD_LIBJAVA_link: build/support/native/java.base/libjava/childproc.o:(.bss+0x0): multiple definition of `parentPathv' build/support/native/java.base/libjava/ProcessImpl_md.o:(.bss+0x0): first defined here collect2: error: ld returned 1 exit status This was not an issue because the original default -fcommon allowed "global variables defined without an initializer" be handled as COMMON symbols, so it would not warn the problem like "same variable is accidentally defined in more than one compilation unit". About -fcommon vs -fno-cmmon: https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-common Moving forward, building jdk with latest versions of GCC will trigger this error. Specifying "--with-extra-cflags='-fcommon'" can make it work, but it just got things hidden again. In addition, -fcommon's behavior "is inconsistent with C++, and on many targets implies a speed and code size penalty on global variable references. It is mainly useful to enable legacy code to link without errors." Last, in case that other jdk developers would revisit this problem once again, I suggest fixing the error explicitly instead of using "-fcommon" Regards Patrick
RE: RFR(trivial): 8222394: HashMap.compute() throws CME on an empty Map if clear() called concurrently
> [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060068.html Thanks for sharing this thread. I saw Pavel's comments: "I believe it should be incremented on an attempt to modify the collection, rather than on the effective result of that attempt" It can be true for bulk modifications like addAll, clear, while for single-entry modifications like put, remove, merge, etc. it depends, some are unconditional ++ and most are conditional ++. So the clarification should be not specific to clear or addAll, but at a common place (if possible) in a higher level javadoc for modCount, or for "fail fast". Regards Patrick -Original Message- From: Stuart Marks Sent: Thursday, May 9, 2019 5:01 AM To: Patrick Zhang OS Cc: core-libs-dev Subject: Re: RFR(trivial): 8222394: HashMap.compute() throws CME on an empty Map if clear() called concurrently On 5/4/19 12:21 AM, Patrick Zhang OS wrote: > Thank you very much for the detailed explanation and examples, which solved > most of my previous questions and confusion. Really appreciate that. I agree > with the opinion about "effort vs benefit". > > Re-thought my original tests concerning CMEs that passed with jdk8u but > failed with jdk11 (should be 9+, but I only checked LTS builds), I was > struggling between "fixing" the "problems" in jdk11 and "making jdk8 fail > similarly". However so far it looks these tests might be over strict, or > "verifying CMEs" itself might be an invalid (or unreliable) operation, > perhaps I should drop all of them. Lastly, a suggestion would be: adding more > comments for this in case anyone else would revisit it with similar > confusions, e.g. HashMap.clear. Great, I'm glad this was helpful. Regarding the tests, yes, I'm afraid those might be overly strict. The specification is admittedly quite loose in this area. This means that the exact behavior may vary from release to release. If a collection is optimized, for example, sometimes it's quite difficult to preserve the exact same behavior in all cases. It's for this reason we avoid specifying the exact behaviors around CME. Of course, when making any changes, we usually do try to preserve the general behavior, just not every specific edge case. I generally welcome code comments to improve clarity, but I'm not sure one is warranted for HashMap.clear(). The placement of modCount++ seems quite reasonable. There are other cases where the choice of conditional-vs-unconditional is made (see [1]) and I don't think we want to go sprinkling explanations of this all around the code. [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060068.html s'marks
RE: RFR(trivial): 8222394: HashMap.compute() throws CME on an empty Map if clear() called concurrently
Hi Stuart, Thank you very much for the detailed explanation and examples, which solved most of my previous questions and confusion. Really appreciate that. I agree with the opinion about "effort vs benefit". Re-thought my original tests concerning CMEs that passed with jdk8u but failed with jdk11 (should be 9+, but I only checked LTS builds), I was struggling between "fixing" the "problems" in jdk11 and "making jdk8 fail similarly". However so far it looks these tests might be over strict, or "verifying CMEs" itself might be an invalid (or unreliable) operation, perhaps I should drop all of them. Lastly, a suggestion would be: adding more comments for this in case anyone else would revisit it with similar confusions, e.g. HashMap.clear. Regards Patrick -Original Message- From: Stuart Marks Sent: Thursday, May 2, 2019 8:39 AM To: Patrick Zhang OS Cc: core-libs-dev Subject: Re: RFR(trivial): 8222394: HashMap.compute() throws CME on an empty Map if clear() called concurrently >> ...merely to serve as a discussion point about the policy for >> throwing ConcurrentModificationException? > Yes, for the time being, I want to see and welcome more ideas on this. > It seems to me that the policy for throwing CME here is not a unified > one, mostly based on experience and testing. Clear, compute, and > computeIfAbsent are more special as I described. OK. For reference, here are some of the words from the ConcurrentModificationException specification: [1] > This exception may be thrown by methods that have detected concurrent > modification of an object when such modification is not permissible. > > For example, it is not generally permissible for one thread to modify > a Collection while another thread is iterating over it. In general, > the results of the iteration are undefined under these circumstances. > Some Iterator implementations (including those of all the general > purpose collection implementations provided by the JRE) may choose to > throw this exception if this behavior is detected. Iterators that do > this are known as fail-fast iterators, as they fail quickly and > cleanly, rather that risking arbitrary, non-deterministic behavior at an > undetermined time in the future. > > Note that this exception does not always indicate that an object has > been concurrently modified by a different thread. If a single thread > issues a sequence of method invocations that violates the contract of > an object, the object may throw this exception. For example, if a > thread modifies a collection directly while it is iterating over the > collection with a fail-fast iterator, the iterator will throw this exception. > > Note that fail-fast behavior cannot be guaranteed as it is, generally > speaking, impossible to make any hard guarantees in the presence of > unsynchronized concurrent modification. Fail-fast operations throw > ConcurrentModificationException on a best-effort basis. Therefore, it > would be wrong to write a program that depended on this exception for > its > correctness: ConcurrentModificationException should be used only to > detect bugs. [1] https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/ConcurrentModificationException.html Similar words are repeated in several different locations around the specification, such as in the ArrayList and HashMap class specifications. I'm not entirely sure what your concerns are with ConcurrentModificationException (and the "fail-fast" concurrent modification policy), but let me discuss a few points. 1. throwing of CME is not guaranteed - "best effort" Unlike most Java specifications, the specification around CME is fairly indefinite. The wording is hedged -- "This exception may be thrown" This implies that CME might or might not be thrown, even in cases where one might expect it to be. It also says that CME is thrown on a "best effort" basis. This doesn't mean that the library makes the maximum possible effort to throw CME in every possible situation. Maybe "best effort" is somewhat misleading. Perhaps "reasonable" effort is more descriptive. For example, ArrayList keeps a modCount field and increments and checks it occasionally. No synchronization is done. If the ArrayList is modified by another thread, the update to modCount might not be visible to all threads, which might result in data corruption instead of a CME. One way to "fix" this would be to make access to modCount synchronized (or to make it volatile, or to make it an AtomicInteger or something) to improve the reliability of detecting concurrent modifications from other threads. This would add complexity to the code and also slow down common operations. Making this extra effo
RE: RFR(trivial): 8222394: HashMap.compute() throws CME on an empty Map if clear() called concurrently
> merely to serve as a discussion point about the policy for throwing
> ConcurrentModificationException?
Yes, for the time being, I want to see and welcome more ideas on this. It seems
to me that the policy for throwing CME here is not a unified one, mostly based
on experience and testing. Clear, compute, and computeIfAbsent are more special
as I described.
Regards
Patrick
-Original Message-
From: Stuart Marks
Sent: Thursday, April 25, 2019 7:48 AM
To: Patrick Zhang OS
Cc: core-libs-dev ; Martin Buchholz
Subject: Re: RFR(trivial): 8222394: HashMap.compute() throws CME on an empty
Map if clear() called concurrently
Hi Patrick,
I guess I'm not sure what you're proposing here. You've updated the patch; are
you proposing that this change be integrated?
Or are you posting code changes, not as a proposal to integrate, but merely to
serve as a discussion point about the policy for throwing
ConcurrentModificationException?
Either way (or something else) is fine; I just don't want to run off in one
direction if you had intended the other.
s'marks
On 4/15/19 3:51 AM, Patrick Zhang OS wrote:
> Hi Stuart,
>
> Thanks. I intentionally modified the map in remapping functions, just like
> those tests in
> http://hg.openjdk.java.net/jdk/jdk/file/00c0906bf4d1/test/jdk/java/util/Map/FunctionalCMEs.java.
> My original tests were: create two threads, modify or perform read-only
> operations in each, and verify those CMEs. There seems some inconsistencies:
> 1. clear() would modify the map completely, so it can be more 'defensible'
> (Martin mentioned so in Jira), while other modifying functions like
> put()/remove()/merge() are 'weaker', so ++modCount happens conditionally, say
> there would be really some structural modifications in map.
> 2. compute()/computeIfAbsent() throws CME almost unconditionally, while
> functions like forEach()/computeIfPresent()/iterator.next() are touching the
> map content practically so these are wrapped by if-clauses.
>
> So the concern is how to undertand "throw CME on a best-effort basis",
> if I want to try best to detect the risk of bugs in program,
> unconditionlly throwing CME can be the right way to go, e.g. do ++modCount in
> removeNode() without telling if the removing would really occur, if I want to
> make it more logically rigorous, we might need this:
> http://cr.openjdk.java.net/~qpzhang/8222394/webrev.02 for
> compute()/computeIfAbsent(). Centainly I know it has to afford the risk of
> missing bugs.
>
> Regards
> Patrick
>
> -----Original Message-
> From: Stuart Marks
> Sent: Saturday, April 13, 2019 4:15 AM
> To: Patrick Zhang OS
> Cc: core-libs-dev
> Subject: Re: RFR(trivial): 8222394: HashMap.compute() throws CME on an
> empty Map if clear() called concurrently
>
> [I'm about to leave for a week's vacation, so I won't be able to
> respond further until after I return]
>
> Hi Patrick,
>
> A bit of background about my thinking on this proposal.
>
> The Collections Framework has a set of weird edge cases ("tricky", as you
> say) that I'll describe as "state-dependent" behavior, where operations that
> seem illegal on their face might or might not throw an exception depending on
> the current state of the system. The current state potentially depends on
> everything that happened previously, including input to the program.
>
> As an example of this, consider the following code snippet:
>
> List list1 = Collections.emptyList();
> List list2 = getStringsFromSomewhere();
> list1.addAll(list2);
>
> What happens on the third line? The spec for Collections.emptyList() [1] says
> that the returned list is immutable. You might think, then, that addAll()
> will throw UnsupportedOperationException.
>
> What actually happens is, "it depends."
>
> If list2 has elements, then addAll() will throw UOE as expected.
> However, if
> list2 happens to be empty, then addAll() returns false, because nothing was
> added.
>
> To me, the code above clearly has a bug: it's calling a mutator method on an
> immutable collection. The only time this doesn't throw an exception is if
> list2 is empty, so it can't possibly have any useful effect in this case.
> Presumably getStringsFromSomewhere() will return some actual strings from
> time to time. If this happens rarely, we might put this code into production,
> and it might blow up unexpectedly when a different set of inputs causes list2
> to be nonempty.
>
> (Aside 1: the Java 9 unmodifiable collections throw UOE
> unconditionally, so
> List.of().addAll(List.of()) will throw UOE.)
>
> (Aside 2: even though it's inconsistent and arguably wrong, I don't
> think this behavi
RE: RFR: 8222334: java -Xss0 triggers StackOverflowError
Done. http://cr.openjdk.java.net/~qpzhang/8222334/webrev.05 Regards Patrick -Original Message- From: David Holmes Sent: Tuesday, April 16, 2019 6:34 PM To: Patrick Zhang OS Cc: core-libs-dev Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError Hi Patrick, On 16/04/2019 7:42 pm, Patrick Zhang OS wrote: > Hi David, > Please see my updates, the two '0' size test cases. I have run them with > jtreg on jdk13 + linux + x86/aarch64 systems respectively, all passed. > http://cr.openjdk.java.net/~qpzhang/8222334/webrev.04 Thanks. Please update copyright years. Also instead of this comment: It can verify the issue fixed in 8222334. Just add 8222334 to the @bug line. Thanks, David > Regards > Patrick > > -Original Message- > From: core-libs-dev On Behalf > Of Patrick Zhang OS > Sent: Tuesday, April 16, 2019 4:23 PM > To: David Holmes > Cc: core-libs-dev > Subject: RE: RFR: 8222334: java -Xss0 triggers StackOverflowError > > Sure I will add this, and fix the intention mentioned by Alan. > > Regards > Patrick > > -Original Message- > From: David Holmes > Sent: Tuesday, April 16, 2019 4:17 PM > To: Patrick Zhang OS > Cc: Alan Bateman ; core-libs-dev > > Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError > > Patrick, > > Sorry should have picked up on this earlier. Can you please update the > following two tests to add a test for '0' as appropriate: > > ./jdk/tools/launcher/TooSmallStackSize.java > ./hotspot/jtreg/runtime/Thread/TooSmallStackSize.java > > Thanks, > David > > On 16/04/2019 5:47 pm, David Holmes wrote: >> On 16/04/2019 5:40 pm, Alan Bateman wrote: >>> On 15/04/2019 08:48, David Holmes wrote: >>>> On 15/04/2019 5:34 pm, Patrick Zhang OS wrote: >>>>> Removed it. >>>>> http://cr.openjdk.java.net/~qpzhang/8222334/webrev.03/jdk.changese >>>>> t >>>>> >>>>> By the way, could you please sponsor to push it once approved? >>>>> thanks in advance. >>>> >>>> Sure - if the core-libs person who also reviews doesn't volunteer >>>> (hint hint ;-) ) >>> This looks okay to me too, I think we should fix the intention in >>> ContinueInNewThread while we are there so it matches the rest of the >>> file. >> >> Thanks Alan! I'll fix the indent before pushing. >> >> David >> - >> >>> -Alan
RE: RFR: 8222334: java -Xss0 triggers StackOverflowError
Hi David, Please see my updates, the two '0' size test cases. I have run them with jtreg on jdk13 + linux + x86/aarch64 systems respectively, all passed. http://cr.openjdk.java.net/~qpzhang/8222334/webrev.04 Regards Patrick -Original Message- From: core-libs-dev On Behalf Of Patrick Zhang OS Sent: Tuesday, April 16, 2019 4:23 PM To: David Holmes Cc: core-libs-dev Subject: RE: RFR: 8222334: java -Xss0 triggers StackOverflowError Sure I will add this, and fix the intention mentioned by Alan. Regards Patrick -Original Message- From: David Holmes Sent: Tuesday, April 16, 2019 4:17 PM To: Patrick Zhang OS Cc: Alan Bateman ; core-libs-dev Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError Patrick, Sorry should have picked up on this earlier. Can you please update the following two tests to add a test for '0' as appropriate: ./jdk/tools/launcher/TooSmallStackSize.java ./hotspot/jtreg/runtime/Thread/TooSmallStackSize.java Thanks, David On 16/04/2019 5:47 pm, David Holmes wrote: > On 16/04/2019 5:40 pm, Alan Bateman wrote: >> On 15/04/2019 08:48, David Holmes wrote: >>> On 15/04/2019 5:34 pm, Patrick Zhang OS wrote: >>>> Removed it. >>>> http://cr.openjdk.java.net/~qpzhang/8222334/webrev.03/jdk.changeset >>>> >>>> By the way, could you please sponsor to push it once approved? >>>> thanks in advance. >>> >>> Sure - if the core-libs person who also reviews doesn't volunteer >>> (hint hint ;-) ) >> This looks okay to me too, I think we should fix the intention in >> ContinueInNewThread while we are there so it matches the rest of the >> file. > > Thanks Alan! I'll fix the indent before pushing. > > David > - > >> -Alan
RE: RFR: 8222334: java -Xss0 triggers StackOverflowError
Sure I will add this, and fix the intention mentioned by Alan. Regards Patrick -Original Message- From: David Holmes Sent: Tuesday, April 16, 2019 4:17 PM To: Patrick Zhang OS Cc: Alan Bateman ; core-libs-dev Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError Patrick, Sorry should have picked up on this earlier. Can you please update the following two tests to add a test for '0' as appropriate: ./jdk/tools/launcher/TooSmallStackSize.java ./hotspot/jtreg/runtime/Thread/TooSmallStackSize.java Thanks, David On 16/04/2019 5:47 pm, David Holmes wrote: > On 16/04/2019 5:40 pm, Alan Bateman wrote: >> On 15/04/2019 08:48, David Holmes wrote: >>> On 15/04/2019 5:34 pm, Patrick Zhang OS wrote: >>>> Removed it. >>>> http://cr.openjdk.java.net/~qpzhang/8222334/webrev.03/jdk.changeset >>>> >>>> By the way, could you please sponsor to push it once approved? >>>> thanks in advance. >>> >>> Sure - if the core-libs person who also reviews doesn't volunteer >>> (hint hint ;-) ) >> This looks okay to me too, I think we should fix the intention in >> ContinueInNewThread while we are there so it matches the rest of the >> file. > > Thanks Alan! I'll fix the indent before pushing. > > David > - > >> -Alan
RE: RFR(trivial): 8222394: HashMap.compute() throws CME on an empty Map if clear() called concurrently
Hi Stuart,
Thanks. I intentionally modified the map in remapping functions, just like
those tests in
http://hg.openjdk.java.net/jdk/jdk/file/00c0906bf4d1/test/jdk/java/util/Map/FunctionalCMEs.java.
My original tests were: create two threads, modify or perform read-only
operations in each, and verify those CMEs. There seems some inconsistencies:
1. clear() would modify the map completely, so it can be more 'defensible'
(Martin mentioned so in Jira), while other modifying functions like
put()/remove()/merge() are 'weaker', so ++modCount happens conditionally, say
there would be really some structural modifications in map.
2. compute()/computeIfAbsent() throws CME almost unconditionally, while
functions like forEach()/computeIfPresent()/iterator.next() are touching the
map content practically so these are wrapped by if-clauses.
So the concern is how to undertand "throw CME on a best-effort basis",
if I want to try best to detect the risk of bugs in program, unconditionlly
throwing CME can be the right way to go, e.g. do ++modCount in removeNode()
without telling if the removing would really occur,
if I want to make it more logically rigorous, we might need this:
http://cr.openjdk.java.net/~qpzhang/8222394/webrev.02 for
compute()/computeIfAbsent(). Centainly I know it has to afford the risk of
missing bugs.
Regards
Patrick
-Original Message-
From: Stuart Marks
Sent: Saturday, April 13, 2019 4:15 AM
To: Patrick Zhang OS
Cc: core-libs-dev
Subject: Re: RFR(trivial): 8222394: HashMap.compute() throws CME on an empty
Map if clear() called concurrently
[I'm about to leave for a week's vacation, so I won't be able to respond
further until after I return]
Hi Patrick,
A bit of background about my thinking on this proposal.
The Collections Framework has a set of weird edge cases ("tricky", as you say)
that I'll describe as "state-dependent" behavior, where operations that seem
illegal on their face might or might not throw an exception depending on the
current state of the system. The current state potentially depends on
everything that happened previously, including input to the program.
As an example of this, consider the following code snippet:
List list1 = Collections.emptyList();
List list2 = getStringsFromSomewhere();
list1.addAll(list2);
What happens on the third line? The spec for Collections.emptyList() [1] says
that the returned list is immutable. You might think, then, that addAll() will
throw UnsupportedOperationException.
What actually happens is, "it depends."
If list2 has elements, then addAll() will throw UOE as expected. However, if
list2 happens to be empty, then addAll() returns false, because nothing was
added.
To me, the code above clearly has a bug: it's calling a mutator method on an
immutable collection. The only time this doesn't throw an exception is if list2
is empty, so it can't possibly have any useful effect in this case.
Presumably getStringsFromSomewhere() will return some actual strings from time
to time. If this happens rarely, we might put this code into production, and it
might blow up unexpectedly when a different set of inputs causes list2 to be
nonempty.
(Aside 1: the Java 9 unmodifiable collections throw UOE unconditionally, so
List.of().addAll(List.of()) will throw UOE.)
(Aside 2: even though it's inconsistent and arguably wrong, I don't think this
behavior of emptyList() should be changed, for compatibility reasons.)
I think you can see the analogy with HashMap.compute(). The cases from the
tests essentially do this:
var m = new HashMap();
// possible modifications to m
m.compute(someKey, (k, v) -> {
m.clear();
return someValue;
});
Looking at this code, and not knowing the state of m, it seems to me it has a
bug. The spec for compute() [2] says "The remapping function should not modify
this map during computation" and there's a call to clear() right there. Indeed,
the current code always throws ConcurrentModificationException.
You're proposing that it not throw CME in the case where m is empty, when
clear() has no effect. This is similar to the case above; if m is often empty,
then this code appears to "work". But if the program's input were to change and
m becomes non-empty, it'll throw CME unexpectedly. On the other hand, it would
allow clear() in exactly the cases where it has no effect.
Overall I don't see that the system is improved by this change. It allows a
particular operation only when it has no effect (thus adding no value), and it
increases the risk of bugs in programs going undetected.
s'marks
[1]
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Collections.html#emptyList()
[2]
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Map.html#compute(K,java.util.function.BiFunction)
On 4/12/19 2:46 AM, Patrick Zhang OS wrote:
> Cr
RE: RFR: 8222334: java -Xss0 triggers StackOverflowError
Removed it. http://cr.openjdk.java.net/~qpzhang/8222334/webrev.03/jdk.changeset By the way, could you please sponsor to push it once approved? thanks in advance. Regards Patrick -Original Message- From: David Holmes Sent: Monday, April 15, 2019 2:33 PM To: Patrick Zhang OS ; core-libs-dev Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError Hi Patrick, On 15/04/2019 3:42 pm, Patrick Zhang OS wrote: > Hi David, > > Many thanks, I integrated your updates into the new patch. Thanks. My only further comment is to not have: 947 * See JDK-8222334 for details Cross references from code to bug reports should be very rare and I don't think this one warrants it. No need to see updated webrev in that case. Cheers, David > >> I think STACK_SIZE_MINIMUM is an empirical value, NOT suitable for 'all' >> platforms, at least not that safe, for example, a tricky experiment is: >> create the initial thread with 320K, and have later VM inner threads created >> with 448K, on my aarch64 system, StackOverflowError would be thrown. >> Fortunately this probably would not occur in real cases, as -Xss60k, >> -Xss320k, etc. can be stopped by these if-clauses. I changed "all >> platforms" to "most platforms". >> "only used for windows" might ambiguously mean "GetDefaultJavaVMInitArgs >> returns 0 for windows only" or "only windows supports 0". I updated it as >> "for example, Windows" > > http://cr.openjdk.java.net/~qpzhang/8222334/webrev.02/ > > Regards > Patrick > > -Original Message- > From: David Holmes > Sent: Monday, April 15, 2019 6:55 AM > To: Patrick Zhang OS ; core-libs-dev > > Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError > > Hi Patrick, > > Please see: > > http://cr.openjdk.java.net/~dholmes/8222334/webrev/ > > for my suggested updates to the commentary. Note that > GetDefaultJavaVMInitArgs returns the build-time default stack sizes and so > will only return 0 (for "use the system default") on Windows. It is not > affected by -XX:ThreadStackSize=n as that only gets processed when the JVM is > actually loaded. > > Thanks, > David > > On 12/04/2019 6:11 pm, David Holmes wrote: >> Hi Patrick, >> >> First apologies that it took me so long to get my head around this. >> :) Let me summarise the problem as I see it. >> >> The launcher specifies no particular semantics for -Xss0, to it 0 is >> just a very small size. However the VM maps -Xss to >> -XX:ThreadStackSize and for it 0 means "use the platform default stack size". >> >> The launcher examines -Xss because it needs to use it to define the >> stacksize for the initial thread created to launch the VM. >> >> The VM examines -Xss to see what stacksize to use for subsequently >> created threads and it treats 0 as 'use the platform default' and it >> otherwise checks the value against some hardcoded minimums and >> reports an error if it is too small. >> >> The initial thread that loads the VM needs sufficient stack to be >> able to process things to the point where it can determine that the >> requested stacksize is too small and report the error. The value of >> the minimum stack is hardcoded into the launcher, as >> STACK_SIZE_MINIMUM (64KB). If the -Xss value is less than that then it gets >> set to that. >> >> If no -Xss is specified then the launcher asks the VM for a >> reasonable value to use for the stacksize of the initial thread (typically >> 1MB). >> >> The problem arises with -Xss0 because this causes the launcher to set >> an initial thread stacksize of STACK_SIZE_MINIMUM, but the VM sees >> this as "use the default" and so does not reject it and tries to >> continue with VM initialization. That can't succeed as we only have a >> tiny STACK_SIZE_MINIMUM stack and so we get StackOverflowError (or >> fail an assert in debug builds). >> >> So the solution, as Patrick proposes, is to treat -Xss0 in the >> launcher as-if -Xss has not been set and so use the VM suggested >> default for the initial thread's stacksize. >> >> So I agree with the functional change here, but have some alternate >> suggestions for additional commentary. Unfortunately I have to step >> away at the moment (its Friday night) so will send that later - sorry. >> >> Thanks, >> David >> >> On 12/04/2019 5:51 pm, Patrick Zhang OS wrote: >>> Moved this to core-libs-dev for review, thanks. >>> >>> Dropped and bcc'ed jdk-dev and jdk
RE: RFR: 8222334: java -Xss0 triggers StackOverflowError
Hi David, Many thanks, I integrated your updates into the new patch. > I think STACK_SIZE_MINIMUM is an empirical value, NOT suitable for 'all' > platforms, at least not that safe, for example, a tricky experiment is: > create the initial thread with 320K, and have later VM inner threads created > with 448K, on my aarch64 system, StackOverflowError would be thrown. > Fortunately this probably would not occur in real cases, as -Xss60k, > -Xss320k, etc. can be stopped by these if-clauses. I changed "all platforms" > to "most platforms". > "only used for windows" might ambiguously mean "GetDefaultJavaVMInitArgs > returns 0 for windows only" or "only windows supports 0". I updated it as > "for example, Windows" http://cr.openjdk.java.net/~qpzhang/8222334/webrev.02/ Regards Patrick -Original Message- From: David Holmes Sent: Monday, April 15, 2019 6:55 AM To: Patrick Zhang OS ; core-libs-dev Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError Hi Patrick, Please see: http://cr.openjdk.java.net/~dholmes/8222334/webrev/ for my suggested updates to the commentary. Note that GetDefaultJavaVMInitArgs returns the build-time default stack sizes and so will only return 0 (for "use the system default") on Windows. It is not affected by -XX:ThreadStackSize=n as that only gets processed when the JVM is actually loaded. Thanks, David On 12/04/2019 6:11 pm, David Holmes wrote: > Hi Patrick, > > First apologies that it took me so long to get my head around this. :) > Let me summarise the problem as I see it. > > The launcher specifies no particular semantics for -Xss0, to it 0 is > just a very small size. However the VM maps -Xss to > -XX:ThreadStackSize and for it 0 means "use the platform default stack size". > > The launcher examines -Xss because it needs to use it to define the > stacksize for the initial thread created to launch the VM. > > The VM examines -Xss to see what stacksize to use for subsequently > created threads and it treats 0 as 'use the platform default' and it > otherwise checks the value against some hardcoded minimums and reports > an error if it is too small. > > The initial thread that loads the VM needs sufficient stack to be able > to process things to the point where it can determine that the > requested stacksize is too small and report the error. The value of > the minimum stack is hardcoded into the launcher, as > STACK_SIZE_MINIMUM (64KB). If the -Xss value is less than that then it gets > set to that. > > If no -Xss is specified then the launcher asks the VM for a reasonable > value to use for the stacksize of the initial thread (typically 1MB). > > The problem arises with -Xss0 because this causes the launcher to set > an initial thread stacksize of STACK_SIZE_MINIMUM, but the VM sees > this as "use the default" and so does not reject it and tries to > continue with VM initialization. That can't succeed as we only have a > tiny STACK_SIZE_MINIMUM stack and so we get StackOverflowError (or > fail an assert in debug builds). > > So the solution, as Patrick proposes, is to treat -Xss0 in the > launcher as-if -Xss has not been set and so use the VM suggested > default for the initial thread's stacksize. > > So I agree with the functional change here, but have some alternate > suggestions for additional commentary. Unfortunately I have to step > away at the moment (its Friday night) so will send that later - sorry. > > Thanks, > David > > On 12/04/2019 5:51 pm, Patrick Zhang OS wrote: >> Moved this to core-libs-dev for review, thanks. >> >> Dropped and bcc'ed jdk-dev and jdk-updates-dev. >> >> Regards >> Patrick >> >> -Original Message- >> From: David Holmes >> Sent: Friday, April 12, 2019 3:43 PM >> To: Patrick Zhang OS ; >> jdk-...@openjdk.java.net >> Cc: jdk-updates-...@openjdk.java.net >> Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError >> >> Hi Patrick, >> >> Please takes this to core-libs-dev for review. >> >> Thanks, >> David >> >> On 12/04/2019 5:24 pm, Patrick Zhang OS wrote: >>> Hi, >>> >>> Please review this patch. >>> >>> The problem is that the launcher does a check on the input -Xss and >>> ensure it >=64K for the initial thread, while vm has another >>> function to determine whether the input stack size is big enough to >>> future threads, such as cgc_thread, vm_thread, java_thead etc. >>> However if -Xss0, the initial thread is created with stack size 64K, >>> while others us
RE: RFR(trivial): 8222394: HashMap.compute() throws CME on an empty Map if clear() called concurrently
Created a ticket to track it, welcome any comments. Thanks.
JBS https://bugs.openjdk.java.net/browse/JDK-8222394
Webrev: http://cr.openjdk.java.net/~qpzhang/map.clear/webrev.01
Regards
Patrick
-Original Message-
From: core-libs-dev On Behalf Of
Patrick Zhang OS
Sent: Saturday, March 30, 2019 1:34 PM
To: core-libs-dev
Subject: ConcurrentModificationException thrown by HashMap.compute() operating
on an empty Map, expected/unexpected?
Hi,
Here I have a case simplified from a practical issue that throws
ConcurrentModificationException (CME) unexpectedly (I think). [0] creates a
HashMap, keeps it empty, and calls m.computeIfAbsent() or m.compute(), in which
a "sneaky" m.clear() occurs, some of the test cases throw CME although there
were no "structural" changes in fact. (A structural modification is defined as
"any operation that adds or deletes one or more mappings...").
This case cannot be reproduced with jdk8u, while jdk9 and beyond can, after the
bug [1] got fixed for computeIfAbsent() concurrent co-modification issues. A
couple of test cases [2] were introduced at that time, and the focus was to
verify the behaviors at resizing, while empty maps were not tested.
A possible "fix" for this issue is to move the unconditional "modCount++" [3]
into the if-clause, which indicates that a "structural" change would be
happening indeed.
public void clear() {
Node[] tab;
- modCount++;
if ((tab = table) != null && size > 0) {
+modCount++;
size = 0;
for (int i = 0; i < tab.length; ++i)
tab[i] = null;
}
}
Therefore, a dilemma here is "modCount++ before-if-clause but overkills some
cases" vs. "modCount++ into-if-clause but weakens the CME checking
potentially". I want to make balance regarding how to "throw CME on a
best-effort basis" more appropriately. Any suggestion?
I understand that CME here in HashMap.java cannot guarantee much and may be
only for debugging purpose, any concurrent modification needs to be typically
accomplished by synchronizing on some object that naturally encapsulates the
map. So the mentioned issue is a just a tricky case.
[0]http://cr.openjdk.java.net/~qpzhang/map.clear/webrev.01/test/jdk/java/util/concurrent/ConcurrentMap/ConcurrentModification.java.udiff.html
[1]https://bugs.openjdk.java.net/browse/JDK-8071667
[2]http://hg.openjdk.java.net/jdk/jdk/file/5a9d780eb9dd/test/jdk/java/util/Map/FunctionalCMEs.java
[3]http://hg.openjdk.java.net/jdk/jdk/file/1042cac8bc2a/src/java.base/share/classes/java/util/HashMap.java#l860
Regards
Patrick
RE: RFR: 8222334: java -Xss0 triggers StackOverflowError
Moved this to core-libs-dev for review, thanks. Dropped and bcc'ed jdk-dev and jdk-updates-dev. Regards Patrick -Original Message- From: David Holmes Sent: Friday, April 12, 2019 3:43 PM To: Patrick Zhang OS ; jdk-...@openjdk.java.net Cc: jdk-updates-...@openjdk.java.net Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError Hi Patrick, Please takes this to core-libs-dev for review. Thanks, David On 12/04/2019 5:24 pm, Patrick Zhang OS wrote: > Hi, > > Please review this patch. > > The problem is that the launcher does a check on the input -Xss and > ensure it >=64K for the initial thread, while vm has another function > to determine whether the input stack size is big enough to future > threads, such as cgc_thread, vm_thread, java_thead etc. However if > -Xss0, the initial thread is created with stack size 64K, while others > use hotspot/system default sizes, which would trigger > StackOverflowError. We could either fine tune the threshold 64K to be > a bigger one, or have the initial thread created with system defaults > that may be what the user expects. This patch chooses the second > solution, to avoid potential side-effect of the first. > > This can be reproduced with 10, 11, 12 too, so I cc'ed jdk-updates-dev here. > > More details please refer to the ticket. > > JBS: https://bugs.openjdk.java.net/browse/JDK-8222334 > > Webrev: http://cr.openjdk.java.net/~qpzhang/8222334/webrev.01/ > > Thanks for David's comments in Jira. > > Regards > > Patrick >
ConcurrentModificationException thrown by HashMap.compute() operating on an empty Map, expected/unexpected?
Hi,
Here I have a case simplified from a practical issue that throws
ConcurrentModificationException (CME) unexpectedly (I think). [0] creates a
HashMap, keeps it empty, and calls m.computeIfAbsent() or m.compute(), in which
a "sneaky" m.clear() occurs, some of the test cases throw CME although there
were no "structural" changes in fact. (A structural modification is defined as
"any operation that adds or deletes one or more mappings...").
This case cannot be reproduced with jdk8u, while jdk9 and beyond can, after the
bug [1] got fixed for computeIfAbsent() concurrent co-modification issues. A
couple of test cases [2] were introduced at that time, and the focus was to
verify the behaviors at resizing, while empty maps were not tested.
A possible "fix" for this issue is to move the unconditional "modCount++" [3]
into the if-clause, which indicates that a "structural" change would be
happening indeed.
public void clear() {
Node[] tab;
- modCount++;
if ((tab = table) != null && size > 0) {
+modCount++;
size = 0;
for (int i = 0; i < tab.length; ++i)
tab[i] = null;
}
}
Therefore, a dilemma here is "modCount++ before-if-clause but overkills some
cases" vs. "modCount++ into-if-clause but weakens the CME checking
potentially". I want to make balance regarding how to "throw CME on a
best-effort basis" more appropriately. Any suggestion?
I understand that CME here in HashMap.java cannot guarantee much and may be
only for debugging purpose, any concurrent modification needs to be typically
accomplished by synchronizing on some object that naturally encapsulates the
map. So the mentioned issue is a just a tricky case.
[0]http://cr.openjdk.java.net/~qpzhang/map.clear/webrev.01/test/jdk/java/util/concurrent/ConcurrentMap/ConcurrentModification.java.udiff.html
[1]https://bugs.openjdk.java.net/browse/JDK-8071667
[2]http://hg.openjdk.java.net/jdk/jdk/file/5a9d780eb9dd/test/jdk/java/util/Map/FunctionalCMEs.java
[3]http://hg.openjdk.java.net/jdk/jdk/file/1042cac8bc2a/src/java.base/share/classes/java/util/HashMap.java#l860
Regards
Patrick
