Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v2]
On Wed, 27 Apr 2022 17:20:11 GMT, Roger Riggs wrote:
>> Ichiroh Takiguchi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8285517: System.getenv() returns unexpected value if environment variable
>> has non ASCII character
>
> src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 68:
>
>> 66: private static final Map theUnmodifiableEnvironment;
>> 67: static final int MIN_NAME_LENGTH = 0;
>> 68: static final Charset cs =
>> Charset.forName(StaticProperty.nativeEncoding(),
>
> cs should have an all-CAPS name to make it clear its a static value
> throughout the implementation.
> And `private` too.
Change to "private static final"
> test/jdk/java/lang/System/i18nEnvArg.java line 41:
>
>> 39:
>> 40: public class i18nEnvArg {
>> 41: final static String text = "\u6F22\u5B57";
>
> Can this be renamed to indicate it is the string under test? like KANJI_TEXT
> or EUC_JP_TEXT or similar.
Use EUC_JP_TEXT
> test/jdk/java/lang/System/i18nEnvArg.java line 49:
>
>> 47: final static int maxSize = 4096;
>> 48:
>> 49: public static void main(String[] args) throws Exception {
>
> There are 3 main()'s in this test; it would be useful to add a comment
> indicating the purpose of each.
Add comments
> test/jdk/java/lang/System/i18nEnvArg.java line 60:
>
>> 58: Process p = pb.start();
>> 59: InputStream is = p.getInputStream();
>> 60: byte[] ba = new byte[maxSize];
>
> You can use `InputStream.readAllBytes()` here to return a byte buffer.
>
> Its not clear why all the bytes are read? I assume its only for a possible
> error from the child.
Use InputStream.readAllBytes()
> test/jdk/java/lang/System/i18nEnvArg.java line 128:
>
>> 126: Method enviorn_mid = cls.getDeclaredMethod("environ");
>> 127: enviorn_mid.setAccessible(true);
>> 128: byte[][] environ = (byte[][]) enviorn_mid.invoke(null,
>
> typo: "enviorn_mid" -> "environ_mid".
Fix typo
> test/jdk/java/lang/System/i18nEnvArg.java line 138:
>
>> 136: bb.put(environ[i+1]);
>> 137: byte[] envb = Arrays.copyOf(ba, bb.position());
>> 138: if (Arrays.equals(kanji, envb)) return;
>
> It seems odd to exit the loop on the first match.
>
> I think AIX always has environment variables not set by the caller.
On this testing, use 2 environment variables:
* LANG=ja_JP.eucjp
* \u6F22\u5B57=\u6F22\u5B57
If the other environment variable is defined, it's printed.
> test/jdk/java/lang/System/i18nEnvArg.java line 146:
>
>> 144: sb.append((char)b);
>> 145: } else {
>> 146: sb.append(String.format("\\x%02X", (int)b &
>> 0xFF));
>
> The methods of java.util.HexFormat may be useful here.
> It seems that the "5C" would fit into this "else" clause and not need a
> separate statement.
Use HexFormat class
-
PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v2]
On Tue, 26 Apr 2022 21:17:34 GMT, Naoto Sato wrote:
>> Ichiroh Takiguchi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8285517: System.getenv() returns unexpected value if environment variable
>> has non ASCII character
>
> Thanks for fixing the issue.
> As to the test, it has lots of redundancy, and too complicated to me. Can you
> simplify the test?
Hello @naotoj and @RogerRiggs .
I appreciate your comments.
I applied the changes.
Additionally, I put bugid into test/jdk/java/lang/ProcessBuilder/Basic.java
> src/java.base/unix/classes/java/lang/ProcessImpl.java line 146:
>
>> 144: private static final LaunchMechanism launchMechanism =
>> platform.launchMechanism();
>> 145: private static final byte[] helperpath =
>> toCString(StaticProperty.javaHome() + "/lib/jspawnhelper");
>> 146: private static Charset jnuCharset = null;
>
> Can we simply call `jnuCharset()` here?
Change jnuCharset to "private static".
> test/jdk/java/lang/ProcessBuilder/Basic.java line 605:
>
>> 603: String fileEncoding = System.getProperty("file.encoding");
>> 604: Charset cs;
>> 605: if (nativeEncoding != null &&
>> !nativeEncoding.equals(fileEncoding)) {
>
> What's the reason for comparing `file.encoding`? Does
> `Charset.forName(nativeEncoding, Charset.defaultCharset())` suffice?
Remove file.encoding related code.
> test/jdk/java/lang/System/i18nEnvArg.java line 26:
>
>> 24: /*
>> 25: * @test
>> 26: * @bug 8285517
>
> If the test should work only on a particular env, should describe here and
> add `@requires` tag.
Add "@requires (os.family == "linux")"
> test/jdk/java/lang/System/i18nEnvArg.java line 50:
>
>> 48:
>> 49: public static void main(String[] args) throws Exception {
>> 50: ProcessBuilder pb = new ProcessBuilder(javeExe,
>
> Can utilize `jdk.test.lib.process.ProcessTools`.
Use ProcessTools class.
-
PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v2]
> On JDK19 with Linux ja_JP.eucjp locale, > System.getenv() returns unexpected value if environment variable has Japanese > EUC characters. > It seems this issue happens because of JEP 400. > Arguments for ProcessBuilder have same kind of issue. Ichiroh Takiguchi has updated the pull request incrementally with one additional commit since the last revision: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character - Changes: - all: https://git.openjdk.java.net/jdk/pull/8378/files - new: https://git.openjdk.java.net/jdk/pull/8378/files/89ee195a..050dd663 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8378=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8378=00-01 Stats: 106 lines in 4 files changed: 26 ins; 41 del; 39 mod Patch: https://git.openjdk.java.net/jdk/pull/8378.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8378/head:pull/8378 PR: https://git.openjdk.java.net/jdk/pull/8378
Integrated: 8285890: Fix some @param tags
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo wrote: > * Syntactically improves a few cases from 8285676 > (https://github.com/openjdk/jdk/pull/8410) > * Fixes a few misspelled `@param` tags for elements that, although are not > included in the API Documentation, are visible in and processed by IDEs This pull request has now been integrated. Changeset: 3eb661bb Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/3eb661bbe7151f3a7e949b6518f57896c2bd4136 Stats: 16 lines in 8 files changed: 0 ins; 0 del; 16 mod 8285890: Fix some @param tags Reviewed-by: dfuchs, mullan, darcy, mchung, wetmore - PR: https://git.openjdk.java.net/jdk/pull/8465
Unused method sun.reflect.misc.MethodUtil.getPublicMethods
Hello. I found a few methods in MethodUtil class which seem unused to me: getPublicMethods, getInterfaceMethods, getInternalPublicMethods ,addMethod. Are they somehow used by VM, or is it just leftovers from some refactorings? I wonder if we can drop them. Andrey Turbanov
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v35]
On Fri, 29 Apr 2022 08:15:32 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-424 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tweak Addressable javadoc
src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
line 101:
> 99: }
> 100:
> 101: public final static class ScopedAccessError extends Error {
This should probably use the canonical modifier order as specified in
[JDK‑8276348] ([GH‑6213]):
Suggestion:
public static final class ScopedAccessError extends Error {
[JDK‑8276348]: https://bugs.openjdk.java.net/browse/JDK-8276348 "[JDK‑8276348]
Use blessed modifier order in java.base"
[GH‑6213]: https://github.com/openjdk/jdk/pull/6213
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 06:31:22 GMT, Andrey Turbanov wrote:
> `Map.containsKey` call is sometimes unnecessary, when it's known that Map
> doesn't contain `null` values.
> Instead we can just use Map.get and compare result with `null`.
> I found one of such place, where Map.containsKey calls could be eliminated -
> `java.time.format.ZoneName`.
> it gives a bit of performance for `toZid`.
>
>
> Benchmark Mode Cnt Score Error Units
> ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op
> ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op
>
>
>
> Benchmark
>
>
> @BenchmarkMode(value = Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS)
> @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
> @Fork(1)
> @State(Scope.Benchmark)
> public class ZoneNamesBench {
>
> @Benchmark
> public String useExistingCountry() {
> return ZoneName.toZid("Africa/Tunis", Locale.ITALY);
> }
>
> @Benchmark
> public String useExistingCountryOld() {
> return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY);
> }
> }
>
>
>
> public static String toZid(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null) {
> String alias = aliases.get(zid);
> if (alias != null) {
> zid = alias;
> mzone = zidToMzone.get(zid);
> }
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map == null || ((zid = map.get(locale.getCountry())) ==
> null)) {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZid(zid);
> }
>
> public static String toZid(String zid) {
> return aliases.getOrDefault(zid, zid);
> }
>
> public static String toZidOld(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null && aliases.containsKey(zid)) {
> zid = aliases.get(zid);
> mzone = zidToMzone.get(zid);
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map != null && map.containsKey(locale.getCountry())) {
> zid = map.get(locale.getCountry());
> } else {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZidOld(zid);
> }
>
> public static String toZidOld(String zid) {
> if (aliases.containsKey(zid)) {
> return aliases.get(zid);
> }
> return zid;
> }
>
>
Logically, this LGTM
-
Marked as reviewed by scolebourne (Author).
PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8285519: Change usages of TimeUnit.convert to TimeUnit.toXXX
On Sat, 23 Apr 2022 21:02:50 GMT, Andrey Turbanov wrote:
> TimeUnit.toMillis/toNanos/toMicros/toSeconds is shorter and a bit faster.
> Compared via JMH benchmark: 150ns -> 125ns/op
>
> Benchamark adapted from
> http://cr.openjdk.java.net/~shade/8152083/TimeUnitBench.java
>
>
> @Warmup(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
> @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
> @Fork(1)
> @State(Scope.Thread)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class TimeUnitBench {
>
> static final int SIZE = TimeUnit.values().length * 10;
>
> @Param({"0", "1", "2", "3", "4", "5", "6", "-1"})
> int bias;
>
> TimeUnit[] mod;
>
> @Setup
> public void setup() {
> mod = new TimeUnit[SIZE];
>
> TimeUnit[] vals = TimeUnit.values();
> for (int c = 0; c < SIZE; c++) {
> if (bias == -1) {
> // megamorphic
> mod[c] = vals[c % vals.length];
> } else {
> mod[c] = vals[bias];
> }
> }
>
> Random r = new Random();
> for (int c = 0; c < 1; c++) {
> int i = r.nextInt();
> for (int o = 0; o < vals.length; o++) {
> if (vals[o].toNanos(i) != TimeUnit.NANOSECONDS.convert(i,
> vals[o]))
> throw new IllegalStateException("switch " + o);
> }
> }
> }
>
> @Benchmark
> public void const_convert(Blackhole bh) {
> for (TimeUnit tu : mod) {
> bh.consume(do_convert(tu));
> }
> }
>
> @Benchmark
> public void value_convert(Blackhole bh) {
> for (TimeUnit tu : mod) {
> bh.consume(do_convert(tu, 1L));
> }
> }
>
> @Benchmark
> public void const_toNanos(Blackhole bh) {
> for (TimeUnit tu : mod) {
> bh.consume(do_toNanos(tu));
> }
> }
>
> @Benchmark
> public void value_toNanos(Blackhole bh) {
> for (TimeUnit tu : mod) {
> bh.consume(do_toNanos(tu, 1L));
> }
> }
>
> @CompilerControl(CompilerControl.Mode.DONT_INLINE)
> private long do_toNanos(TimeUnit tu) {
> return tu.toNanos(1L);
> }
>
> @CompilerControl(CompilerControl.Mode.DONT_INLINE)
> private long do_toNanos(TimeUnit tu, long value) {
> return tu.toNanos(value);
> }
>
> @CompilerControl(CompilerControl.Mode.DONT_INLINE)
> private long do_convert(TimeUnit tu) {
> return TimeUnit.NANOSECONDS.convert(1L, tu);
> }
>
> @CompilerControl(CompilerControl.Mode.DONT_INLINE)
> private long do_convert(TimeUnit tu, long value) {
> return TimeUnit.NANOSECONDS.convert(value, tu);
> }
> }
>
> Results:
>
> Benchmark(bias) Mode CntScoreError Units
> TimeUnitBench.const_convert 0 avgt5 151,856 ± 28,595 ns/op
> TimeUnitBench.const_convert 1 avgt5 150,720 ± 23,863 ns/op
> TimeUnitBench.const_convert 2 avgt5 151,432 ± 23,184 ns/op
> TimeUnitBench.const_convert 3 avgt5 150,959 ± 24,726 ns/op
> TimeUnitBench.const_convert 4 avgt5 150,966 ± 25,280 ns/op
> TimeUnitBench.const_convert 5 avgt5 150,976 ± 25,542 ns/op
> TimeUnitBench.const_convert 6 avgt5 151,118 ± 25,254 ns/op
> TimeUnitBench.const_convert -1 avgt5 152,673 ± 29,861 ns/op
> TimeUnitBench.const_toNanos 0 avgt5 121,296 ± 25,236 ns/op
> TimeUnitBench.const_toNanos 1 avgt5 121,707 ± 25,364 ns/op
> TimeUnitBench.const_toNanos 2 avgt5 121,736 ± 25,726 ns/op
> TimeUnitBench.const_toNanos 3 avgt5 121,822 ± 25,491 ns/op
> TimeUnitBench.const_toNanos 4 avgt5 121,867 ± 26,090 ns/op
> TimeUnitBench.const_toNanos 5 avgt5 121,927 ± 25,611 ns/op
> TimeUnitBench.const_toNanos 6 avgt5 121,821 ± 25,843 ns/op
> TimeUnitBench.const_toNanos -1 avgt5 121,923 ± 25,206 ns/op
> TimeUnitBench.value_convert 0 avgt5 150,525 ± 25,439 ns/op
> TimeUnitBench.value_convert 1 avgt5 151,098 ± 24,024 ns/op
> TimeUnitBench.value_convert 2 avgt5 151,259 ± 25,381 ns/op
> TimeUnitBench.value_convert 3 avgt5 151,030 ± 25,548 ns/op
> TimeUnitBench.value_convert 4 avgt5 151,290 ± 25,998 ns/op
> TimeUnitBench.value_convert 5 avgt5 151,006 ± 25,448 ns/op
> TimeUnitBench.value_convert 6 avgt5 150,945 ± 25,314 ns/op
> TimeUnitBench.value_convert -1 avgt5 151,186 ± 25,814 ns/op
> TimeUnitBench.value_toNanos 0 avgt5 121,556 ± 25,745 ns/op
> TimeUnitBench.value_toNanos 1 avgt5 123,410 ± 22,323 ns/op
> TimeUnitBench.value_toNanos 2 avgt5 125,452 ± 26,575 ns/op
> TimeUnitBench.value_toNanos 3 avgt5 125,297 ± 26,204 ns/op
> TimeUnitBench.value_toNanos
