Re: [11] RFR: 8181157: CLDR Timezone name fallback implementation
Hi Sherman, thanks for the review. On 4/23/18 1:06 PM, Xueming Shen wrote: Naoto, Here some comments (1) CLDRTimeZoneNameProviderImpl.java: Ln#58: to use Stream.toArray(String[]::new) ? no sure which one is faster Ln#155-160: is it worth considering to check all possible empty slots in "names" here (from index_std_long to index_std_short?) to avoid check/load "compact" multiple times? it appears doable for deriveFallbackNames() but I'm not sure about the invocation at #ln100, so a "?". (2) CLDRConverter.java: Ln#674: (not in updated code) why update the "local" jreMetaMap? just wonder it'd better to use Optional.ifPresentOrElse(...) here Ln:707: (don't know which one is better though :-)) .toMap(Map.Entry::getKey, Map.Entry::getValue); Addressed all of the above suggestions. Unnecessary jreMetaMap update was a good catch! (3) LocaleResources.java just wanted to confirm the "locale.resources.debug" is something we wanted to be a public interface or not? Eventually I'd be public (8057075), but it is a private use for the time being. Here's the updated webrev: http://cr.openjdk.java.net/~naoto/8181157/webrev.06/ Naoto Thanks, Sherman On 04/17/2018 02:28 PM, Naoto Sato wrote: Hello, Please review the changes to the following issue: https://bugs.openjdk.java.net/browse/JDK-8181157 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8181157/webrev.05/ This RFE is to implement CLDR time zone names fallback mechanism [1]. Prior to this fix, time zone names are substituted with English names. With this change, it is generated according to the logic defined in TR 35. Here are the highlight of the changes: CLDRConverter: - CLDRConverter has changed to not substitute/invent time zone display names, except English bundle for compatibility & runtime performance. - For English bundle, copy over COMPAT's names as before. - CLDRConverer now parses regionFormat/gmtZeroFormat/exemplarCity CLDR provider: - CLDRTimeZoneNameProviderImpl is introduced to generate fallback names at runtime. - If COMPAT provider is present, use it also as a fallback, before the last resort (GMT offset format) Naoto [1] http://www.unicode.org/reports/tr35/tr35-dates.html#Time_Zone_Names
Re: [PATCH] minor regex cleanup: use switch for enum
On Mon, Apr 23, 2018 at 5:05 PM Xueming Shen wrote: > > I would assume in case of an exception thrown from > appendExpandedReplacement() we don't > want "text" to be pushed into the "sb". > > -sherman > Perhaps. Though the behavior under exception is undefined and this function is probably primarily used though the replaceAll API, which wouldn’t return the intermediate sb under the case of exception. My reading of the blame was the temp StringBuilder was an artifact of the api previously using StringBuffer externally. >
Re: [PATCH] minor regex cleanup: use switch for enum
On Mon, Apr 23, 2018 at 5:18 PM David Lloyd wrote: > FWIW I strongly doubt this will improve performance; probably the > opposite in fact, as IIRC an enum switch generates an extra class > (though perhaps this has changed). The original code was quite > compact and utilized identity comparisons, and given there are only > three alternatives it probably was able to exploit branch prediction > as well (if such a thing even matters in this context). Well, there are enum switches on the same enum elsewhere in the class, so should those instead be replaced by if checks? A larger change could remove this branch entirely, with different classes for each of the types, which are known during compile.
Re: [PATCH] minor regex cleanup: use switch for enum
FWIW I strongly doubt this will improve performance; probably the
opposite in fact, as IIRC an enum switch generates an extra class
(though perhaps this has changed). The original code was quite
compact and utilized identity comparisons, and given there are only
three alternatives it probably was able to exploit branch prediction
as well (if such a thing even matters in this context).
I'm not a reviewer but this change seems pointless without supporting perf data.
On Mon, Apr 23, 2018 at 4:05 PM, Xueming Shen wrote:
>
> I would assume in case of an exception thrown from
> appendExpandedReplacement() we don't
> want "text" to be pushed into the "sb".
>
> -sherman
>
> On 4/23/18, 1:49 PM, Isaac Levy wrote:
>>
>> Thanks. Another small perf patch below -- maybe we can combine. Avoids a
>> StringBuilder allocation:
>>
>> --- a/src/java.base/share/classes/java/util/regex/Matcher.java
>> +++ b/src/java.base/share/classes/java/util/regex/Matcher.java
>> @@ -993,13 +993,11 @@
>> public Matcher appendReplacement(StringBuilder sb, String replacement)
>> {
>> // If no match, return error
>> if (first < 0)
>> throw new IllegalStateException("No match available");
>> -StringBuilder result = new StringBuilder();
>> -appendExpandedReplacement(replacement, result);
>> // Append the intervening text
>> sb.append(text, lastAppendPosition, first);
>> // Append the match substitution
>> +appendExpandedReplacement(replacement, sb);
>> -sb.append(result);
>>
>>
>> On Mon, Apr 23, 2018 at 4:31 PM, Xueming Shen > > wrote:
>> > this looks fine.
>> >
>> > -sherman
>
>
--
- DML
Re: [PATCH] Add compareToUnsigned to java.nio.*Buffer
Hi Robert, Thanks. The addition of mismatch is reasonable. I am unsure about compareToUnsigned, since we are trying to be a conservative about what we add to Buffer. I’ll take a closer look but it may take a little while to get around to it. Would you mind logging a web bug and attaching your patch? The bug will get triaged and make its way to me, then i won’t forget and you can track it. Thanks, Paul. > On Apr 23, 2018, at 7:23 AM, Robert Stupp wrote: > > The proposed patch is an improvement for Byte/Short/Int/LongBuffer classes to > add compareToUnsigned() that works like compareTo() but treats the values as > unsigned to leverage the vectorized mismatch implementations, which are not > publicly accessible. > > In addition to that, it adds mismatch() for all *Buffer classes. > > Both new methods basically expose all the goodness of > jdk.internal.util.ArraysSupport to *Buffer. > > The patch includes adoptions to the existing unit tests as well. > > Robert > > -- > Robert Stupp > @snazy > >
Re: [PATCH] minor regex cleanup: use switch for enum
I would assume in case of an exception thrown from
appendExpandedReplacement() we don't
want "text" to be pushed into the "sb".
-sherman
On 4/23/18, 1:49 PM, Isaac Levy wrote:
Thanks. Another small perf patch below -- maybe we can combine. Avoids
a StringBuilder allocation:
--- a/src/java.base/share/classes/java/util/regex/Matcher.java
+++ b/src/java.base/share/classes/java/util/regex/Matcher.java
@@ -993,13 +993,11 @@
public Matcher appendReplacement(StringBuilder sb, String
replacement) {
// If no match, return error
if (first < 0)
throw new IllegalStateException("No match available");
-StringBuilder result = new StringBuilder();
-appendExpandedReplacement(replacement, result);
// Append the intervening text
sb.append(text, lastAppendPosition, first);
// Append the match substitution
+appendExpandedReplacement(replacement, sb);
-sb.append(result);
On Mon, Apr 23, 2018 at 4:31 PM, Xueming Shen > wrote:
> this looks fine.
>
> -sherman
Re: [PATCH] minor regex cleanup: use switch for enum
Thanks. Another small perf patch below -- maybe we can combine. Avoids a
StringBuilder allocation:
--- a/src/java.base/share/classes/java/util/regex/Matcher.java
+++ b/src/java.base/share/classes/java/util/regex/Matcher.java
@@ -993,13 +993,11 @@
public Matcher appendReplacement(StringBuilder sb, String replacement) {
// If no match, return error
if (first < 0)
throw new IllegalStateException("No match available");
-StringBuilder result = new StringBuilder();
-appendExpandedReplacement(replacement, result);
// Append the intervening text
sb.append(text, lastAppendPosition, first);
// Append the match substitution
+appendExpandedReplacement(replacement, sb);
-sb.append(result);
On Mon, Apr 23, 2018 at 4:31 PM, Xueming Shen
wrote:
> this looks fine.
>
> -sherman
Re: [PATCH] minor regex cleanup: use switch for enum
this looks fine.
-sherman
On 4/23/18, 12:26 PM, Isaac Levy wrote:
ping?
Isaac
On Wed, Apr 18, 2018 at 2:58 PM, Isaac Levy wrote:
Hi,
Minor improvement in readability (and probably perf) for Pattern. Switch
is more consistent with the rest of the impl and the resulting tableswitch
avoids a comparison for possessives.
-Isaac
--- a/src/java.base/share/classes/java/util/regex/Pattern.java
+++ b/src/java.base/share/classes/java/util/regex/Pattern.java
@@ -4356,7 +4356,9 @@
-if (type == Qtype.GREEDY)
+switch (type) {
+case GREEDY:
return match0(matcher, i, j, seq);
-else if (type == Qtype.LAZY)
+case LAZY:
return match1(matcher, i, j, seq);
-else
+default:
return match2(matcher, i, j, seq);
+}
@@ -4527,7 +4529,10 @@
-if (type == Qtype.GREEDY) {
+switch (type) {
+case GREEDY:
ret = match0(matcher, i, cmin, seq);
+break;
-} else if (type == Qtype.LAZY) {
+case LAZY:
ret = match1(matcher, i, cmin, seq);
+break;
-} else {
+default:
ret = match2(matcher, i, cmin, seq);
}
Re: [11] RFR: 8181157: CLDR Timezone name fallback implementation
Naoto, Here some comments (1) CLDRTimeZoneNameProviderImpl.java: Ln#58: to use Stream.toArray(String[]::new) ? no sure which one is faster Ln#155-160: is it worth considering to check all possible empty slots in "names" here (from index_std_long to index_std_short?) to avoid check/load "compact" multiple times? it appears doable for deriveFallbackNames() but I'm not sure about the invocation at #ln100, so a "?". (2) CLDRConverter.java: Ln#674: (not in updated code) why update the "local" jreMetaMap? just wonder it'd better to use Optional.ifPresentOrElse(...) here Ln:707: (don't know which one is better though :-)) .toMap(Map.Entry::getKey, Map.Entry::getValue); (3) LocaleResources.java just wanted to confirm the "locale.resources.debug" is something we wanted to be a public interface or not? Thanks, Sherman On 04/17/2018 02:28 PM, Naoto Sato wrote: Hello, Please review the changes to the following issue: https://bugs.openjdk.java.net/browse/JDK-8181157 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8181157/webrev.05/ This RFE is to implement CLDR time zone names fallback mechanism [1]. Prior to this fix, time zone names are substituted with English names. With this change, it is generated according to the logic defined in TR 35. Here are the highlight of the changes: CLDRConverter: - CLDRConverter has changed to not substitute/invent time zone display names, except English bundle for compatibility & runtime performance. - For English bundle, copy over COMPAT's names as before. - CLDRConverer now parses regionFormat/gmtZeroFormat/exemplarCity CLDR provider: - CLDRTimeZoneNameProviderImpl is introduced to generate fallback names at runtime. - If COMPAT provider is present, use it also as a fallback, before the last resort (GMT offset format) Naoto [1] http://www.unicode.org/reports/tr35/tr35-dates.html#Time_Zone_Names
Re: [PATCH] minor regex cleanup: use switch for enum
ping?
Isaac
On Wed, Apr 18, 2018 at 2:58 PM, Isaac Levy wrote:
> Hi,
>
> Minor improvement in readability (and probably perf) for Pattern. Switch
> is more consistent with the rest of the impl and the resulting tableswitch
> avoids a comparison for possessives.
>
> -Isaac
>
> --- a/src/java.base/share/classes/java/util/regex/Pattern.java
> +++ b/src/java.base/share/classes/java/util/regex/Pattern.java
> @@ -4356,7 +4356,9 @@
> -if (type == Qtype.GREEDY)
> +switch (type) {
> +case GREEDY:
> return match0(matcher, i, j, seq);
> -else if (type == Qtype.LAZY)
> +case LAZY:
> return match1(matcher, i, j, seq);
> -else
> +default:
> return match2(matcher, i, j, seq);
> +}
>
> @@ -4527,7 +4529,10 @@
> -if (type == Qtype.GREEDY) {
> +switch (type) {
> +case GREEDY:
> ret = match0(matcher, i, cmin, seq);
> +break;
> -} else if (type == Qtype.LAZY) {
> +case LAZY:
> ret = match1(matcher, i, cmin, seq);
> +break;
> -} else {
> +default:
> ret = match2(matcher, i, cmin, seq);
> }
>
>
Re: RFR of JDK-8201469,test under java/rmi should be restricted to not run concurrently
Hi Hamlin, Do you have a sense of what rmi tests are problematic to run concurrently? Maybe you can subset to reduce the increased impact on execution time? Paul. > On Apr 20, 2018, at 12:21 AM, Hamlin Li wrote: > > Is someone available to review the following patch? > > Thank you > > -Hamlin > > > On 19/04/2018 4:17 PM, Hamlin Li wrote: >> would you please review the following patch? >> >> bug: https://bugs.openjdk.java.net/browse/JDK-8201469 >> >> http://cr.openjdk.java.net/~mli/8201469/webrev.00/ >> >> ( For othervm.dirs property, I just reformat it. ) >> >> >> In my test result, with jtreg option "-concurrency:4", after apply the >> patch, tier3 tests on different platforms will be slower about 1.5~2.0 times >> than before. >> I think stability of tests are more important than executing time, how do >> you think about it? >> >> Thank you >> -Hamlin >
Re: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
Hi Goetz, It should be "out of bounds" without hyphens. Simon On 20/04/2018 11:03, Lindenmaier, Goetz wrote: Hi David, What about this: java.lang.ArrayIndexOutOfBoundsException: Arraycopy source index -1 out-of-bounds for double[10]. java.lang.ArrayIndexOutOfBoundsException: Arraycopy target index 10 out-of-bounds for object array[10]. java.lang.ArrayIndexOutOfBoundsException: Negative length -19 in arraycopy from int[3] to int[9]. I'll reply to the other points in a comprehensive mail when I know how to put the message. Thanks, Goetz. -Original Message- From: David Holmes [mailto:david.hol...@oracle.com] Sent: Freitag, 20. April 2018 09:25 To: Lindenmaier, Goetz ; hotspot-runtime- d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; aarch64- port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; core-libs- dev Libs Subject: Re: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException. Hi Goetz, This is not a file by file review ... On 19/04/2018 10:24 PM, Lindenmaier, Goetz wrote: Hi, New webrev: http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/02/ I admit my wording is not optimal. src/hotspot/share/oops/typeArrayKlass.cpp Sorry but this is still far too verbose for my tastes. The type of the array is not relevant. For the array copy its okay to indicate src or dst array. But the message should be clear and succinct not prose-like. Plus you have a lot of repetition in the ss.print statements when only one thing is changing. src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp I'm not seeing why the throw_index_out_of_bounds_exception should be tied to whether or not you have an array reference. Certainly I hate seeing the array ref being used as an implicit bool! It's because I extracted this change from a bigger one. Our message reads like this: Object [] oa1 = new Object[10] oa1[12] "ArrayIndexOutOfBoundsException while trying to load from index 12 of an object array with length 10, loaded from local variable 'oa1'" ... which seems not optimal, either. But it mentions the type (object), the operation (load, store etc ...) and the variable name. Naming the array is quite detailed if it is in a complex expression (like returned from a call). I'll contribute more of this if appreciated, this is a first step. I've never thought this complexity was warranted. We've had a few RFE's of this nature over the years and they have been closed (not necessarily by me I should point out!). Printing "index N is outside range [0, length-1]" is problematic for length '0'. I followed the proposal by Roger: "Index -1 out-of-bounds for length 10." You can easily special-case length 0. But Roger's proposal for consistency with existing messages make sense - not withstanding Andrew's dislike for the hyphens. I removed the change to ArrayIndexOutOfBoundsException.java. That's good. I extended the test to cover the exception thrown in arraycopy better The test seems somewhat excessive, and I now see it is because of the more elaborate error messages you have at SAP. But with only the index and the array length of interest here the test can be considerably smaller. The creation tests for ArrayIndexOutOfBoundsException don't seem relevant in this context either. This looks more TCK like. David - and added the elementary type to the message text. This probably needs improvement in the text, too. There are (currently) these cases: bject[] oa1 = new Object[10]; Object[] oa2 = new Object[5]; System.arraycopy(oa1, -17, oa2, 0, 5); "while trying to copy from index -17 of an object array with length 10"); System.arraycopy(oa1, 2, oa2, -18, 5); "while trying to copy to index -18 of an object array with length 5"); System.arraycopy(oa1, 2, oa2, 0, -19); "while trying to copy a negative range -19 from an object array with length 10 to an object array with length 5"); System.arraycopy(oa1, 8, oa2, 0, 5); "while trying to copy from index 13 of an object array with length 10"); System.arraycopy(oa1, 1, oa2, 0, 7); "while trying to copy to index 7 of an object array with length 5"); double[] ta1 = new double[10]; double[] ta2 = new double[5]; System.arraycopy(ta1, -17, ta2, 0, 5); "while trying to copy from index -17 of a doubl array with length 10"); System.arraycopy(ta1, 2, ta2, -18, 5); "while trying to copy to index -18 of a double array with length 5"); System.arraycopy(ta1, 2, ta2, 0, -19); "while trying to copy a negative range -19 from a double array with length 10 to a double array with length 5"); System.arraycopy(ta1, 8, ta2, 0, 5); "while trying to copy from index 13 of a double array with length 10"); System.arraycopy(ta1, 1, ta2, 0, 7); "while trying to copy to index 7 of a double array with length 5"); Maybe it should say: Arraycopy source index -1 out-of-bounds for double array of length 10. Arraycopy target index 10 out-of-bounds for object array of length 10. Negative range -19 when copying from an object arra
[PATCH] Add compareToUnsigned to java.nio.*Buffer
The proposed patch is an improvement for Byte/Short/Int/LongBuffer
classes to add compareToUnsigned() that works like compareTo() but
treats the values as unsigned to leverage the vectorized mismatch
implementations, which are not publicly accessible.
In addition to that, it adds mismatch() for all *Buffer classes.
Both new methods basically expose all the goodness of
jdk.internal.util.ArraysSupport to *Buffer.
The patch includes adoptions to the existing unit tests as well.
Robert
--
Robert Stupp
@snazy
# HG changeset patch
# User Robert Stupp
# Date 1524399060 -7200
# Sun Apr 22 14:11:00 2018 +0200
# Branch buffer-compUns-mismatch
# Node ID 06f0013900eba02b3136aa3bd72b3f981e3339df
# Parent fcd5df7aa235ca39852b04eb589e25f156870ce4
Add unsigned comparison to *Buffer
diff --git a/src/java.base/share/classes/java/nio/X-Buffer.java.template
b/src/java.base/share/classes/java/nio/X-Buffer.java.template
--- a/src/java.base/share/classes/java/nio/X-Buffer.java.template
+++ b/src/java.base/share/classes/java/nio/X-Buffer.java.template
@@ -1342,6 +1342,35 @@
return this.remaining() - that.remaining();
}
+#if[!char]
+#if[integralType]
+/**
+ * Compares this buffer to another treating the values as unsigned.
+ *
+ * Two $type$ buffers are compared by comparing their sequences of
+ * remaining elements lexicographically, without regard to the starting
+ * position of each sequence within its corresponding buffer.
+ * Pairs of {@code $type$} elements are compared as if by invoking
+ * {@link $Fulltype$#compareUnsigned($type$,$type$)}.
+ *
+ * A $type$ buffer is not comparable to any other type of object.
+ *
+ * @param that the $type$ buffer to be compared.
+ * @return A negative integer, zero, or a positive integer as this buffer
+ * is less than, equal to, or greater than the given buffer
+ */
+public int compareToUnsigned($Type$Buffer that) {
+int i = BufferMismatch.mismatch(this, this.position(),
+that, that.position(),
+Math.min(this.remaining(),
that.remaining()));
+if (i >= 0) {
+return compareUnsigned(this.get(this.position() + i),
that.get(that.position() + i));
+}
+return this.remaining() - that.remaining();
+}
+#end[integralType]
+#end[!char]
+
private static int compare($type$ x, $type$ y) {
#if[floatingPointType]
return ((x < y) ? -1 :
@@ -1353,6 +1382,34 @@
#end[floatingPointType]
}
+#if[!char]
+#if[integralType]
+private static int compareUnsigned($type$ x, $type$ y) {
+return $Fulltype$.compareUnsigned(x, y);
+}
+#end[integralType]
+#end[!char]
+
+/**
+ * Find the relative index of a mismatch between this buffer and another
+ * starting the current positions.
+ *
+ * A $type$ buffer is not comparable to any other type of object.
+ *
+ * @param that the $type$ buffer to be compared.
+ * @return the relative index of a mismatch between the two buffers,
+ * otherwise -1 if no mismatch. The index will be within the
range of
+ * (inclusive) {@link #position()} to (exclusive) the smaller of
+ * {@link #remaining() this.position()+this.remaining()} and
+ * {@link #remaining() this.position()+that.remaining()}.
+ */
+public int mismatch($Type$Buffer that) {
+int r = BufferMismatch.mismatch(this, this.position(),
+that, that.position(),
+Math.min(this.remaining(),
that.remaining()));
+return r == -1 ? -1 : r + this.position();
+}
+
// -- Other char stuff --
#if[char]
diff --git a/test/jdk/java/nio/Buffer/Basic-X.java.template
b/test/jdk/java/nio/Buffer/Basic-X.java.template
--- a/test/jdk/java/nio/Buffer/Basic-X.java.template
+++ b/test/jdk/java/nio/Buffer/Basic-X.java.template
@@ -697,6 +697,17 @@
if (b.compareTo(b2) != 0) {
fail("Comparison to identical buffer != 0", b, b2);
}
+#if[compareToUnsigned]
+if (b.compareToUnsigned(b2) != 0) {
+fail("Unsighed comparison to identical buffer != 0", b, b2);
+}
+#end[compareToUnsigned]
+if (b.mismatch(b2) != -1) {
+fail("Mismatch to identical buffer != -1", b, b2);
+}
+if (b2.mismatch(b) != -1) {
+fail("Mismatch to identical buffer != -1", b, b2);
+}
b.limit(b.limit() + 1);
b.position(b.limit() - 1);
b.put(($type$)99);
@@ -706,6 +717,11 @@
fail("Non-identical buffers equal", b, b2);
if (b.compareTo(b2) <= 0)
fail("Comparison to shorter buffer <= 0", b, b2);
+#if[compareToUnsigned]
+if (b.compareToUnsigned(b2) <= 0) {
+fail("Unsighed comparison to lesser shorter buffer <= 0", b, b2);
+}
+#end
[PATCH] Prefer TMPDIR over hard coded /tmp
For MacOS and Windows, Java prefers the user's temporary directory for
java.io.tmpdir, but not for Linux, where it is always set to /tmp. The
burden with this is that if you want to use a different temp directory,
you have to explicitly pass -Djava.io.tmpdir=... on the command line,
which can be difficult especially for unit tests or microbenchmarks in
3rd party code ran via Maven for example.
java_props_t.tmp_dir is always initialized as P_tmpdir in
GetJavaProperties (src/java.base/unix/native/libjava/java_props_md.c).
The attached patch changed the behavior to use the content of the
environment variable TMPDIR, if present. Note that this does not change
where the hsperf* folders are created.
I'm not sure why java.io.tmpdir is always /tmp in Linux as according to
the SCM history, this was always as it is (references the initial
OpenJDK commit).
I haven't found any tests that validates the content of java.io.tmpdir.
Robert
--
Robert Stupp
@snazy
# HG changeset patch
# User Robert Stupp
# Date 1524398509 -7200
# Sun Apr 22 14:01:49 2018 +0200
# Branch use-TMPDIR-env-Linux
# Node ID 3f9f58a5d4049fcba8e5201e321bf71984430ce9
# Parent fcd5df7aa235ca39852b04eb589e25f156870ce4
Use TMPDIR environment variable for java.io.tmpdir on Linux
diff --git a/src/java.base/unix/native/libjava/java_props_md.c
b/src/java.base/unix/native/libjava/java_props_md.c
--- a/src/java.base/unix/native/libjava/java_props_md.c
+++ b/src/java.base/unix/native/libjava/java_props_md.c
@@ -358,7 +358,7 @@
return &sprops;
}
-/* tmp dir */
+/* tmp dir, use the default from _PATH_VARTMP */
sprops.tmp_dir = P_tmpdir;
#ifdef MACOSX
/* darwin has a per-user temp dir */
@@ -367,6 +367,14 @@
if (pathSize > 0 && pathSize <= PATH_MAX) {
sprops.tmp_dir = tmp_path;
}
+#else
+/* Use the temporary directory injected via the environment variable TMPDIR
+ * instead of the "hard coded" _PATH_VARTMP
+ */
+v = getenv("TMPDIR");
+if (v) {
+sprops.tmp_dir = strdup(v);
+}
#endif /* MACOSX */
/* Printing properties */
Re: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
On 23/04/2018 5:08 PM, Lindenmaier, Goetz wrote: Hi, I still don't like the array type information - it's not relevant to the error IMHO. That aside, for the array copy cases I'd go for something like: arraycopy src index -1 out of bounds for double[10] arraycopy dest index -1 out of bounds for double[10] arraycopy end src index 11 out of bounds for double[10] arraycopy end dest index 11 out of bounds for double[10] It's a good idea to point out it's the end of the region to copy. But I would find "last src index" better to understand than "end src index": Sure - end/last/final whatever you prefer. "last arraycopy src index 11 out of bounds for double[10]" I'd prefer to see "arraycopy" always come first to show clearly these are specific to arraycopy. Even "arraycopy: ...". arraycopy length -19 is negative In the final case the array lengths are not relevant. I think they might help in case the length computation depends on the array lengths, which is quite likely the case. But I'll leave it out. Happy to let others give opinions. David - I'll reply to the other points in a comprehensive mail when I know how to put the message. ... these all are not related to the wording of the message. I don't oppose additional useful information in exception messages, but if it not directly relevant it just makes the error message harder to decipher IMO. SAP JVM tries to give all information useful to track down the cause. In many situations the printed information might be superfluous, but in this one critical situation you will be very happy to have it. SAP builds 'real' software on Java, not just 100 line jtreg tests ... But the message should stay clear. Therefore I also don't like the 1-sentence rule you sometimes mention. Several sentences can give structure to distinguish root cause information and additional information. Not relevant here currently, though. I also don't want to pay additional runtime costs just to make an exception message a little bit clearer. Yes, especially it should not impose any cost on the case without exception. Best regards, Goetz. Thanks, David Thanks, Goetz. -Original Message- From: David Holmes [mailto:david.hol...@oracle.com] Sent: Freitag, 20. April 2018 09:25 To: Lindenmaier, Goetz ; hotspot-runtime- d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; aarch64- port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; core- libs- dev Libs Subject: Re: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException. Hi Goetz, This is not a file by file review ... On 19/04/2018 10:24 PM, Lindenmaier, Goetz wrote: Hi, New webrev: http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/02/ I admit my wording is not optimal. src/hotspot/share/oops/typeArrayKlass.cpp Sorry but this is still far too verbose for my tastes. The type of the array is not relevant. For the array copy its okay to indicate src or dst array. But the message should be clear and succinct not prose-like. Plus you have a lot of repetition in the ss.print statements when only one thing is changing. src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp I'm not seeing why the throw_index_out_of_bounds_exception should be tied to whether or not you have an array reference. Certainly I hate seeing the array ref being used as an implicit bool! It's because I extracted this change from a bigger one. Our message reads like this: Object [] oa1 = new Object[10] oa1[12] "ArrayIndexOutOfBoundsException while trying to load from index 12 of an object array with length 10, loaded from local variable 'oa1'" ... which seems not optimal, either. But it mentions the type (object), the operation (load, store etc ...) and the variable name. Naming the array is quite detailed if it is in a complex expression (like returned from a call). I'll contribute more of this if appreciated, this is a first step. I've never thought this complexity was warranted. We've had a few RFE's of this nature over the years and they have been closed (not necessarily by me I should point out!). Printing "index N is outside range [0, length-1]" is problematic for length '0'. I followed the proposal by Roger: "Index -1 out-of-bounds for length 10." You can easily special-case length 0. But Roger's proposal for consistency with existing messages make sense - not withstanding Andrew's dislike for the hyphens. I removed the change to ArrayIndexOutOfBoundsException.java. That's good. I extended the test to cover the exception thrown in arraycopy better The test seems somewhat excessive, and I now see it is because of the more elaborate error messages you have at SAP. But with only the index and the array length of interest here the test can be considerably smaller. The creation tests for ArrayIndexOutOfBoundsException don't seem relevant in this context either. This looks more TCK like. David - and added the elementary type to the message text.
RE: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
Hi Simon, I chose hyphens because they are used in other AIOOB messages, too. But others (including me) seem not to like them either, so I'll leave them out. Best regards, Goetz. > -Original Message- > From: Simon Nash [mailto:si...@cjnash.com] > Sent: Freitag, 20. April 2018 17:17 > To: Lindenmaier, Goetz > Cc: David Holmes ; hotspot-runtime- > d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; aarch64- > port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; core-libs- > dev Libs > Subject: Re: RFR(M): 8201593: Print array length in > ArrayIndexOutOfBoundsException. > > Hi Goetz, > It should be "out of bounds" without hyphens. > > Simon > > On 20/04/2018 11:03, Lindenmaier, Goetz wrote: > > Hi David, > > > > What about this: > > java.lang.ArrayIndexOutOfBoundsException: Arraycopy source index -1 > out-of-bounds for double[10]. > > java.lang.ArrayIndexOutOfBoundsException: Arraycopy target index 10 > out-of-bounds for object array[10]. > > java.lang.ArrayIndexOutOfBoundsException: Negative length -19 in > arraycopy from int[3] to int[9]. > > > > I'll reply to the other points in a comprehensive mail when I know > > how to put the message. > > > > Thanks, > > Goetz. > > > > > >> -Original Message- > >> From: David Holmes [mailto:david.hol...@oracle.com] > >> Sent: Freitag, 20. April 2018 09:25 > >> To: Lindenmaier, Goetz ; hotspot-runtime- > >> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; > aarch64- > >> port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; core- > libs- > >> dev Libs > >> Subject: Re: RFR(M): 8201593: Print array length in > >> ArrayIndexOutOfBoundsException. > >> > >> Hi Goetz, > >> > >> This is not a file by file review ... > >> > >> On 19/04/2018 10:24 PM, Lindenmaier, Goetz wrote: > >>> Hi, > >>> > >>> New webrev: > >>> http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/02/ > >>> > >>> I admit my wording is not optimal. > >> src/hotspot/share/oops/typeArrayKlass.cpp > >> > >> Sorry but this is still far too verbose for my tastes. The type of the > >> array is not relevant. For the array copy its okay to indicate src or > >> dst array. But the message should be clear and succinct not prose-like. > >> Plus you have a lot of repetition in the ss.print statements when only > >> one thing is changing. > >> > >> src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp > >> > >> I'm not seeing why the throw_index_out_of_bounds_exception should > be > >> tied to whether or not you have an array reference. Certainly I hate > >> seeing the array ref being used as an implicit bool! > >> > >>> It's because I extracted this change from a bigger one. Our message > reads > >> like this: > >>> Object [] oa1 = new Object[10] > >>> oa1[12] > >>> "ArrayIndexOutOfBoundsException while trying to load from index 12 of > an > >> object array with length 10, loaded from local variable 'oa1'" > >>> ... which seems not optimal, either. But it mentions the type (object), > the > >> operation (load, store etc ...) and the variable name. > >>> Naming the array is quite detailed if it is in a complex expression (like > >> returned from a call). > >>> I'll contribute more of this if appreciated, this is a first step. > >> I've never thought this complexity was warranted. We've had a few RFE's > >> of this nature over the years and they have been closed (not necessarily > >> by me I should point out!). > >> > >>> Printing "index N is outside range [0, length-1]" is problematic > >>> for length '0'. I followed the proposal by Roger: > >>> "Index -1 out-of-bounds for length 10." > >> You can easily special-case length 0. But Roger's proposal for > >> consistency with existing messages make sense - not withstanding > >> Andrew's dislike for the hyphens. > >> > >>> I removed the change to ArrayIndexOutOfBoundsException.java. > >> That's good. > >> > >>> I extended the test to cover the exception thrown in arraycopy better > >> The test seems somewhat excessive, and I now see it is because of the > >> more elaborate error messages you have at SAP. But with only the index > >> and the array length of interest here the test can be considerably smaller. > >> > >> The creation tests for ArrayIndexOutOfBoundsException don't seem > >> relevant in this context either. This looks more TCK like. > >> > >> David > >> - > >> > >>> and added the elementary type to the message text. This probably > >>> needs improvement in the text, too. There are (currently) these cases: > >>> > >>> bject[] oa1 = new Object[10]; > >>> Object[] oa2 = new Object[5]; > >>> System.arraycopy(oa1, -17, oa2, 0, 5); > >>> "while trying to copy from index -17 of an object array with length 10"); > >>> System.arraycopy(oa1, 2, oa2, -18, 5); > >>> "while trying to copy to index -18 of an object array with length 5"); > >>> System.arraycopy(oa1, 2, oa2, 0, -19); > >>> "while trying to copy a negative range -19 from an object array with > length > >> 10 to an object array with
RE: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.
Hi, > I still don't like the array type information - it's not relevant to the > error IMHO. > > That aside, for the array copy cases I'd go for something like: > > arraycopy src index -1 out of bounds for double[10] > arraycopy dest index -1 out of bounds for double[10] > arraycopy end src index 11 out of bounds for double[10] > arraycopy end dest index 11 out of bounds for double[10] It's a good idea to point out it's the end of the region to copy. But I would find "last src index" better to understand than "end src index": "last arraycopy src index 11 out of bounds for double[10]" > arraycopy length -19 is negative > > In the final case the array lengths are not relevant. I think they might help in case the length computation depends on the array lengths, which is quite likely the case. But I'll leave it out. > > I'll reply to the other points in a comprehensive mail when I know > > how to put the message. ... these all are not related to the wording of the message. > > I don't oppose additional useful information in exception messages, but > if it not directly relevant it just makes the error message harder to > decipher IMO. SAP JVM tries to give all information useful to track down the cause. In many situations the printed information might be superfluous, but in this one critical situation you will be very happy to have it. SAP builds 'real' software on Java, not just 100 line jtreg tests ... But the message should stay clear. Therefore I also don't like the 1-sentence rule you sometimes mention. Several sentences can give structure to distinguish root cause information and additional information. Not relevant here currently, though. > I also don't want to pay additional runtime costs just to > make an exception message a little bit clearer. Yes, especially it should not impose any cost on the case without exception. Best regards, Goetz. > Thanks, > David > > > Thanks, > >Goetz. > > > > > >> -Original Message- > >> From: David Holmes [mailto:david.hol...@oracle.com] > >> Sent: Freitag, 20. April 2018 09:25 > >> To: Lindenmaier, Goetz ; hotspot-runtime- > >> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; > aarch64- > >> port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; core- > libs- > >> dev Libs > >> Subject: Re: RFR(M): 8201593: Print array length in > >> ArrayIndexOutOfBoundsException. > >> > >> Hi Goetz, > >> > >> This is not a file by file review ... > >> > >> On 19/04/2018 10:24 PM, Lindenmaier, Goetz wrote: > >>> Hi, > >>> > >>> New webrev: > >>> http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/02/ > >>> > >>> I admit my wording is not optimal. > >> > >> src/hotspot/share/oops/typeArrayKlass.cpp > >> > >> Sorry but this is still far too verbose for my tastes. The type of the > >> array is not relevant. For the array copy its okay to indicate src or > >> dst array. But the message should be clear and succinct not prose-like. > >> Plus you have a lot of repetition in the ss.print statements when only > >> one thing is changing. > >> > >> src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp > >> > >> I'm not seeing why the throw_index_out_of_bounds_exception should > be > >> tied to whether or not you have an array reference. Certainly I hate > >> seeing the array ref being used as an implicit bool! > >> > >>> It's because I extracted this change from a bigger one. Our message > reads > >> like this: > >>> Object [] oa1 = new Object[10] > >>> oa1[12] > >>> "ArrayIndexOutOfBoundsException while trying to load from index 12 of > an > >> object array with length 10, loaded from local variable 'oa1'" > >>> ... which seems not optimal, either. But it mentions the type (object), > the > >> operation (load, store etc ...) and the variable name. > >>> Naming the array is quite detailed if it is in a complex expression (like > >> returned from a call). > >>> I'll contribute more of this if appreciated, this is a first step. > >> > >> I've never thought this complexity was warranted. We've had a few RFE's > >> of this nature over the years and they have been closed (not necessarily > >> by me I should point out!). > >> > >>> Printing "index N is outside range [0, length-1]" is problematic > >>> for length '0'. I followed the proposal by Roger: > >>> "Index -1 out-of-bounds for length 10." > >> > >> You can easily special-case length 0. But Roger's proposal for > >> consistency with existing messages make sense - not withstanding > >> Andrew's dislike for the hyphens. > >> > >>> I removed the change to ArrayIndexOutOfBoundsException.java. > >> > >> That's good. > >> > >>> I extended the test to cover the exception thrown in arraycopy better > >> > >> The test seems somewhat excessive, and I now see it is because of the > >> more elaborate error messages you have at SAP. But with only the index > >> and the array length of interest here the test can be considerably smaller. > >> > >> The creation tests for ArrayIndexOutOfBoundsException don't seem > >>
