Re: RFR: JDK-8260273: DataOutputStream writeChars optimization
On Fri, 22 Jan 2021 02:57:36 GMT, Hamlin Li wrote:
> this is minor optimization following JDK-8254078.
> based on my tests with jmh, it has better performance when apply following
> patch:
>
> diff --git a/src/java.base/share/classes/java/io/DataOutputStream.java
> b/src/java.base/share/classes/java/io/DataOutputStream.java
> index 9a9a687403c..4ea497fc7c0 100644
> --- a/src/java.base/share/classes/java/io/DataOutputStream.java
> +++ b/src/java.base/share/classes/java/io/DataOutputStream.java
> @@ -300,8 +300,9 @@ public class DataOutputStream extends FilterOutputStream
> implements DataOutput {
> int len = s.length();
> for (int i = 0 ; i < len ; i++) {
> int v = s.charAt(i);
> - out.write((v >>> 8) & 0xFF);
> - out.write((v >>> 0) & 0xFF);
> + writeBuffer[0] = (byte)(v >>> 8);
> + writeBuffer[1] = (byte)(v >>> 0);
> + out.write(writeBuffer, 0, 2);
> }
> incCount(len * 2);
> }
>
> Basically, it has better performance when apply above patch:
>
> // without writeChars optimization patch, (-XX:-UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 115.208 ± 0.327 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 276.795
> ± 0.449 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 12356.969 ± 22.427 us/op
>
> // with writeChars optimization patch, (-XX:-UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 133.706 ± 0.274 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 130.979
> ± 0.155 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 6814.272 ± 52.770 us/op
>
>
> // without writeChars optimization patch, (-XX:+UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 130.367 ± 8.759 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 37.559
> ± 0.059 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 12385.030 ± 376.560 us/op
>
> // with writeChars optimization patch, (-XX:+UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 45.494 ± 7.018 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 33.015
> ± 0.349 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 6845.549 ± 38.712 us/op
Marked as reviewed by alanb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/2190
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Mon, 18 Jan 2021 11:03:06 GMT, Martin Buchholz wrote: >> 8252412: [macos11] system dynamic libraries removed from filesystem > > Martin Buchholz has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Marked as reviewed by valeriep (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Fri, 22 Jan 2021 22:55:22 GMT, Jiangli Zhou wrote: >> Ok, I see Java_sun_security_smartcardio_PlatformPCSC_initialize does dlopen >> using the 'jLibName' (string) obtained from getLibraryName() and throws >> IOException if dlopen fails. The change seems safe enough. >> >> I'm wondering if you want to check the file first then check the parent >> directory if the file does not exist. Not sure if that's a little more >> optimal on older macos, so I'll leave that to you to decide. >> >> For the jtreg test, how about converting Dominik's TestPCSC? As the file is >> a shared for 'unix' platforms, it feels safer at least with some level of >> unit test. Could you please give some more contexts about the >> functionalities associated with PCSC are broken on macos? > > Martin and I had an off-line chat and Martin convinced me that the existing > jtreg tests (such as test/jdk/javax/smartcardio and > test/jdk/sun/security/smartcardio are sufficient) to cover the case. Right, existing tests should cover this already since running the test requires that the library must be loaded. Changes look fine, thanks for fixing this. Kind of surprised the existing filtering didn't catch this as security-related changes and send this to security group for review. - PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Fri, 22 Jan 2021 22:56:08 GMT, Jiangli Zhou wrote: >> Martin Buchholz has refreshed the contents of this pull request, and >> previous commits have been removed. The incremental views will show >> differences compared to the previous content of the PR. > > Marked as reviewed by jiangli (Reviewer). Last call for a reviewer familiar with macos or smartcardio. In case of crickets I will submit soon. - PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Fri, 22 Jan 2021 22:16:28 GMT, Jiangli Zhou wrote: >> The directory structure is intact - only the file is removed from the >> filesystem. >> More generally, for many frameworks, where there used to be >> >> >> the file is gone, but >> >> >> remains. >> >> I don't think we need a jtreg test, since any functionality associated with >> PCSC is broken on this platform. I added label noreg-other > > Ok, I see Java_sun_security_smartcardio_PlatformPCSC_initialize does dlopen > using the 'jLibName' (string) obtained from getLibraryName() and throws > IOException if dlopen fails. The change seems safe enough. > > I'm wondering if you want to check the file first then check the parent > directory if the file does not exist. Not sure if that's a little more > optimal on older macos, so I'll leave that to you to decide. > > For the jtreg test, how about converting Dominik's TestPCSC? As the file is a > shared for 'unix' platforms, it feels safer at least with some level of unit > test. Could you please give some more contexts about the functionalities > associated with PCSC are broken on macos? Martin and I had an off-line chat and Martin convinced me that the existing jtreg tests (such as test/jdk/javax/smartcardio and test/jdk/sun/security/smartcardio are sufficient) to cover the case. - PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Mon, 18 Jan 2021 11:03:06 GMT, Martin Buchholz wrote: >> 8252412: [macos11] system dynamic libraries removed from filesystem > > Martin Buchholz has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Marked as reviewed by jiangli (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Fri, 22 Jan 2021 20:08:48 GMT, Martin Buchholz wrote:
>> src/java.smartcardio/unix/classes/sun/security/smartcardio/PlatformPCSC.java
>> line 132:
>>
>>> 130: // existence of the containing directory instead of the file.
>>> 131: lib = PCSC_FRAMEWORK;
>>> 132: if (new File(lib).getParentFile().isDirectory()) {
>>
>> This is outside of my normal area, could you please explain why checking the
>> existence of the containing directory is equivalent to checking the file
>> here? Does it provide the expected behavior on all unix-like platforms or
>> only macos?
>>
>> Could you please also provide a jtreg test for this change?
>
> The directory structure is intact - only the file is removed from the
> filesystem.
> More generally, for many frameworks, where there used to be
>
>
> the file is gone, but
>
>
> remains.
>
> I don't think we need a jtreg test, since any functionality associated with
> PCSC is broken on this platform. I added label noreg-other
Ok, I see Java_sun_security_smartcardio_PlatformPCSC_initialize does dlopen
using the 'jLibName' (string) obtained from getLibraryName() and throws
IOException if dlopen fails. The change seems safe enough.
I'm wondering if you want to check the file first then check the parent
directory if the file does not exist. Not sure if that's a little more optimal
on older macos, so I'll leave that to you to decide.
For the jtreg test, how about converting Dominik's TestPCSC? As the file is a
shared for 'unix' platforms, it feels safer at least with some level of unit
test. Could you please give some more contexts about the functionalities
associated with PCSC are broken on macos?
-
PR: https://git.openjdk.java.net/jdk/pull/2119
Integrated: 8258917: NativeMemoryTracking is handled by launcher inconsistenly
On Fri, 15 Jan 2021 23:50:16 GMT, Alex Menkov wrote: > The fix adds NMT handling for non-java launchers This pull request has now been integrated. Changeset: bdc305e1 Author:Alex Menkov URL: https://git.openjdk.java.net/jdk/commit/bdc305e1 Stats: 30 lines in 2 files changed: 16 ins; 1 del; 13 mod 8258917: NativeMemoryTracking is handled by launcher inconsistenly Reviewed-by: zgu - PR: https://git.openjdk.java.net/jdk/pull/2106
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port
On Fri, 22 Jan 2021 18:49:42 GMT, Anton Kozlov wrote: > Please review the implementation of JEP 391: macOS/AArch64 Port. > > It's heavily based on existing ports to linux/aarch64, macos/x86_64, and > windows/aarch64. > > Major changes are in: > * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks > JDK-8253817, JDK-8253818) > * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with necessary > adjustments (JDK-8253819) > * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), > required on macOS/AArch64 platform. It's implemented with > pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a > thread, so W^X mode change relates to the java thread state change (for java > threads). In most cases, JVM executes in write-only mode, except when calling > a generated stub like SafeFetch, which requires a temporary switch to > execute-only mode. The same execute-only mode is enabled when a java thread > executes in java or native states. This approach of managing W^X mode turned > out to be simple and efficient enough. > * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) Build changes in general look good. make/autoconf/flags-cflags.m4 line 169: > 167: WARNINGS_ENABLE_ALL="-Wall -Wextra -Wformat=2 > $WARNINGS_ENABLE_ADDITIONAL" > 168: > 169: DISABLED_WARNINGS="unknown-warning-option unused-parameter unused > format-nonliteral" Globally disabling a warning needs some motivation. Why is this needed? Does it really need to be applied everywhere or is there a specific binary or source file where this is triggered? - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java
On Fri, 22 Jan 2021 16:57:41 GMT, Mahendra Chhipa
wrote:
>> Can this be done all in `EnclosingClassTest.java`, without a new
>> `RunEnclosingClassTest.java`?
>>
>> Adding the `@BeforeClass` and `@AfterClass` methods to what's there, you may
>> just need to
>> change the `test()` calls to use reflection - something along the lines of:
>>
>>> test(`Class.forName("EnclosingClass").getDeclaredConstructor().newInstance());`
>
> Review comments implemented.
Can the new code be added to the existing `NonJavaNames.java` instead of adding
a new `NonJavaNameTest.java` file?
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
My github comment was mangled forwarding to core-libs. I suspect the skara
bidirectional mailing list forwarding bot discards lines with leading "/"
.
Instead the bot should pass on unrecognized github comment commands
unmodified.
On Fri, Jan 22, 2021 at 12:12 PM Martin Buchholz
wrote:
> On Fri, 22 Jan 2021 19:46:13 GMT, Jiangli Zhou
> wrote:
>
> >> Martin Buchholz has refreshed the contents of this pull request, and
> previous commits have been removed. The incremental views will show
> differences compared to the previous content of the PR.
> >
> >
> src/java.smartcardio/unix/classes/sun/security/smartcardio/PlatformPCSC.java
> line 132:
> >
> >> 130: // existence of the containing directory instead of the
> file.
> >> 131: lib = PCSC_FRAMEWORK;
> >> 132: if (new File(lib).getParentFile().isDirectory()) {
> >
> > This is outside of my normal area, could you please explain why checking
> the existence of the containing directory is equivalent to checking the
> file here? Does it provide the expected behavior on all unix-like platforms
> or only macos?
> >
> > Could you please also provide a jtreg test for this change?
>
> The directory structure is intact - only the file is removed from the
> filesystem.
> More generally, for many frameworks, where there used to be
>
>
> the file is gone, but
>
>
> remains.
>
> I don't think we need a jtreg test, since any functionality associated
> with PCSC is broken on this platform. I added label noreg-other
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/2119
>
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Fri, 22 Jan 2021 19:46:13 GMT, Jiangli Zhou wrote:
>> Martin Buchholz has refreshed the contents of this pull request, and
>> previous commits have been removed. The incremental views will show
>> differences compared to the previous content of the PR.
>
> src/java.smartcardio/unix/classes/sun/security/smartcardio/PlatformPCSC.java
> line 132:
>
>> 130: // existence of the containing directory instead of the file.
>> 131: lib = PCSC_FRAMEWORK;
>> 132: if (new File(lib).getParentFile().isDirectory()) {
>
> This is outside of my normal area, could you please explain why checking the
> existence of the containing directory is equivalent to checking the file
> here? Does it provide the expected behavior on all unix-like platforms or
> only macos?
>
> Could you please also provide a jtreg test for this change?
The directory structure is intact - only the file is removed from the
filesystem.
More generally, for many frameworks, where there used to be
the file is gone, but
remains.
I don't think we need a jtreg test, since any functionality associated with
PCSC is broken on this platform. I added label noreg-other
-
PR: https://git.openjdk.java.net/jdk/pull/2119
RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port
Please review the implementation of JEP 391: macOS/AArch64 Port. It's heavily based on existing ports to linux/aarch64, macos/x86_64, and windows/aarch64. Major changes are in: * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks JDK-8253817, JDK-8253818) * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with necessary adjustments (JDK-8253819) * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), required on macOS/AArch64 platform. It's implemented with pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a thread, so W^X mode change relates to the java thread state change (for java threads). In most cases, JVM executes in write-only mode, except when calling a generated stub like SafeFetch, which requires a temporary switch to execute-only mode. The same execute-only mode is enabled when a java thread executes in java or native states. This approach of managing W^X mode turned out to be simple and efficient enough. * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) - Commit messages: - Fix build - JDK-8253816: Update after recent changes - Rework gtests to always have wx_write - Revert gtest changes - Fix gtests in debug - Merge remote-tracking branch 'upstream/master' into jdk-macos - Fix windows_aarch64 - Use r18_tls instead of r18_reserved - JDK-8253742: bsd_aarch64 part - JDK-8257882: bsd_aarch64 part - ... and 40 more: https://git.openjdk.java.net/jdk/compare/82adfb32...3d4f4c23 Changes: https://git.openjdk.java.net/jdk/pull/2200/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2200=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253795 Stats: 3335 lines in 117 files changed: 3230 ins; 41 del; 64 mod Patch: https://git.openjdk.java.net/jdk/pull/2200.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2200/head:pull/2200 PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Mon, 18 Jan 2021 11:03:06 GMT, Martin Buchholz wrote:
>> 8252412: [macos11] system dynamic libraries removed from filesystem
>
> Martin Buchholz has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR.
src/java.smartcardio/unix/classes/sun/security/smartcardio/PlatformPCSC.java
line 132:
> 130: // existence of the containing directory instead of the file.
> 131: lib = PCSC_FRAMEWORK;
> 132: if (new File(lib).getParentFile().isDirectory()) {
This is outside of my normal area, could you please explain why checking the
existence of the containing directory is equivalent to checking the file here?
Does it provide the expected behavior on all unix-like platforms or only macos?
Could you please also provide a jtreg test for this change?
-
PR: https://git.openjdk.java.net/jdk/pull/2119
Integrated: 8246788: ZoneRules invariants can be broken
On Fri, 22 Jan 2021 05:39:55 GMT, Stuart Marks wrote: > Tighten up argument checking in constructor. This pull request has now been integrated. Changeset: a8871776 Author:Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/a8871776 Stats: 90 lines in 2 files changed: 87 ins; 0 del; 3 mod 8246788: ZoneRules invariants can be broken Reviewed-by: rriggs, naoto - PR: https://git.openjdk.java.net/jdk/pull/2191
Re: RFR: 8246788: ZoneRules invariants can be broken
On Fri, 22 Jan 2021 17:12:34 GMT, Daniel Fuchs wrote: >> Or even maybe `rulesArray = >> lastRules.toArray(ZoneOffsetTransitionRule[]::new);`? > > Good point - but that would be: > > ZoneOffsetTransitionRule[] rulesArray = > lastRules.toArray(ZoneOffsetTransitionRule[]::new).clone(); Interesting. This last one is more concise, but it's a bit harder to reason about. The lastRules implementation could return an array of a type other than ZOTR[]. If it's unrelated or a supertype, this would result in ClassCastException -- probably not a problem. If it were a subtype of ZOTR[], this would get stored in the object's field. Is this a problem? Turns out it can't happen, since ZOTR is final. While not wrong, I don't think this is the right idiom. It occurs to me that there should by another overload Arrays.copyOf(array, newType) that changes the type without changing the length. This would let us get rid of the local variable. - PR: https://git.openjdk.java.net/jdk/pull/2191
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v2]
On Fri, 22 Jan 2021 16:52:02 GMT, Mahendra Chhipa
wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8183372
>
> Mahendra Chhipa has updated the pull request incrementally with one
> additional commit since the last revision:
>
> NonJavaName Tests updated
> Used newInstance() method to create the different EnclosingClasses at
> runtime
test/jdk/java/lang/Class/forName/NonJavaNameTest.java line 43:
> 41: * @run testng/othervm NonJavaNameTest
> 42: * @summary Verify names that aren't legal Java names are accepted by
> forName.
> 43: * @author Joseph D. Darcy
we can drop this `@author` in particular this is a new file.
test/jdk/java/lang/Class/forName/NonJavaNameTest.java line 49:
> 47: private static final String SRC_DIR = System.getProperty("test.src");
> 48: private static final String NONJAVA_NAMES_SRC = SRC_DIR + "/classes/";
> 49: private static final String NONJAVA_NAMES_CLASSES =
> System.getProperty("test.classes", ".");
I was at first confused with `NON_NAMES_CLASSES` of what it means.For
simplicity and clarity, better to rename these variables:
s/NONJAVA_NAMES_SRC/TEST_SRC/
s/NONJAVA_NAMES_CLASSES/TEST_CLASSES/
test/jdk/java/lang/Class/forName/NonJavaNameTest.java line 96:
> 94:
> 95: @AfterClass
> 96: public void deleteInvalidNameClasses() throws IOException {
jtreg will do the clean up and this is not necessary.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 95:
> 93: Files.deleteIfExists(pkg2Dir);
> 94: Files.deleteIfExists(pkg1File);
> 95: Files.deleteIfExists(pkg1Dir);
You can consider using `jdk.test.lib.util.FileUtils` test library to remove the
entire directory.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 97:
> 95: Files.deleteIfExists(pkg1Dir);
> 96: Files.createDirectories(pkg2Dir);
> 97: } catch (IOException ex) {
The test should fail when running into any error.Using the test library
will do the retry
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 176:
> 174: }
> 175:
> 176: private void check(final Class c, final Class enc,
I see that you make all parameters in all methods final. It just adds noise.
Can you leave it out?
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 197:
> 195: c.getSimpleName(), annotation.simple(),
> 196: c.getCanonicalName(),
> 197: annotation.hasCanonical() ? annotation.canonical() :
> null);
I am guessing this whitespace in line 195-197 is not intentional? Same for
line 203-206?
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 103:
> 101: createAndWriteEnclosingClasses(enclosingPath, pkg2File,
> "pkg1.pkg2");
> 102:
> 103: String javacPath = JDKToolFinder.getJDKTool("javac");
You can use `jdk.test.lib.compiler.CompilerUtils` test library to compile the
file in process.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 71:
> 69: /*
> 70: * @test
> 71: * @bug 4992173 4992170
this needs `@modules jdk.compiler`
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 141:
> 139: }
> 140:
> 141: private void createAndWriteEnclosingClasses(final Path source, final
> Path target, final String packagePath) {
s/packagePath/packageName/
no need to declare parameters as `final`
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 147:
> 145: while ((line = br.readLine()) != null) {
> 146: if (line.contains("canonical=\"EnclosingClass")) {
> 147: line = line.replaceAll("canonical=\"EnclosingClass",
> "canonical=\"" + packagePath + ".EnclosingClass");
suggestion: declare `var className = packageName + ".EnclosingClass";` and
replace those occurrance.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 155:
> 153: bw.println(line);
> 154: }
> 155: } catch (IOException ex) {
Should not swallow the error and instead, let it propagate and the test should
fail.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Integrated: 8259707: LDAP channel binding does not work with StartTLS extension
On Thu, 14 Jan 2021 19:28:27 GMT, Alexey Bakhtin wrote: > Please review a small patch to enable LDAP TLS Channel Binding with StartTLS > Extension. > Test from the bug report and jtreg javax/naming tests are passed. This pull request has now been integrated. Changeset: 874aef4a Author:Alexey Bakhtin Committer: Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/874aef4a Stats: 74 lines in 2 files changed: 39 ins; 21 del; 14 mod 8259707: LDAP channel binding does not work with StartTLS extension Reviewed-by: mullan, dfuchs, aefimov - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v4]
On Fri, 22 Jan 2021 18:16:52 GMT, Daniel Fuchs wrote: >> Alexey Bakhtin has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update copyright year > > LGTM. Thanks for taking this on! I will sponsor this! - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v4]
On Fri, 22 Jan 2021 14:42:10 GMT, Alexey Bakhtin wrote: >> Please review a small patch to enable LDAP TLS Channel Binding with StartTLS >> Extension. >> Test from the bug report and jtreg javax/naming tests are passed. > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year LGTM. Thanks for taking this on! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2085
Integrated: 8259922 MethodHandles.collectArguments does not throw IAE if pos is outside the arity range
On Wed, 20 Jan 2021 18:29:00 GMT, Johannes Kuhn wrote: > Add explicit range check to `MethodHandles.collectArgumentsCheck`. > Added test case that exercises all cases. > > This is a behavioral change, from throwing an unspecified exception to > throwing an IllegalArgumentException, as specified. > No spec change needed, as the IllegalArgumentException is already specified > to be thrown in those cases. > > Feel free to suggest a better place for the tests. This pull request has now been integrated. Changeset: bf5e8015 Author:Johannes Kuhn Committer: Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/bf5e8015 Stats: 86 lines in 2 files changed: 86 ins; 0 del; 0 mod 8259922: MethodHandles.collectArguments does not throw IAE if pos is outside the arity range Reviewed-by: mchung - PR: https://git.openjdk.java.net/jdk/pull/2171
Re: RFR: 8246788: ZoneRules invariants can be broken
On Fri, 22 Jan 2021 15:00:17 GMT, Florent Guillaume wrote: >> src/java.base/share/classes/java/time/zone/ZoneRules.java line 263: >> >>> 261: // last rules >>> 262: Object[] temp = lastRules.toArray(); >>> 263: ZoneOffsetTransitionRule[] rulesArray = Arrays.copyOf(temp, >>> temp.length, ZoneOffsetTransitionRule[].class); >> >> LGTM. Could be replaced by: >> >> ZoneOffsetTransitionRule[] rulesArray = >> (ZoneOffsetTransitionRule[])lastRules.toArray(new >> ZoneOffsetTransitionRule[0]).clone(); >> >> if you wanted - but what you currently have is good for me. > > Or even maybe `rulesArray = > lastRules.toArray(ZoneOffsetTransitionRule[]::new);`? Good point - but that would be: ZoneOffsetTransitionRule[] rulesArray = lastRules.toArray(ZoneOffsetTransitionRule[]::new).clone(); - PR: https://git.openjdk.java.net/jdk/pull/2191
Re: RFR: 8246788: ZoneRules invariants can be broken
On Fri, 22 Jan 2021 14:48:00 GMT, Daniel Fuchs wrote: >> Tighten up argument checking in constructor. > > src/java.base/share/classes/java/time/zone/ZoneRules.java line 263: > >> 261: // last rules >> 262: Object[] temp = lastRules.toArray(); >> 263: ZoneOffsetTransitionRule[] rulesArray = Arrays.copyOf(temp, >> temp.length, ZoneOffsetTransitionRule[].class); > > LGTM. Could be replaced by: > > ZoneOffsetTransitionRule[] rulesArray = > (ZoneOffsetTransitionRule[])lastRules.toArray(new > ZoneOffsetTransitionRule[0]).clone(); > > if you wanted - but what you currently have is good for me. Or even maybe `rulesArray = lastRules.toArray(ZoneOffsetTransitionRule[]::new);`? - PR: https://git.openjdk.java.net/jdk/pull/2191
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java
On Thu, 21 Jan 2021 05:19:00 GMT, Brent Christian wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8183372
>
> Can this be done all in `EnclosingClassTest.java`, without a new
> `RunEnclosingClassTest.java`?
>
> Adding the `@BeforeClass` and `@AfterClass` methods to what's there, you may
> just need to
> change the `test()` calls to use reflection - something along the lines of:
>
>> test(`Class.forName("EnclosingClass").getDeclaredConstructor().newInstance());`
Review comments implemented.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: 8246788: ZoneRules invariants can be broken
On Fri, 22 Jan 2021 05:39:55 GMT, Stuart Marks wrote: > Tighten up argument checking in constructor. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2191
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v2]
> https://bugs.openjdk.java.net/browse/JDK-8183372 Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision: NonJavaName Tests updated Used newInstance() method to create the different EnclosingClasses at runtime - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/a02e66a5..e664bd73 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=00-01 Stats: 404 lines in 5 files changed: 186 ins; 210 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8260273: DataOutputStream writeChars optimization
On Fri, 22 Jan 2021 02:57:36 GMT, Hamlin Li wrote:
> this is minor optimization following JDK-8254078.
> based on my tests with jmh, it has better performance when apply following
> patch:
>
> diff --git a/src/java.base/share/classes/java/io/DataOutputStream.java
> b/src/java.base/share/classes/java/io/DataOutputStream.java
> index 9a9a687403c..4ea497fc7c0 100644
> --- a/src/java.base/share/classes/java/io/DataOutputStream.java
> +++ b/src/java.base/share/classes/java/io/DataOutputStream.java
> @@ -300,8 +300,9 @@ public class DataOutputStream extends FilterOutputStream
> implements DataOutput {
> int len = s.length();
> for (int i = 0 ; i < len ; i++) {
> int v = s.charAt(i);
> - out.write((v >>> 8) & 0xFF);
> - out.write((v >>> 0) & 0xFF);
> + writeBuffer[0] = (byte)(v >>> 8);
> + writeBuffer[1] = (byte)(v >>> 0);
> + out.write(writeBuffer, 0, 2);
> }
> incCount(len * 2);
> }
>
> Basically, it has better performance when apply above patch:
>
> // without writeChars optimization patch, (-XX:-UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 115.208 ± 0.327 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 276.795
> ± 0.449 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 12356.969 ± 22.427 us/op
>
> // with writeChars optimization patch, (-XX:-UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 133.706 ± 0.274 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 130.979
> ± 0.155 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 6814.272 ± 52.770 us/op
>
>
> // without writeChars optimization patch, (-XX:+UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 130.367 ± 8.759 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 37.559
> ± 0.059 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 12385.030 ± 376.560 us/op
>
> // with writeChars optimization patch, (-XX:+UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 45.494 ± 7.018 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 33.015
> ± 0.349 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 6845.549 ± 38.712 us/op
Marked as reviewed by bpb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/2190
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v4]
On Fri, 22 Jan 2021 14:42:10 GMT, Alexey Bakhtin wrote: >> Please review a small patch to enable LDAP TLS Channel Binding with StartTLS >> Extension. >> Test from the bug report and jtreg javax/naming tests are passed. > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: JDK-8260273: DataOutputStream writeChars optimization
On Fri, 22 Jan 2021 02:57:36 GMT, Hamlin Li wrote:
> this is minor optimization following JDK-8254078.
> based on my tests with jmh, it has better performance when apply following
> patch:
>
> diff --git a/src/java.base/share/classes/java/io/DataOutputStream.java
> b/src/java.base/share/classes/java/io/DataOutputStream.java
> index 9a9a687403c..4ea497fc7c0 100644
> --- a/src/java.base/share/classes/java/io/DataOutputStream.java
> +++ b/src/java.base/share/classes/java/io/DataOutputStream.java
> @@ -300,8 +300,9 @@ public class DataOutputStream extends FilterOutputStream
> implements DataOutput {
> int len = s.length();
> for (int i = 0 ; i < len ; i++) {
> int v = s.charAt(i);
> - out.write((v >>> 8) & 0xFF);
> - out.write((v >>> 0) & 0xFF);
> + writeBuffer[0] = (byte)(v >>> 8);
> + writeBuffer[1] = (byte)(v >>> 0);
> + out.write(writeBuffer, 0, 2);
> }
> incCount(len * 2);
> }
>
> Basically, it has better performance when apply above patch:
>
> // without writeChars optimization patch, (-XX:-UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 115.208 ± 0.327 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 276.795
> ± 0.449 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 12356.969 ± 22.427 us/op
>
> // with writeChars optimization patch, (-XX:-UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 133.706 ± 0.274 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 130.979
> ± 0.155 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 6814.272 ± 52.770 us/op
>
>
> // without writeChars optimization patch, (-XX:+UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 130.367 ± 8.759 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 37.559
> ± 0.059 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 12385.030 ± 376.560 us/op
>
> // with writeChars optimization patch, (-XX:+UseBiasedLocking)
> Benchmark (basicType) (size) Mode Cnt Score Error Units
> DataOutputStreamTest.dataOutputStreamOverBufferedFileStream STRING 4096 avgt
> 6 45.494 ± 7.018 us/op
> DataOutputStreamTest.dataOutputStreamOverByteArray STRING 4096 avgt 6 33.015
> ± 0.349 us/op
> DataOutputStreamTest.dataOutputStreamOverRawFileStream STRING 4096 avgt 6
> 6845.549 ± 38.712 us/op
Thanks, this should have been included in the 8254078.
-
Marked as reviewed by rriggs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2190
Re: RFR: 8246788: ZoneRules invariants can be broken
On Fri, 22 Jan 2021 05:39:55 GMT, Stuart Marks wrote: > Tighten up argument checking in constructor. src/java.base/share/classes/java/time/zone/ZoneRules.java line 263: > 261: // last rules > 262: Object[] temp = lastRules.toArray(); > 263: ZoneOffsetTransitionRule[] rulesArray = Arrays.copyOf(temp, > temp.length, ZoneOffsetTransitionRule[].class); LGTM. Could be replaced by: ZoneOffsetTransitionRule[] rulesArray = (ZoneOffsetTransitionRule[])lastRules.toArray(new ZoneOffsetTransitionRule[0]).clone(); if you wanted - but what you currently have is good for me. - PR: https://git.openjdk.java.net/jdk/pull/2191
Re: RFR: 8246788: ZoneRules invariants can be broken
On Fri, 22 Jan 2021 05:39:55 GMT, Stuart Marks wrote: > Tighten up argument checking in constructor. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2191
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v4]
> Please review a small patch to enable LDAP TLS Channel Binding with StartTLS > Extension. > Test from the bug report and jtreg javax/naming tests are passed. Alexey Bakhtin has updated the pull request incrementally with one additional commit since the last revision: Update copyright year - Changes: - all: https://git.openjdk.java.net/jdk/pull/2085/files - new: https://git.openjdk.java.net/jdk/pull/2085/files/e3bf8c36..40447456 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2085=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2085=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2085.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2085/head:pull/2085 PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen
On Thu, 21 Jan 2021 16:54:36 GMT, Paul Sandoz wrote: >>> Unfortunately it is still problematic because you have replaced the >>> intrinsic check (that performed by `Object.checksIndex`, or more >>> specifically >>> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/util/Preconditions.java#L261)). >>> >>> Further, you have replaced the bounds check options, in place for >>> experimentation. We are not ready yet to collapse our options, further >>> analysis is required as bounds checks can be very sensitive in C2. >>> >>> I would be open to you adding a third case, so that you can analyze the >>> performance without perturbing the default behavior. I suspect the correct >>> fix is to make intrinsic `Objects.checkFromIndexSize` in a similar manner >>> to `Object.checksIndex`. >> >> Hi @PaulSandoz , >> >> Thanks for your kind review and comments. >> >> To be honest, I didn't get your first point. >> As for the example above, the intrinsified Objects.checkIndex seems to >> generate more code than inlined if-statements. >> So I'm confused why it's a problem to replace the intrinsic check. >> Did you mean an intrinsic is always (or for most cases) better than >> non-intrinc or inlined if-statements? >> And why? >> >> Could you please make it more clearer to us? >> Thanks. > > The intrinsic enables C2 to more reliably elide bounds checks. I don't know > the exact details but at a high-level it transforms signed values into > unsigned values (and therefore the signed comparisons become unsigned > comparisons), which helps C2 remove checks when there is a dominating check > of, for example, an upper loop bound. > > You say "the intrinsified Objects.checkIndex seems to generate more code than > inlined if-statements". Can you present some examples of Java code and the > corresponding C2 generated assembler where this happens? Hi @PaulSandoz , I will show you the assembly code next week since it is already Friday night now. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2127
Integrated: 8259842: Remove Result cache from StringCoding
On Fri, 15 Jan 2021 14:33:19 GMT, Claes Redestad wrote: > The `StringCoding.resultCached` mechanism is used to remove the allocation of > a `StringCoding.Result` object on potentially hot paths used in some `String` > constructors. Using a `ThreadLocal` has overheads though, and the overhead > got a bit worse after JDK-8258596 which addresses a leak by adding a > `SoftReference`. > > This patch refactors much of the decode logic back into `String` and gets rid > of not only the `Result` cache, but the `Result` class itself along with the > `StringDecoder` class and cache. > > Microbenchmark results: > Baseline > > Benchmark (charsetName) Mode Cnt > ScoreError Units > decodeCharsetUS-ASCII avgt 15 > 193.043 ± 8.207 ns/op > decodeCharset:·gc.alloc.rate.normUS-ASCII avgt 15 > 112.009 ± 0.001B/op > decodeCharset ISO-8859-1 avgt 15 > 164.580 ± 6.514 ns/op > decodeCharset:·gc.alloc.rate.norm ISO-8859-1 avgt 15 > 112.009 ± 0.001B/op > decodeCharset UTF-8 avgt 15 > 328.370 ± 18.420 ns/op > decodeCharset:·gc.alloc.rate.norm UTF-8 avgt 15 > 224.019 ± 0.002B/op > decodeCharset MS932 avgt 15 > 328.870 ± 20.056 ns/op > decodeCharset:·gc.alloc.rate.norm MS932 avgt 15 > 232.020 ± 0.002B/op > decodeCharset ISO-8859-6 avgt 15 > 193.603 ± 12.305 ns/op > decodeCharset:·gc.alloc.rate.norm ISO-8859-6 avgt 15 > 112.010 ± 0.001B/op > decodeCharsetNameUS-ASCII avgt 15 > 209.454 ± 9.040 ns/op > decodeCharsetName:·gc.alloc.rate.normUS-ASCII avgt 15 > 112.009 ± 0.001B/op > decodeCharsetName ISO-8859-1 avgt 15 > 188.234 ± 7.533 ns/op > decodeCharsetName:·gc.alloc.rate.norm ISO-8859-1 avgt 15 > 112.009 ± 0.001B/op > decodeCharsetName UTF-8 avgt 15 > 399.463 ± 12.437 ns/op > decodeCharsetName:·gc.alloc.rate.norm UTF-8 avgt 15 > 224.019 ± 0.003B/op > decodeCharsetName MS932 avgt 15 > 358.839 ± 15.385 ns/op > decodeCharsetName:·gc.alloc.rate.norm MS932 avgt 15 > 208.017 ± 0.003B/op > decodeCharsetName ISO-8859-6 avgt 15 > 162.570 ± 7.090 ns/op > decodeCharsetName:·gc.alloc.rate.norm ISO-8859-6 avgt 15 > 112.009 ± 0.001B/op > decodeDefault N/A avgt 15 > 316.081 ± 11.101 ns/op > decodeDefault:·gc.alloc.rate.norm N/A avgt 15 > 224.019 ± 0.002B/op > > Patched: > Benchmark (charsetName) Mode Cnt > ScoreError Units > decodeCharsetUS-ASCII avgt 15 > 159.153 ± 6.082 ns/op > decodeCharset:·gc.alloc.rate.normUS-ASCII avgt 15 > 112.009 ± 0.001B/op > decodeCharset ISO-8859-1 avgt 15 > 134.433 ± 6.203 ns/op > decodeCharset:·gc.alloc.rate.norm ISO-8859-1 avgt 15 > 112.009 ± 0.001B/op > decodeCharset UTF-8 avgt 15 > 297.234 ± 21.654 ns/op > decodeCharset:·gc.alloc.rate.norm UTF-8 avgt 15 > 224.019 ± 0.002B/op > decodeCharset MS932 avgt 15 > 311.583 ± 16.445 ns/op > decodeCharset:·gc.alloc.rate.norm MS932 avgt 15 > 208.018 ± 0.002B/op > decodeCharset ISO-8859-6 avgt 15 > 156.216 ± 6.522 ns/op > decodeCharset:·gc.alloc.rate.norm ISO-8859-6 avgt 15 > 112.010 ± 0.001B/op > decodeCharsetNameUS-ASCII avgt 15 > 180.752 ± 9.411 ns/op > decodeCharsetName:·gc.alloc.rate.normUS-ASCII avgt 15 > 112.010 ± 0.001B/op > decodeCharsetName ISO-8859-1 avgt 15 > 156.170 ± 8.003 ns/op > decodeCharsetName:·gc.alloc.rate.norm ISO-8859-1 avgt 15 > 112.010 ± 0.001B/op > decodeCharsetName UTF-8 avgt 15 > 370.337 ± 22.199 ns/op > decodeCharsetName:·gc.alloc.rate.norm UTF-8 avgt 15 > 224.019 ± 0.001B/op > decodeCharsetName
Re: RFR: 8259842: Remove Result cache from StringCoding [v11]
On Thu, 21 Jan 2021 22:43:50 GMT, Naoto Sato wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Reduce code duplication in getBytes/getBytesNoRepl > > Marked as reviewed by naoto (Reviewer). Passed testing tiers 1-4 - PR: https://git.openjdk.java.net/jdk/pull/2102
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v3]
On Thu, 21 Jan 2021 19:57:04 GMT, Alexey Bakhtin wrote: >> Please review a small patch to enable LDAP TLS Channel Binding with StartTLS >> Extension. >> Test from the bug report and jtreg javax/naming tests are passed. > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Add comments and volatile modifier for tlsHandshakeListener Hi Alexey, The latest changes look good to me. Thanks for handling a case of sequential StartTLS requests on one LDAP context and running the modified test. I've also checked that existing LDAP tests shows no failures with the proposed changed. You might also want to update last modification years to `2021` in both files. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/2085
