Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > update LastModified LGTM. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v7]
On Sat, 26 Mar 2022 12:51:04 GMT, liach wrote: >>> You probably wanna allow for a non-NEW instance for the corner case where >>> the given size is 0 - no elements. >> >> @ChrisHegarty I guess we shouldn't. >> >> I want to make it 100% equals to `new HashMap()` constructor, thus migrate >> all usecases. >> >> So if we apply this, and when the original usage use this map object as a >> lock, or put some elements after the call(sometimes people cannot decide if >> they would really put elements in this map), bad things would happen. >> >> Besides, there already a function` Map.of()` for such functionality, so >> programmer should use that instead if they really want a shared empty map. > > hash maps are modifiable. empty instances can be changed, and returning such > shared instances are inherently unsafe. Yeah, sorry. Ignore my comment, I was wrong. I completely agree with your reasoning. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v7]
On Thu, 24 Mar 2022 17:43:31 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with two additional > commits since the last revision: > > - update jmh > - refine javadoc; refine implement when expectedSize < 0 This is a very nice addition. In Elasticsearch we have such API points, which are tedious to get right and test. src/java.base/share/classes/java/util/HashMap.java line 2584: > 2582: > 2583: /** > 2584: * Creates a new, empty HashMap with an initial table size You probably wanna allow for a non-NEW instance for the corner case where the given size is 0 - no elements. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8283683: Make ThreadLocalRandom a final class
On Fri, 25 Mar 2022 13:32:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which marks the `ThreadLocalRandom` > class as `final`? Related JBS issue > https://bugs.openjdk.java.net/browse/JDK-8283683. > > A CSR has been filed too https://bugs.openjdk.java.net/browse/JDK-8283688. > > tier1, tier2 and tier3 tests have been run with this change and no related > failures have been noticed. LGTM. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7958
Integrated: 8282444: Module finder incorrectly assumes default file system path-separator character
On Mon, 28 Feb 2022 11:12:17 GMT, Chris Hegarty wrote: > The module finder implementation incorrectly uses the path-separator > character from the default file system, when mapping the relative path > of an entry in an exploded module to a package name. This causes > problems on Windows [*] when using a module finder with a custom file > system that has a path-separator character that differs from that of the > default file system, e.g. the zip file system (which uses '/', > rather than '\' ). > > Rather than adding a new test for this, I deciced to just modify an > existing one. The existing test exercises the module finder with a > custom file system, but does so with modules have trivial single-level > packages names. I just expanded the module to have a subpackage. This > is sufficient to reproduce the bug, and verify the fix. > > [*] > > java.lang.module.FindException: Error reading module: /m2 > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) > at > java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) > at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) > at > java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) > at > java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) > at > ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) > at > ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) > at > ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) > at ... > Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r not > found in module > at > java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) > at > java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) > at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) > at > java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) > ... 36 more This pull request has now been integrated. Changeset: 369291b2 Author:Chris Hegarty URL: https://git.openjdk.java.net/jdk/commit/369291b265e13d625c5f465da9b1854c0d70c435 Stats: 10 lines in 5 files changed: 2 ins; 0 del; 8 mod 8282444: Module finder incorrectly assumes default file system path-separator character Reviewed-by: alanb - PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character [v3]
> The module finder implementation incorrectly uses the path-separator > character from the default file system, when mapping the relative path > of an entry in an exploded module to a package name. This causes > problems on Windows [*] when using a module finder with a custom file > system that has a path-separator character that differs from that of the > default file system, e.g. the zip file system (which uses '/', > rather than '\' ). > > Rather than adding a new test for this, I deciced to just modify an > existing one. The existing test exercises the module finder with a > custom file system, but does so with modules have trivial single-level > packages names. I just expanded the module to have a subpackage. This > is sufficient to reproduce the bug, and verify the fix. > > [*] > > java.lang.module.FindException: Error reading module: /m2 > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) > at > java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) > at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) > at > java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) > at > java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) > at > ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) > at > ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) > at > ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) > at ... > Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r not > found in module > at > java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) > at > java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) > at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) > at > java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) > ... 36 more Chris Hegarty has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'openjdk:master' into modulefinder_separator_win - update copyright year - add tag with OrigBugId 8178380, and bugFixId 8282444 - fix file separator in module finder with custom fs - Changes: - all: https://git.openjdk.java.net/jdk/pull/7632/files - new: https://git.openjdk.java.net/jdk/pull/7632/files/220c43c2..a7cbfd0c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7632=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7632=01-02 Stats: 3764 lines in 134 files changed: 3088 ins; 449 del; 227 mod Patch: https://git.openjdk.java.net/jdk/pull/7632.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7632/head:pull/7632 PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character
On Mon, 28 Feb 2022 14:34:43 GMT, Alan Bateman wrote: > we should create another issue to create more tests for custom file systems. Yeah, that's a good point. I'll take a closer look at this and see how best to expand the coverage (separately). > I assume you'll update the date in the copyright header of the changed files. Done. >> Hi Chris, maybe you should add @bug 8282444 to >> ModulesInCustomFileSystem.java too? > The downside is that it would make it look like the test was added fro this > issue. I think it only works if the original issue for the module system > implementation is there too. I added a tag with both the original bugId that introduced the test, 8178380, and bug Fix Id for this issue, 8282444. - PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character [v2]
> The module finder implementation incorrectly uses the path-separator > character from the default file system, when mapping the relative path > of an entry in an exploded module to a package name. This causes > problems on Windows [*] when using a module finder with a custom file > system that has a path-separator character that differs from that of the > default file system, e.g. the zip file system (which uses '/', > rather than '\' ). > > Rather than adding a new test for this, I deciced to just modify an > existing one. The existing test exercises the module finder with a > custom file system, but does so with modules have trivial single-level > packages names. I just expanded the module to have a subpackage. This > is sufficient to reproduce the bug, and verify the fix. > > [*] > > java.lang.module.FindException: Error reading module: /m2 > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) > at > java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) > at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) > at > java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) > at > java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) > at > ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) > at > ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) > at > ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) > at ... > Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r not > found in module > at > java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) > at > java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) > at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) > at > java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) > ... 36 more Chris Hegarty has updated the pull request incrementally with two additional commits since the last revision: - update copyright year - add tag with OrigBugId 8178380, and bugFixId 8282444 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7632/files - new: https://git.openjdk.java.net/jdk/pull/7632/files/ca6cdf4c..220c43c2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7632=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7632=00-01 Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7632.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7632/head:pull/7632 PR: https://git.openjdk.java.net/jdk/pull/7632
RFR: 8282444: Module finder incorrectly assumes default file system path-separator character
The module finder implementation incorrectly uses the path-separator character from the default file system, when mapping the relative path of an entry in an exploded module to a package name. This causes problems on Windows [*] when using a module finder with a custom file system that has a path-separator character that differs from that of the default file system, e.g. the zip file system (which uses '/', rather than '\' ). Rather than adding a new test for this, I deciced to just modify an existing one. The existing test exercises the module finder with a custom file system, but does so with modules have trivial single-level packages names. I just expanded the module to have a subpackage. This is sufficient to reproduce the bug, and verify the fix. [*] java.lang.module.FindException: Error reading module: /m2 at java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) at java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) at java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) at java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) at ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) at ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) at ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) at ... Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r not found in module at java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) at java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) at java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) at java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) ... 36 more - Commit messages: - fix file separator in module finder with custom fs Changes: https://git.openjdk.java.net/jdk/pull/7632/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7632=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282444 Stats: 7 lines in 4 files changed: 1 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7632.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7632/head:pull/7632 PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v12]
On Wed, 3 Nov 2021 13:23:36 GMT, Aleksei Efimov wrote: >> This change implements a new service provider interface for host name and >> address resolution, so that java.net.InetAddress API can make use of >> resolvers other than the platform's built-in resolver. >> >> The following API classes are added to `java.net.spi` package to facilitate >> this: >> - `InetAddressResolverProvider` - abstract class defining a service, and >> is, essentially, a factory for `InetAddressResolver` resolvers. >> - `InetAddressResolverProvider.Configuration ` - an interface describing the >> platform's built-in configuration for resolution operations that could be >> used to bootstrap a resolver construction, or to implement partial >> delegation of lookup operations. >> - `InetAddressResolver` - an interface that defines methods for the >> fundamental forward and reverse lookup operations. >> - `InetAddressResolver.LookupPolicy` - a class whose instances describe the >> characteristics of one forward lookup operation. >> >> More details in [JEP-418](https://openjdk.java.net/jeps/418). >> >> Testing: new and existing `tier1:tier3` tests > > Aleksei Efimov has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 18 commits: > > - Merge branch 'master' into JDK-8244202_JEP418_impl > - Merge branch 'master' into JDK-8244202_JEP418_impl > - Replace 'system' with 'the system-wide', move comment section > - Add @ throws NPE to hosts file resolver javadoc > - Changes to address review comments > - Update tests to address SM deprecation > - Merge branch 'master' into JDK-8244202_JEP418_impl > - More javadoc updates to API classes > - Review updates + move resolver docs to the provider class (CSR update to > follow) > - Change InetAddressResolver method names > - ... and 8 more: > https://git.openjdk.java.net/jdk/compare/a316c06e...0542df51 LGTM. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8268595: java/io/Serializable/serialFilter/GlobalFilterTest.java#id1 failed in timeout
On Tue, 19 Oct 2021 13:00:01 GMT, Jaikiran Pai wrote: > The `GlobalFilterTest` has to 2 `@test` tags. One of them has failed with a > timeout as noted in https://bugs.openjdk.java.net/browse/JDK-8268595. The > timeout seems to have happened even after the tests had already completed > successfully. Like I note in the JBS comments of that issue, I suspect it > might have to do with the > "-XX:StartFlightRecording:name=DeserializationEvent,dumponexit=true" usage in > this test action. > > The commit in this PR removes that second `@test` altogether because (correct > me if I'm wrong) from what I understand, this test never enables the > DeserializationEvent which means there is no JFR events being captured for > deserialization in this test, nor does the test do any JFR events related > testing. So, I think this second `@test` is virtually a no-op when it comes > to the JFR testing. There's a separate `TestDeserializationEvent` which has a > comprehensive testing of the DeserializationEvent. Marked as reviewed by chegar (Reviewer). I agree that the second `@test` is not all that useful, and can probably be just removed. If I remember correctly, I added the second `@test` during development of the feature before the more comprehensive test was added, and also when experimenting with the event being enabled by default, but it is not adding much value now. - PR: https://git.openjdk.java.net/jdk/pull/6008
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes wrote: >> This change implements a simple web server that can be run on the >> command-line with `java -m jdk.httpserver`. >> >> This is facilitated by adding an entry point for the `jdk.httpserver` >> module, an implementation class whose main method is run when the above >> command is executed. This is the first such module entry point in the JDK. >> >> The server is a minimal HTTP server that serves the static files of a given >> directory, similar to existing alternatives on other platforms and >> convenient for testing, development, and debugging. >> >> Additionally, a small API is introduced for programmatic creation and >> customization. >> >> Testing: tier1-3. > > Julia Boes has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Merge branch 'master' into simpleserver > - Merge remote-tracking branch 'origin/simpleserver' into simpleserver > - Merge branch 'master' into simpleserver > - refactor isHidden,isReadable,isSymlink checks and cleanup tests > - Merge branch 'master' into simpleserver > - check isHidden, isSymlink, isReadable for all path segments > - add checks for all path segments > - Merge branch 'master' into componentcheck > - Merge branch 'master' into simpleserver > - improve output on startup > - ... and 6 more: > https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131 Overall, this is a very nice addition. Good work. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
On Tue, 24 Aug 2021 03:03:48 GMT, Vicente Romero wrote: >> Please review this simple PR along with the associated CSR. The PR is >> basically adding a line the the specification of method >> `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a >> NPE will be thrown. >> >> TIA >> >> link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > addressing review comments Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8271396: Spelling errors
On Wed, 3 Feb 2021 19:12:25 GMT, Emmanuel Bourg wrote: > This PR fixes the following spelling errors: > > choosen -> chosen > commad -> command > hiearchy -> hierarchy > leagacy -> legacy > minium -> minimum > subsytem -> subsystem > unamed -> unnamed Trivially, looks ok to me. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2385
Re: [jdk17] RFR: 8269772: [macos-aarch64] test compilation failed with "SocketException: No buffer space available"
On Mon, 5 Jul 2021 11:21:39 GMT, Daniel Fuchs wrote: > Please find here a trivial fix for: > > 8269772: [macos-aarch64] test compilation failed with "SocketException: No > buffer space available" > > Running several of the websocket tests concurrently on some of the CI > machines is causing resource exhaustion, because resources are only freed > after the TIMED_WAIT delay has expired once the tests have finished. > The proposed solution is to run these tests in exclusive dir mode. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/210
Re: RFR: 8269383: (bf) ByteOrder should expose methods to test if platform is big or little endian
On Fri, 25 Jun 2021 13:30:56 GMT, Yi Yang wrote: > Hi, can I have a review of this change that adds two new utility methods for > java.nio.ByteOrder? Looking through the whole JDK codebase, most calls of > ByteOrder.nativeOrder() is to check if the underlying platform is > little-endian/big-endian(e.g. #4596 ). There is no reason to only provide > ByteOrder.nativeOrder while leaving big-endian/little-endian checking methods > blank. For most cases(in JDK or in user code), we want a checking(byte order) > rather than retrieving(byte order). > > Thanks! On the face of it, I do like this API enhancement. I've coded similar myself a number of times, well isBigEndian. Is there any other potential benefits, performance or otherwise, that than be achieved by such an API? - PR: https://git.openjdk.java.net/jdk/pull/4595
Re: [jdk17] RFR: JDK-8266269: Lookup::accessClass fails with IAE when accessing an arrayClass with a protected inner class as component class
On Thu, 24 Jun 2021 18:42:23 GMT, Mandy Chung wrote: > `Lookup::accessClass` should determine the accessibility of the element type. > An array class is accessible if and only if its element type is accessible. > > This also fixes a spec bug to document `@throws NullPointerException` if the > argument is null. > > Please review the CSR: > https://bugs.openjdk.java.net/browse/JDK-8269312 Marked as reviewed by chegar (Reviewer). test/jdk/java/lang/invoke/t8150782/TestFindClass.java line 42: > 40: > 41: import org.testng.annotations.*; > 42: import p.Foo; maybe a stray import? - PR: https://git.openjdk.java.net/jdk17/pull/137
Re: [jdk17] RFR: JDK-8266310: deadlock between System.loadLibrary and JNI FindClass loading another class
> On 24 Jun 2021, at 22:27, Mandy Chung wrote: > > On Fri, 18 Jun 2021 09:50:49 GMT, Aleksei Voitylov > wrote: > >> Resubmitting the following PR https://github.com/openjdk/jdk/pull/3976 >> against JDK17. >> >> This fixes the deadlock in ClassLoader between the two lock objects - a lock >> object associated with the class being loaded, and the >> ClassLoader.loadedLibraryNames hash map, locked during the native library >> load operation. >> >> Further details can be found in the original PR. >> >> Testing: jtreg and jck testing with no regressions. A new regression test >> was developed. > > This is a risky area and I agree it needs some bake time. The fix has been > ready for some time but it takes longer than we hope to get this reviewed and > approved (I was one causing the delay). I am not uncomfortable getting this > in JDK 17 but I will not object if others think this should be fixed in JDK > 18 (and backport to 17 update if desirable) as this is a long standing issue > and no urgency to get this fixed. Fixing initially in 18, allowing some “bake” time, then considering a backport to a 17 update, seems prudent. -Chris.
Re: [jdk17] RFR: JDK-8266310: deadlock between System.loadLibrary and JNI FindClass loading another class
On Fri, 18 Jun 2021 09:50:49 GMT, Aleksei Voitylov wrote: > Resubmitting the following PR https://github.com/openjdk/jdk/pull/3976 > against JDK17. > > This fixes the deadlock in ClassLoader between the two lock objects - a lock > object associated with the class being loaded, and the > ClassLoader.loadedLibraryNames hash map, locked during the native library > load operation. > > Further details can be found in the original PR. > > Testing: jtreg and jck testing with no regressions. A new regression test was > developed. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/96
Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method [v3]
On Tue, 22 Jun 2021 16:18:11 GMT, Roger Riggs wrote: >> Add java.util.Objects.newIdentity to supply a unique object with identity. >> This is a replacement code can be used today for the traditional new >> Object() idiom, which will be deprecated under Project Valhalla. >> Refer to [JEP 401: Primitive Objects >> (Preview)](https://openjdk.java.net/jeps/401) for background. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright in BasicObjectsTest Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/112
Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method [v3]
On Tue, 22 Jun 2021 16:18:11 GMT, Roger Riggs wrote:
>> Add java.util.Objects.newIdentity to supply a unique object with identity.
>> This is a replacement code can be used today for the traditional new
>> Object() idiom, which will be deprecated under Project Valhalla.
>> Refer to [JEP 401: Primitive Objects
>> (Preview)](https://openjdk.java.net/jeps/401) for background.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update copyright in BasicObjectsTest
src/java.base/share/classes/java/util/Objects.java line 492:
> 490: /**
> 491: * {@return a new instance of an unspecified class}
> 492: * The object has a unique identity; no other references to it exist.
Is this a new javadoc style/tag ?
-
PR: https://git.openjdk.java.net/jdk17/pull/112
Re: RFR: 8268469: Update java.time to use switch expressions [v4]
On Tue, 22 Jun 2021 09:58:55 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.time` >> packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - 8268469: Removed alignment of arrow operators in some cases; reverted > logic of switch/case in LocalDate > - Merge remote-tracking branch 'origin/master' into JDK-8268469 > - 8268469: Removed excessive spacing; corrected misplaced comments > - Merge remote-tracking branch 'origin/master' into JDK-8268469 > - Merge remote-tracking branch 'origin/master' into JDK-8268469 > - 8268469: Update java.time to use switch expressions Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268469: Update java.time to use switch expressions [v3]
On Wed, 16 Jun 2021 10:59:59 GMT, Stephen Colebourne
wrote:
>> Patrick Concannon has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - 8268469: Removed excessive spacing; corrected misplaced comments
>> - Merge remote-tracking branch 'origin/master' into JDK-8268469
>> - Merge remote-tracking branch 'origin/master' into JDK-8268469
>> - 8268469: Update java.time to use switch expressions
>
> src/java.base/share/classes/java/time/Month.java line 480:
>
>> 478: int leap = leapYear ? 1 : 0;
>> 479: return switch (this) {
>> 480: case JANUARY -> 1;
>
> Unnecessary alignment
The vertical alignment improves readability in these short-line cases. Removing
the spaces before the arrows will make it a little harder to discern the
difference between the cases.
-
PR: https://git.openjdk.java.net/jdk/pull/4433
Re: [jdk17] RFR: 8268080: java/util/concurrent/forkjoin/AsyncShutdownNow.java fails with java.util.concurrent.RejectedExecutionException
On Wed, 16 Jun 2021 09:57:29 GMT, Julia Boes wrote: > In the methods in question, `RejectedExecutionException` is an expected > exception that was previously unhandled (it is a `RuntimeException`, not a > subclass of `ExecutionException`). This change adds > `RejectedExecutionException` to the existing catch clause. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/74
[jdk17] Integrated: 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed"
On Fri, 11 Jun 2021 15:26:50 GMT, Chris Hegarty wrote: > There is the possibility for a race in the test, where the outbound socket > buffer is still being filled, after 5 seconds, when the main test thread > tries to close the resource scope. > > The test has been updated to set the send/receive buffer sizes in an > attempt/hint to limit the accepted/connected socket buffer sizes. Actual > buffer sizes in use will likely be larger due to TCP auto-tuning, but the > hint typically reduces the overall scaled sizes. This is primarily to > stabilize outstanding write operations. Additionally, to ameliorate the > wait-for-writebuf-to-fill situation, the dumb sleep 5secs has been replaced > with a heuristic that checks that the number of bytes written quiesces. With > these changes, the test successfully passes several thousands of runs. This pull request has now been integrated. Changeset: fe48ea9d Author:Chris Hegarty URL: https://git.openjdk.java.net/jdk17/commit/fe48ea9d7975188853bc165ce29789753f4758f2 Stats: 52 lines in 1 file changed: 46 ins; 4 del; 2 mod 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed" Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk17/pull/30
Re: [jdk17] RFR: 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed"
On Fri, 11 Jun 2021 16:27:07 GMT, Daniel Fuchs wrote: >> There is the possibility for a race in the test, where the outbound socket >> buffer is still being filled, after 5 seconds, when the main test thread >> tries to close the resource scope. >> >> The test has been updated to set the send/receive buffer sizes in an >> attempt/hint to limit the accepted/connected socket buffer sizes. Actual >> buffer sizes in use will likely be larger due to TCP auto-tuning, but the >> hint typically reduces the overall scaled sizes. This is primarily to >> stabilize outstanding write operations. Additionally, to ameliorate the >> wait-for-writebuf-to-fill situation, the dumb sleep 5secs has been replaced >> with a heuristic that checks that the number of bytes written quiesces. With >> these changes, the test successfully passes several thousands of runs. > > test/jdk/java/foreign/channels/TestAsyncSocketChannels.java line 283: > >> 281: readNBytes(asc2, bytesWritten.get()); >> 282: assertTrue(scope.isAlive()); >> 283: awaitOutstandingWrites(outstandingWriteOps); > > An alternative would be to delegate the call to latch.countDown() to the > TestHandler subclass, and call it at the end of completed() only when > outstandingWriteOps.decrementAndGet() == 0; As it stands the latch is > released with the first call to `completed` (instead of being released with > the last one) - and that's what makes this method necessary. Strictly speaking it wasn't necessary to touch this area of code to resolve the failures that we're seeing - I decided to refactor this into a separate method to improve readability of the test logic. Here again, awaiting for completion is not strictly necessary, just good practice to ensure that all threads and operations associated with the test scenario complete before the next scenario. I'll see if this can be simplified as you suggest. - PR: https://git.openjdk.java.net/jdk17/pull/30
[jdk17] RFR: 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed"
There is the possibility for a race in the test, where the outbound socket buffer is still being filled, after 5 seconds, when the main test thread tries to close the resource scope. The test has been updated to set the send/receive buffer sizes in an attempt/hint to limit the accepted/connected socket buffer sizes. Actual buffer sizes in use will likely be larger due to TCP auto-tuning, but the hint typically reduces the overall scaled sizes. This is primarily to stabilize outstanding write operations. Additionally, to ameliorate the wait-for-writebuf-to-fill situation, the dumb sleep 5secs has been replaced with a heuristic that checks that the number of bytes written quiesces. With these changes, the test successfully passes several thousands of runs. - Commit messages: - initial changes Changes: https://git.openjdk.java.net/jdk17/pull/30/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=30=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268342 Stats: 52 lines in 1 file changed: 46 ins; 4 del; 2 mod Patch: https://git.openjdk.java.net/jdk17/pull/30.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/30/head:pull/30 PR: https://git.openjdk.java.net/jdk17/pull/30
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v15]
On Tue, 8 Jun 2021 14:26:43 GMT, Roger Riggs wrote: >> JEP 415: Context-specific Deserialization Filters extends the >> deserialization filtering mechanisms with more flexible and customizable >> protections against malicious deserialization. See JEP 415: >> https://openjdk.java.net/jeps/415. >> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are >> extended with additional >> configuration mechanisms and filter utilities. >> >> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and >> `ObjectInputStream`: >> >> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Clarify exceptions that occur if initializing the filter factory from > jdk.serialFilterFactory fails Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: JDK-8266254: Update to use jtreg 6
On Wed, 2 Jun 2021 16:13:48 GMT, Jonathan Gibbons wrote: > Please review the change to update to using jtreg 6. > > The primary change is to the jib-profiles.js file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > All the tests that could be updated ahead of time have been updated. There > are a few tests remaining that need to be done at this time, because of the > change in the module name for TestNG 7.3. It changed from a default of > `testng` to and explicit `org.testng`. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v9]
On Mon, 31 May 2021 14:07:54 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.io`, >> `java.math`, and `java.text` packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Added missing brace > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Removed trailing whitespace > - 8267670: Remove redundant code from switch > - 8267670: Updated code to use yield > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Update java.io, java.math, and java.text to use switch expressions Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8266310: deadlock while loading the JNI code [v6]
On Thu, 27 May 2021 14:31:59 GMT, Aleksei Voitylov wrote: >> Please review this PR which fixes the deadlock in ClassLoader between the >> two lock objects - a lock object associated with the class being loaded, and >> the ClassLoader.loadedLibraryNames hash map, locked during the native >> library load operation. >> >> Problem being fixed: >> >> The initial reproducer demonstrated a deadlock between the JarFile/ZipFile >> and the hash map. That deadlock exists even when the ZipFile/JarFile lock is >> removed because there's another lock object in the class loader, associated >> with the name of the class being loaded. Such objects are stored in >> ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads >> exactly the same class, whose signature is being verified in another thread. >> >> Proposed fix: >> >> The proposed patch suggests to get rid of locking loadedLibraryNames hash >> map and synchronize on each entry name, as it's done with class names in see >> ClassLoader.getClassLoadingLock(name) method. >> >> The patch introduces nativeLibraryLockMap which holds the lock objects for >> each library name, and the getNativeLibraryLock() private method is used to >> lazily initialize the corresponding lock object. nativeLibraryContext was >> changed to ThreadLocal, so that in any concurrent thread it would have a >> NativeLibrary object on top of the stack, that's being currently >> loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names >> of all native libraries loaded - in line with class loading code, it is not >> explicitly cleared. >> >> Testing: jtreg and jck testing with no regressions. A new regression test >> was developed. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > address review comments Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3976
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v3]
On Tue, 25 May 2021 18:54:48 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.io`, >> `java.math`, and `java.text` packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8267670: Remove redundant code from switch Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8267587: Update java.util to use enhanced switch [v5]
On Tue, 25 May 2021 11:49:18 GMT, Tagir F. Valeev wrote:
>> Inspired by PR#4088. Most of the changes are done automatically using
>> IntelliJ IDEA refactoring. Some manual adjustments are also performed,
>> including indentations, moving comments, extracting common cast out of
>> switch expression branches, etc.
>>
>> I also noticed that there are some switches having one branch only in
>> JapaneseImperialCalendar.java. Probably it would be better to replace them
>> with `if` statement?
>
> Tagir F. Valeev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> More vertical alignment
src/java.base/share/classes/java/util/Calendar.java line 1507:
> 1505: }
> 1506: case "japanese" -> cal = new
> JapaneseImperialCalendar(zone, locale, true);
> 1507: default -> throw new IllegalArgumentException("unknown
> calendar type: " + type);
In this case, it would be good to yield the result of the switch expression and
assign that to a local, rather than assigning in each of the case arms, e.g:
final Calendar cal = switch (type) {
case "gregory" -> new GregorianCalendar(zone, locale, true);
case "iso8601" -> {
GregorianCalendar gcal = new GregorianCalendar(zone, locale, true);
// make gcal a proleptic Gregorian
gcal.setGregorianChange(new Date(Long.MIN_VALUE));
// and week definition to be compatible with ISO 8601
setWeekDefinition(MONDAY, 4);
yield gcal;
}
case "buddhist" -> {
var buddhistCalendar = new BuddhistCalendar(zone, locale);
buddhistCalendar.clear();
yield buddhistCalendar;
}
case "japanese" -> new JapaneseImperialCalendar(zone, locale, true);
default -> throw new IllegalArgumentException("unknown calendar type: "
+ type);
};
-
PR: https://git.openjdk.java.net/jdk/pull/4161
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]
On Tue, 25 May 2021 14:53:44 GMT, Patrick Concannon
wrote:
>> src/java.base/share/classes/java/io/StreamTokenizer.java line 795:
>>
>>> 793: * case statements
>>> 794: */
>>> 795: if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
>>
>> Maybe (since its easier to grok the yield rather than the assignment of ret
>> in branches):
>>
>> String ret = switch (ttype) {
>> case TT_EOF -> "EOF";
>> case TT_EOL -> "EOL";
>> case TT_WORD-> sval;
>> case TT_NUMBER -> "n=" + nval;
>> case TT_NOTHING -> "NOTHING";
>> default -> {
>> /*
>> * ttype is the first character of either a quoted string or
>> * is an ordinary character. ttype can definitely not be less
>> * than 0, since those are reserved values used in the
>> previous
>> * case statements
>> */
>> if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
>> yield sval;
>> }
>> char s[] = new char[3];
>> s[0] = s[2] = ''';
>> s[1] = (char) ttype;
>> yield new String(s);
>> }
>> };
>> return "Token[" + ret + "], line " + LINENO;
>
> Code updated as suggested. See adc8af4
The snippet above both uses yield in the default case, and also removes the
assignments from the other arms. adc8af4 overlooks the redundant assignments in
the non-default cases.
-
PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]
On Tue, 25 May 2021 14:57:22 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my code for updating the code in the `java.io`,
>> `java.math`, and `java.text` packages to make use of the switch expressions?
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request with a new target base due to
> a merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains three additional
> commits since the last revision:
>
> - 8267670: Updated code to use yield
> - Merge remote-tracking branch 'origin/master' into JDK-8267670
> - 8267670: Update java.io, java.math, and java.text to use switch expressions
src/java.base/share/classes/java/io/ObjectStreamClass.java line 2172:
> 2170: switch (typeCodes[i]) {
> 2171: case 'L', '[' -> vals[offsets[i]] =
> unsafe.getReference(obj, readKeys[i]);
> 2172: default -> throw new InternalError();
suggest:
vals[offsets[i]] = switch (typeCodes[i]) {
case 'L', '[' -> unsafe.getReference(obj, readKeys[i]);
default -> throw new InternalError();
src/java.base/share/classes/java/io/StreamTokenizer.java line 787:
> 785: case TT_WORD-> ret = sval;
> 786: case TT_NUMBER -> ret = "n=" + nval;
> 787: case TT_NOTHING -> ret = "NOTHING";
This is not correct, the assignments to ret in these case arms is redundant.
-
PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions
On Tue, 25 May 2021 09:37:58 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.io`,
> `java.math`, and `java.text` packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/io/StreamTokenizer.java line 795:
> 793: * case statements
> 794: */
> 795: if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
Maybe (since its easier to grok the yield rather than the assignment of ret in
branches):
String ret = switch (ttype) {
case TT_EOF -> "EOF";
case TT_EOL -> "EOL";
case TT_WORD-> sval;
case TT_NUMBER -> "n=" + nval;
case TT_NOTHING -> "NOTHING";
default -> {
/*
* ttype is the first character of either a quoted string or
* is an ordinary character. ttype can definitely not be less
* than 0, since those are reserved values used in the previous
* case statements
*/
if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
yield sval;
}
char s[] = new char[3];
s[0] = s[2] = ''';
s[1] = (char) ttype;
yield new String(s);
}
};
return "Token[" + ret + "], line " + LINENO;
-
PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions
On Tue, 25 May 2021 09:37:58 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.io`,
> `java.math`, and `java.text` packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/io/ObjectInputStream.java line 1877:
> 1875: descriptor.checkInitialized();
> 1876: }
> 1877: default -> throw new
> StreamCorruptedException(
What would you think of assigning descriptor with the value returned from
evaluating the switch?
Either:
1)
ObjectStreamClass descriptor = switch (tc) {
case TC_NULL-> (ObjectStreamClass) readNull();
case TC_PROXYCLASSDESC -> readProxyDesc(unshared);
case TC_CLASSDESC -> readNonProxyDesc(unshared);
case TC_REFERENCE -> readAndCheckHandle(unshared);
default -> throw new StreamCorruptedException(String.format("invalid
type code: %02X", tc));
};
return descriptor;
}
, where the body of TC_REFERENCE is enclosed in readAndCheckHandle, OR
2) Simply
case TC_REFERENCE -> {
var d = (ObjectStreamClass)readHandle(unshared);
// Should only reference initialized class descriptors
d.checkInitialized();
yield d; }
-
PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v7]
On Mon, 24 May 2021 21:57:50 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Move merge and rejectUndecidedClass methods to OIF.Config
> As default methods on OIF, their implementations were not concrete and not
> trustable
src/java.base/share/classes/java/io/ObjectInputFilter.java line 73:
> 71: * var filter =
> ObjectInputFilter.Config.createFilter("example.*;java.base/*;!*")
> 72: * ObjectInputFilter.Config.setSerialFilter(filter);
> 73: * }
It's good to have a straightforward example, but it has an implicit assumption
- that the built-in filter factory is in operation ( and will not change ). I
wonder if there is a way to update the example (without too much fuss), or
otherwise add a textual qualification. Though, I'm not sure what this would
look like.
src/java.base/share/classes/java/io/ObjectInputFilter.java line 400:
> 398: * {@link BinaryOperator {@literal
> BinaryOperator}} interface, provide its implementation and
> 399: * be accessible via the {@linkplain
> ClassLoader#getSystemClassLoader() application class loader}.
> 400: * The filter factory configured using the system or security
> property during initialization
What is the expected behaviour if the factory property is to set to a non-class
or non-accessible class? The current implementation does (it probably should be
more graceful) :
$ java -Djdk.serialFilterFactory=allow T
Exception in thread "main" java.lang.ExceptionInInitializerError
at
java.base/java.io.ObjectInputFilter$Config.(ObjectInputFilter.java:537)
at
java.base/java.io.ObjectInputStream.(ObjectInputStream.java:394)
at T.main(T.java:5)
Caused by: java.lang.ClassNotFoundException: allow
at
java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:636)
at
java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:182)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:519)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:466)
at
java.base/java.io.ObjectInputFilter$Config.(ObjectInputFilter.java:519)
... 2 more
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v7]
On Mon, 24 May 2021 21:57:50 GMT, Roger Riggs wrote: >> JEP 415: Context-specific Deserialization Filters extends the >> deserialization filtering mechanisms with more flexible and customizable >> protections against malicious deserialization. See JEP 415: >> https://openjdk.java.net/jeps/415. >> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are >> extended with additional >> configuration mechanisms and filter utilities. >> >> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and >> `ObjectInputStream`: >> >> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Move merge and rejectUndecidedClass methods to OIF.Config > As default methods on OIF, their implementations were not concrete and not > trustable The conf/security/java.security file will need to be updated as part of this change. It does not have an entry for the factory property, and also its description of jdk.serialFilter will be no longer accurate - since filter set by jdk.serialFilter may no longer have any impact on OIS, if a filter factory is specified as either a system property or security property. - PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]
On Mon, 24 May 2021 15:09:26 GMT, Roger Riggs wrote: > i) is too limiting. It should be possible for an application to check whether > a filter factory has been provided on the command line (by calling > getSerialFilterFactory) and if not setting the factory itself. It may also > want to install its own filter factory that delegates to the builtin factory > without needed to re-implement the builtin behavior. How is this supposed to work in practice? getSerialFilterFactory always returns a non-null factory, so how does one know whether or not the returned factory is the built-in factory, a factory set by the command line (or security property) ? (without resorting to implementation assumptions) - PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v5]
On Sat, 22 May 2021 20:16:37 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Editorial javadoc updated based on review comments.
> Clarified behavior of rejectUndecidedClass method.
> Example test added to check status returned from file.
src/java.base/share/classes/java/io/ObjectInputFilter.java line 829:
> 827: * Returns a filter that returns {@code Status.ALLOWED} if the
> check is for limits
> 828: * and not checking a class; otherwise {@code Status.UNDECIDED}.
> 829: * If the {@link FilterInfo#serialClass()} is {@code null}, the
> filter returns
"Returns a filter that returns {@code Status.ALLOWED} ... ". Maybe "Returns a
filter that allows all limits ..".
Up-levelling a bit, it seems that filter operations fall into two broad
categories: 1) class-checking, and 2)limits-checking. At least, in the way that
some of these factories and adapters are laid out. If we had such a notional
at the class-level, then maybe that could influence the naming of these
factories - and tie them to a higher-level concept.
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v5]
On Sat, 22 May 2021 20:16:37 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Editorial javadoc updated based on review comments.
> Clarified behavior of rejectUndecidedClass method.
> Example test added to check status returned from file.
src/java.base/share/classes/java/io/ObjectInputFilter.java line 790:
> 788: * @return {@link Status#ALLOWED} if the predicate on the class
> returns {@code true},
> 789: * otherwise {@link Status#UNDECIDED}
> 790: */
The return is "a filter that ..."
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v5]
On Sat, 22 May 2021 20:16:37 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Editorial javadoc updated based on review comments.
> Clarified behavior of rejectUndecidedClass method.
> Example test added to check status returned from file.
src/java.base/share/classes/java/io/ObjectInputFilter.java line 770:
> 768: /**
> 769: * Returns a filter that returns {@code Status.ALLOWED} if the
> predicate on the class is {@code true},
> 770: * otherwise the {@code otherStatus}.
I originally overlooked the fact that UNDECIDED can be returned by these
filters. Would it be clearer to drop "otherwise the otherStatus" ?? I also
wonder if otherStatus carries its own weight? How useful is it to return an
otherStatus that is not UNDECIDED?
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]
On Fri, 21 May 2021 17:09:00 GMT, Roger Riggs wrote: >> src/java.base/share/classes/java/io/ObjectInputFilter.java line 365: >> >>> 363: * A utility class to set and get the JVM-wide deserialization >>> filter factory, >>> 364: * the static JVM-wide filter, or to create a filter from a >>> pattern string. >>> 365: * If a JVM-wide filter factory or static JVM-wide filter is set, >>> it will determine the filter >> >> This concerns me, "A JVM-wide filter factory". I was going to suggest that >> it should be "The ..", but then realised that there can only ever be one >> present at a time, but in the lifetime of a JVM there can be two (since >> getSerialFilterFactory if invoked before setSerialFilterFactory will >> subsequently return a different JVM-wide factory). Is this intentional? It >> would great if this could be "The ..", so that setXXX can only be invoked >> successfully if getXXX has not been. This may seen somewhat insignificant, >> but the fact that the JVM-wide factory can change make the model harder >> understand. > > It is reasonable to require that the factory be set before any OIS is > constructed. > Similar to the restriction that the filter on a stream cannot be changed > after the first call to readObject. > So an IllegalStateException added to Config.setSerialFilterFactory. Ok, great. So setSerialFilterFactory cannot be successfully invoked after any of i) getSerialFilterFactory, or ii) an OIS is constructed. I don't yet see this in the code. - PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]
On Fri, 21 May 2021 17:21:15 GMT, Roger Riggs wrote:
>> src/java.base/share/classes/java/io/ObjectInputFilter.java line 107:
>>
>>> 105: * Note that the filter may be used directly or combined with
>>> other filters by the
>>> 106: * {@linkplain Config#setSerialFilterFactory(BinaryOperator)
>>> JVM-wide filter factory}.
>>> 107: *
>>
>> This list is a little confusing to me, but could be that I don't fully grok
>> it yet. getSerialFilterFactory returns THE JVM-wide factory, whether that be
>> the built-in factory implementation or not is somewhat irrelevant. No need
>> to treat them differently - one either sets a factory (in which case that is
>> the JVM-wide factory), or one does not set the factory (in which case the
>> built-in factory is the JVM-wide factory).
>
> In previous versions, calling OIS.setObjectInputFilter determined exactly the
> filter used for the stream.
> With the filter factory enhancement, the current filter factory determines
> how the argument to OIS.setObjectInputFilter is used. There are plenty of
> cases where the filter factory will combine it with other filters and the
> composite will becomes the filter for the stream.
Here is the source of my confusion. The bulleted list is enumerating how a
stream-specific filter is determined, but I see an extra step in that which
should be unnecessary. It is currently:
1. Check JVM-wide filter factory
2. If no JVM-wide, check built-in factory
3. setSerialFilterFactory
, but 1 and 2 are not separate and distinct cases - there is always a JVM-wide
deserialization filter factory. The JVM-wide deserialization filter factory is
either i) set through a property, or ii) set explicitly by an API call, or iii)
the built-in implementation.
If the initialisation of the JVM-wide deserialization filter factory is
separated out from how the stream-specific factory is determined, then I
believe that it will be easier to follow.
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]
On Thu, 20 May 2021 16:10:11 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Simplify factory interface to BinaryOperator and cleanup
> the example
src/java.base/share/classes/java/io/ObjectInputFilter.java line 280:
> 278: * filter status
> 279: */
> 280: default ObjectInputFilter rejectUndecidedClass() {
It is my understanding that an implSpec will be required for this and the other
default method.
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]
On Fri, 21 May 2021 17:25:07 GMT, Roger Riggs wrote: > The static is intended to distinguish that single filter from the others. The > static vs current distinction is part of JEP 290 from which this evolved. I can kinda grok that now, I see "current filter" in JEP 290. I think that the new terms "JVM-wide filter" and "stream-specific filter", are more accurate and straightforward to follow. > The migration to "de-serialization" from the previous "serialization" is as > yet incomplete. Ok. I assume that will be completed as part of this PR. - PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]
On Thu, 20 May 2021 16:10:11 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Simplify factory interface to BinaryOperator and cleanup
> the example
src/java.base/share/classes/java/io/ObjectInputFilter.java line 559:
> 557: * Returns the static JVM-wide deserialization filter or {@code
> null} if not configured.
> 558: *
> 559: * @return the static JVM-wide deserialization filter or {@code
> null} if not configured
Is "static" significant here? Can it be dropped? I fine myself questioning if
the "static JVM-wide" and "JVM-wide" are two different filters. If we do this
then we have just two terms: 1) the "JVM-wide deserialization filter" and 2)
the "JVM-wide deserialization filter factory".
Additionally, can you please check all occurrence of these, to ensure that they
are used consistently in all parts of the spec. I think I see
serial/serialization (without the "de" ) used in a few places.
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]
On Thu, 20 May 2021 16:10:11 GMT, Roger Riggs wrote:
>> JEP 415: Context-specific Deserialization Filters extends the
>> deserialization filtering mechanisms with more flexible and customizable
>> protections against malicious deserialization. See JEP 415:
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are
>> extended with additional
>> configuration mechanisms and filter utilities.
>>
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and
>> `ObjectInputStream`:
>>
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Simplify factory interface to BinaryOperator and cleanup
> the example
src/java.base/share/classes/java/io/ObjectInputFilter.java line 56:
> 54: *
> 55: *
> 56: * To protect the JVM against deserialization vulnerabilities,
> application developers
I would personally drop "the JVM", leaving "To protect against deserialization
..", since the protection is applicable to more than the JVM ( applications,
libraries, etc).
src/java.base/share/classes/java/io/ObjectInputFilter.java line 58:
> 56: * To protect the JVM against deserialization vulnerabilities,
> application developers
> 57: * need a clear description of the objects that can be serialized or
> deserialized
> 58: * by each component or library. For each context and use case,
> developers should
drop "serialized or" - filtering is not relevant to serializing.
src/java.base/share/classes/java/io/ObjectInputFilter.java line 75:
> 73: * protect individual contexts by providing a custom filter for each.
> When the stream
> 74: * is constructed, the filter factory can identify the execution context
> based upon
> 75: * the current thread-local state, hierarchy of callers, library, module,
> and class loader.
This list looks like it is enumerating what the filter factory does, but it is
just an example of what could be done. Maybe that needs to be made more
explicit. ".. the filter factory can identify execution context based upon,
say, ... whatever it likes .. system nanoTime, ... " Or just add "e.g."
src/java.base/share/classes/java/io/ObjectInputFilter.java line 107:
> 105: * Note that the filter may be used directly or combined with other
> filters by the
> 106: * {@linkplain Config#setSerialFilterFactory(BinaryOperator)
> JVM-wide filter factory}.
> 107: *
This list is a little confusing to me, but could be that I don't fully grok it
yet. getSerialFilterFactory returns THE JVM-wide factory, whether that be the
built-in factory implementation or not is somewhat irrelevant. No need to treat
them differently - one either sets a factory (in which case that is the
JVM-wide factory), or one does not set the factory (in which case the built-in
factory is the JVM-wide factory).
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]
On Fri, 21 May 2021 03:02:43 GMT, Brent Christian wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Simplify factory interface to BinaryOperator and
>> cleanup the example
>
> src/java.base/share/classes/java/io/ObjectInputStream.java line 193:
>
>> 191: * the state.
>> 192: *
>> 193: * The deserialization filter for a stream is determined in one of the
>> following ways:
>
> Should this line start with a `` ?
At this point in the OIS spec it would be good to introduce the idea of a
stream-specific filter. Maybe open this paragraph with some additional context,
like say:
* An {@code ObjectInputStream} has an optional stream-specific
* deserialization filter. The stream-specific deserialization filter
* is determined in one of the following ways:
then enumerate the ways in which the stream-specific deserialization filter is
determine, and drop how the filter factory can be set, etc. That is best left
to the Config.
-
PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]
On Thu, 20 May 2021 16:10:11 GMT, Roger Riggs wrote: >> JEP 415: Context-specific Deserialization Filters extends the >> deserialization filtering mechanisms with more flexible and customizable >> protections against malicious deserialization. See JEP 415: >> https://openjdk.java.net/jeps/415. >> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are >> extended with additional >> configuration mechanisms and filter utilities. >> >> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and >> `ObjectInputStream`: >> >> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Simplify factory interface to BinaryOperator and cleanup > the example src/java.base/share/classes/java/io/ObjectInputFilter.java line 365: > 363: * A utility class to set and get the JVM-wide deserialization > filter factory, > 364: * the static JVM-wide filter, or to create a filter from a pattern > string. > 365: * If a JVM-wide filter factory or static JVM-wide filter is set, it > will determine the filter This concerns me, "A JVM-wide filter factory". I was going to suggest that it should be "The ..", but then realised that there can only ever be one present at a time, but in the lifetime of a JVM there can be two (since getSerialFilterFactory if invoked before setSerialFilterFactory will subsequently return a different JVM-wide factory). Is this intentional? It would great if this could be "The ..", so that setXXX can only be invoked successfully if getXXX has not been. This may seen somewhat insignificant, but the fact that the JVM-wide factory can change make the model harder understand. - PR: https://git.openjdk.java.net/jdk/pull/3996
Re: RFR: 8266936: Add a finalization JFR event [v2]
On Tue, 18 May 2021 22:41:06 GMT, Brent Christian wrote: >> Please review this enhancement to add a new JFR event, generated whenever a >> finalizer is run. >> (The makeup is similar to the Deserialization event, >> [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).) >> >> The event's only datum (beyond those common to all jfr events) is the class >> of the object that was finalized. >> >> The Category for the event: >> `"Java Virtual Machine" / "GC" / "Finalization"` >> is what made sense to me, even though the event is generated from library >> code. >> >> Along with the new regtest, I added a run mode to the basic finalizer test >> to enable jfr. >> Automated testing looks good so far. >> >> Thanks, >> -Brent > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Test flag should be volatile My 0.02$ ;-) I really like the idea of a jdk.FinalizationStatistics event. The devil is (as always) in the details. Multiple concerns have been raised above, which benefit from being separated out: 1) Raising an event per invocation of each individual finalizer may be costly when JFR is enabled (since there could be an extremely large number of these), as well as a little unwieldy for a human observer. Aggregating invocations of finalizers and reporting periodically seems like a nice solution to this. 2) A jdk.FinalizationStatistics event that provides an aggregate count of *all* finalizer invocations seems most straightforward, but less useful. A jdk.FinalizationStatistics event that provides per-class invocation metrics seems more useful, but at the expense of a more complex event structure. Maybe model jdk.FinalizationStatistics as a tuple of Class and long (count) - periodically committing multiple jdk.FinalizationStatistics events, one event per class? ( or was the thought to somehow aggregate all these per-class metrics into a single jdk.FinalizationStatistics event? ) 3) If we keep the currently proposed semantic - capturing actual invocation/queueing counts (rather than registrations), then I see no reason why the implementation of a jdk.FinalizationStatistics needs to be in the JVM. The metrics can be captured in Java code and reported in a similar way to the container metrics (being proposed in [PR 3126][PR3126]). Surely, this would be more straightforward to implement and maintain, no? 4) The startup cost of JFR. I dunno enough about this, but what I do know is that a handler needs to be spun per Java-event, so maybe this has some bearing on the decision of no.3 above? [PR3126]: https://github.com/openjdk/jdk/pull/3126 - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. The changes in the net area look fine. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods
On Thu, 13 May 2021 10:49:21 GMT, Lance Andersen wrote: > HI all, > > Please review the fix to HashesTest.java to support running on TestNG 7.4. > > The fix adds a no-arg constructor which TestNG requires. > > The change allows the test to run with the current jtreg as well as the > upcoming jtreg > > > Best > Lance The non-static state in this test class is initialized for each of the static testXXX scenarios. An alternative could be to move said state (four fields) into a static inner class, and have each of the testXXX scenarios create an instance of that class with the test-specific path. That would also allow the addition of the no-args public constructor to HashesTest, and the testXXX methods to be made non-static. - PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8266310: deadlock while loading the JNI code
On Tue, 11 May 2021 13:10:30 GMT, Aleksei Voitylov wrote: > Please review this PR which fixes the deadlock in ClassLoader between the two > lock objects - a lock object associated with the class being loaded, and the > ClassLoader.loadedLibraryNames hash map, locked during the native library > load operation. > > Problem being fixed: > > The initial reproducer demonstrated a deadlock between the JarFile/ZipFile > and the hash map. That deadlock exists even when the ZipFile/JarFile lock is > removed because there's another lock object in the class loader, associated > with the name of the class being loaded. Such objects are stored in > ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads > exactly the same class, whose signature is being verified in another thread. > > Proposed fix: > > The proposed patch suggests to get rid of locking loadedLibraryNames hash map > and synchronize on each entry name, as it's done with class names in see > ClassLoader.getClassLoadingLock(name) method. > > The patch introduces nativeLibraryLockMap which holds the lock objects for > each library name, and the getNativeLibraryLock() private method is used to > lazily initialize the corresponding lock object. nativeLibraryContext was > changed to ThreadLocal, so that in any concurrent thread it would have a > NativeLibrary object on top of the stack, that's being currently > loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names of > all native libraries loaded - in line with class loading code, it is not > explicitly cleared. > > Testing: jtreg and jck testing with no regressions. A new regression test > was developed. Hi Aleksei, As you may know, I looked into a similar issue recently and put together a reproducer [1] (which is probably similar to what you have). The reproducer, run at the time against Oracle 11u, demonstrates the issue on the mainline too, but the deadlock is slightly different. The reason I mention it here is that the reproducer encounters the issue whether there is an attempt to load the same class or another class ( from the same jar file ). In fact, the issue is even more general, the problem is with trying to load a class, not already loaded, from a jar further down on the class path ( the class may not even exist, just that it causes the loader to walk the class path up to the jar being verified ). I filed an issue for this, which may need to be closed as a duplicate depending on the outcome of this PR [2]. [1] https://github.com/ChrisHegarty/deadlock [2] https://bugs.openjdk.java.net/browse/JDK-8266350 - PR: https://git.openjdk.java.net/jdk/pull/3976
Re: RFR: 8266155: Convert java.base to use Stream.toList() [v2]
On Wed, 28 Apr 2021 16:57:25 GMT, Ian Graves wrote: >> 8266155: Convert java.base to use Stream.toList() > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Removing redundant imports Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3734
Re: RFR: 8266155: Convert java.base to use Stream.toList()
On Tue, 27 Apr 2021 21:34:02 GMT, Ian Graves wrote: > 8266155: Convert java.base to use Stream.toList() Marked as reviewed by chegar (Reviewer). src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 6788: > 6786: } > 6787: > 6788: /** I think the import of Collectors can be dropped in this file? And maybe a few other files too? - PR: https://git.openjdk.java.net/jdk/pull/3734
Re: RFR: 8266078: Reader.read(CharBuffer) advances Reader position for read-only Charbuffers [v3]
On Wed, 28 Apr 2021 15:29:20 GMT, Brian Burkhalter wrote: >> Please consider this request to modify `Reader.read(CharBuffer)` to check >> whether the buffer is read-only before reading any characters from the >> character stream. This can happen now if the buffer is read-only. Character >> are first read thereby advancing the stream before an attempt is made to put >> them in the `CharBuffer` thus incorrectly advancing the stream position. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8266078: Remove confusing comment Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3725
Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]
On Wed, 28 Apr 2021 10:42:54 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-412 [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/412
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address first batch of review comments
Like Paul, I tracked the changes to this API in the Panama repo. Most of my
remaining comments are small and come from re-reading the API docs.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
270:
> 268:
> 269: /**
> 270: * Converts a Java string into a null-terminated C string, using the
> platform's default charset,
Sorry if this has come up before, but, is the platform's default charset the
right choice here? For other areas, we choose UTF-8 as the default. In fact,
there is a draft JEP to move the default charset to UTF-8. So if there is an
implicit need to match the underlying platform's charset then this may need to
be revisited. For now, I just want to check that this is not an accidental
reliance on the platform's default charset, but a deliberate one.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 101:
> 99: * Lifecycle and confinement
> 100: *
> 101: * Memory segments are associated to a resource scope (see {@link
> ResourceScope}), which can be accessed using
Typo?? "Memory segments are associated *with* a resource scope"
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 112:
> 110: MemoryAccess.getLong(segment); // already closed!
> 111: * }
> 112: * Additionally, access to a memory segment in subject to the
> thread-confinement checks enforced by the owning scope; that is,
Typo?? "Additionally, access to a memory segment *is* subject to ..."
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 114:
> 112: * Additionally, access to a memory segment in subject to the
> thread-confinement checks enforced by the owning scope; that is,
> 113: * if the segment is associated with a shared scope, it can be accessed
> by multiple threads; if it is associated with a confined
> 114: * scope, it can only be accessed by the thread which own the scope.
Typo?? "it can only be accessed by the thread which ownS the scope."
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 693:
> 691: */
> 692: static MemorySegment allocateNative(MemoryLayout layout,
> ResourceScope scope) {
> 693: Objects.requireNonNull(scope);
Should the allocateNative methods declare that they throw ISE, if the given
ResourceScope is not alive? ( I found myself asking this q, then considering
the behaviour of a SegmentAllocator that is asked to allocate after a RS has
been closed )
-
Marked as reviewed by chegar (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3699
Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]
On Wed, 28 Apr 2021 09:06:29 GMT, Chris Hegarty wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address first batch of review comments
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java
> line 205:
>
>> 203: * until all the resource scope handles acquired from it have been
>> {@link #release(Handle)} released}.
>> 204: * @return a resource scope handle.
>> 205: */
>
> Given recent changes, it might be good to generalise the opening sentence
> here. Maybe :
> " Acquires a resource scope handle associated with this resource scope. If
> the resource scope is explicit ... " Or some such.
The spec for the acquire method talks only of explicit resource scopes. The
comment is suggesting that the spec could be generalised a little, since it is
possible to acquire a resource scope handle associated with implicit scopes.
-
PR: https://git.openjdk.java.net/jdk/pull/3699
Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator)
On Mon, 26 Apr 2021 17:10:13 GMT, Maurizio Cimadamore
wrote:
> This PR contains the API and implementation changes for JEP-412 [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/412
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java
line 135:
> 133: } finally {
> 134:segment.scope().release(segmentHandle);
> 135: }
I do like the symmetry in this example, but just to add an alternative idiom:
`segmentHandle.scope().release(segmentHandle)`
, which guarantees to avoid release throwing and IAE
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java
line 186:
> 184: * this resource scope is confined, and this method is
> called from a thread other than the thread owning this resource scope
> 185: * this resource scope is shared and a resource associated
> with this scope is accessed while this method is called
> 186: * one or more handles (see {@link #acquire()}) associated
> with this resource scope have not been closed
Minor spec suggestion: "... associated with this resource scope have not been
*released*"
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java
line 205:
> 203: * until all the resource scope handles acquired from it have been
> {@link #release(Handle)} released}.
> 204: * @return a resource scope handle.
> 205: */
Given recent changes, it might be good to generalise the opening sentence here.
Maybe :
" Acquires a resource scope handle associated with this resource scope. If the
resource scope is explicit ... " Or some such.
-
PR: https://git.openjdk.java.net/jdk/pull/3699
Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator)
On Tue, 27 Apr 2021 18:40:24 GMT, Alan Bateman wrote: >> This PR contains the API and implementation changes for JEP-412 [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/412 > > src/java.base/share/classes/sun/nio/ch/IOUtil.java line 466: > >> 464: } >> 465: >> 466: private static final JavaNioAccess NIO_ACCESS = >> SharedSecrets.getJavaNioAccess(); > > It might be cleaner to move to acquire/release methods to their own > supporting class as it's not really IOUtil. I went back and forth on this a number of times already. I think where we landed is a reasonable place, given the current shape of the code. Scope is a private property of Buffer, and as such should be consulted anywhere where a buffer's address is being accessed. In fact, a prior prototype would not allow access to the underlying address value without the caller passing a valid handle for the buffer view's scope. It's hard to find the sweet-spot here between code reuse and safety, but the high-order bit is that the code accessing the address is auditable and testable to avoid accessing memory unsafely. Maybe there is a better alternative implementation code structure (at the cost of some duplication), but it is not obvious to me what that is (and I have given it some thought). Suggestions welcome. Note, there is a little more follow-on work to be done in this area, if we are to expand support to other non-TCP channel implementations. Maybe investigation into possible code refactorings could be done as part of that? - PR: https://git.openjdk.java.net/jdk/pull/3699
Re: RFR: 8265746: Update java.time to use instanceof pattern variable (part II) [v2]
On Fri, 23 Apr 2021 15:38:53 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review the second half of my update for the `java.time` >> package to make use of the `instanceof` pattern variable? >> >> This PR was split into two parts due to the large number of files affected. >> >> Kind regards, >> >> Patrick > > Patrick Concannon has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Removed redundant braces > - Merge remote-tracking branch 'origin/master' into JDK-8265746 > - 8265746: Update java.time to use instanceof pattern variable (part II) Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3650
Re: RFR: 8265746: Update java.time to use instanceof pattern variable (part II)
On Fri, 23 Apr 2021 10:44:30 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review the second half of my update for the `java.time` > package to make use of the `instanceof` pattern variable? > > This PR was split into two parts due to the large number of files affected. > > Kind regards, > > Patrick Marked as reviewed by chegar (Reviewer). src/java.base/share/classes/java/time/Clock.java line 623: > 621: return (obj instanceof FixedClock other > 622: && instant.equals(other.instant) > 623: && zone.equals(other.zone)); The outer set of braces is redundant. src/java.base/share/classes/java/time/Clock.java line 673: > 671: return (obj instanceof OffsetClock other > 672: && baseClock.equals(other.baseClock) > 673: && offset.equals(other.offset)); The outer set of braces is redundant. src/java.base/share/classes/java/time/ZonedDateTime.java line 2191: > 2189: && dateTime.equals(other.dateTime) > 2190: && offset.equals(other.offset) > 2191: && zone.equals(other.zone)); same here. - PR: https://git.openjdk.java.net/jdk/pull/3650
Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v7]
On Wed, 21 Apr 2021 11:06:16 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.time` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - Updated single letter pattern variable name in java/time/Duration > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Updated single letter pattern variable names > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - 8263668: Update java.time to use instanceof pattern variable Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v2]
On Mon, 12 Apr 2021 14:09:55 GMT, Conor Cleary wrote: >> ### Description >> This fix is part of a previous effort to both cleanup/modernise JNDI code, >> the details of which can be seen in >> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number >> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where >> only a single object unique to the requirements of the method is used. The >> issues these occurrences of AICs cause are highlighted below. >> >> - AICs, in the cases concerned with this fix, are used where only one >> operation is required. While AICs can be useful for more complex >> implementations (using interfaces, multiple methods needed, local fields >> etc.), Lambdas are better suited here as they result in a more readable and >> concise solution. >> >> ### Fixes >> - Where applicable, occurrences of AICs were replaced with lambdas to >> address the issues above resulting primarily in more readable/concise code. > > Conor Cleary has updated the pull request incrementally with two additional > commits since the last revision: > > - Update copyright headers > - Tidied up lambdas Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3416
Integrated: 8264827: Large mapped buffer/segment crash the VM when calling isLoaded
On Wed, 7 Apr 2021 15:45:30 GMT, Chris Hegarty wrote: > Avoid overflow when calculating the number of pages for large mapped segment > sizes. This pull request has now been integrated. Changeset: 3c9858dd Author: Chris Hegarty URL: https://git.openjdk.java.net/jdk/commit/3c9858dd Stats: 36 lines in 5 files changed: 23 ins; 0 del; 13 mod 8264827: Large mapped buffer/segment crash the VM when calling isLoaded Reviewed-by: alanb, mcimadamore - PR: https://git.openjdk.java.net/jdk/pull/3378
Re: RFR: 8264827: Large mapped buffer/segment crash the VM when calling isLoaded [v2]
> Avoid overflow when calculating the number of pages for large mapped segment > sizes. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Skip testing on 32-bit systems - Changes: - all: https://git.openjdk.java.net/jdk/pull/3378/files - new: https://git.openjdk.java.net/jdk/pull/3378/files/1816b200..1dbe4a63 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3378=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3378=00-01 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3378.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3378/head:pull/3378 PR: https://git.openjdk.java.net/jdk/pull/3378
Re: RFR: 8264827: Large mapped buffer/segment crash the VM when calling isLoaded
On Wed, 7 Apr 2021 15:45:30 GMT, Chris Hegarty wrote:
> Avoid overflow when calculating the number of pages for large mapped segment
> sizes.
Note for reviewers on the test. A 3GB file is sufficient to demonstrate the
issue in the old code, as follows (with a 4K page size, since the narrowing
cast of size is the significant issue):
jshell> int pageSize() { return 4 * 1024; }
| created method pageSize()
jshell> int pageCount(long size) { return (int)(size + (long)pageSize() - 1L) /
pageSize(); }
| created method pageCount(long)
jshell> pageCount(3L * 1024L * 1024L * 1024L)
$3 ==> -262143
-
PR: https://git.openjdk.java.net/jdk/pull/3378
RFR: 8264827: Large mapped buffer/segment crash the VM when calling isLoaded
Avoid overflow when calculating the number of pages for large mapped segment sizes. - Commit messages: - Test update - Initial changes Changes: https://git.openjdk.java.net/jdk/pull/3378/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3378=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264827 Stats: 32 lines in 5 files changed: 19 ins; 0 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/3378.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3378/head:pull/3378 PR: https://git.openjdk.java.net/jdk/pull/3378
Re: RFR: 8263754: HexFormat 'fromHex' methods should be static
On Thu, 25 Mar 2021 20:08:14 GMT, Roger Riggs wrote: > A number of HexFormat methods converting from strings to numbers do not use > delimiter, prefix, suffix, and uppercase parameters and would be more > convenient if the methods were static. > > These APIs were added early in JDK 17 and are being updated before GA. > This PR updates existing uses in the JDK but there may be compiler warnings > in non-JDK source files. > >public boolean isHexDigit(int); >public int fromHexDigit(int); >public int fromHexDigits(java.lang.CharSequence); >public int fromHexDigits(java.lang.CharSequence, int, int); >public long fromHexDigitsToLong(java.lang.CharSequence); >public long fromHexDigitsToLong(java.lang.CharSequence, int, int); Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3205
Re: RFR: 8263320: [test] Add Object Stream Formatter to work with test utility HexPrinter
On Tue, 9 Mar 2021 21:37:16 GMT, Roger Riggs wrote: > ObjectStreamPrinter is a Formatter plugin to the test library HexPrinter. > > A binary stream of serialized java objects is converted into a textual form > by parsing the header, typecodes, and interpreting the stream contents. The > formatter can be used standalone or with the HexPrinter to align the > formatted stream with the corresponding hexadecimal bytes. > > StreamDump is a test utility to pass the contents of a file to the HexPrinter > utility. The format of the file is guessed from the encoding and initial > bytes. ASN.1 and serialized object streams are supported. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2900
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v5]
On Tue, 16 Mar 2021 14:47:29 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed patch for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8263108? >> >> As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and >> `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due >> to the nature of the code involved in their static blocks. A thread dump of >> one such deadlock (reproduced using the code attached to that issue) is as >> follows: >> >> "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s >> tid=0x7ff88e01ca00 nid=0x6003 in Object.wait() [0x7a4c6000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67) >> - waiting on the Class initialization monitor for >> java.lang.constant.ConstantDescs >> at Deadlock.threadA(Deadlock.java:14) >> at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s >> tid=0x7ff88b936a00 nid=0x9b03 in Object.wait() [0x7a5c9000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145) >> - waiting on the Class initialization monitor for >> java.lang.constant.DynamicConstantDesc >> at >> java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239) >> at Deadlock.threadB(Deadlock.java:24) >> at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> The commit in this PR resolves that deadlock by moving the `canonicalMap` >> initialization in `DynamicConstantDesc`, from the static block, to a lazily >> initialized map, into the `tryCanonicalize` (private) method of the same >> class. That's the only method which uses this map. >> >> The implementation in `tryCanonicalize` carefully ensures that the deadlock >> isn't shifted from the static block to this method, by making sure that the >> `synchronization` on `DynamicConstantDesc` in this method happens only when >> it's time to write the state to the shared `canonicalMap`. This has an >> implication that the method local variable `canonDescs` can get initialized >> by multiple threads unnecessarily but from what I can see, that's the only >> way we can avoid this deadlock. This penalty of multiple threads >> initializing and then throwing away that map isn't too huge because that >> will happen only once when the `canonicalMap` is being initialized for the >> first time. >> >> An alternate approach that I thought of was to completely get rid of this >> shared cache `canonicalMap` and instead just use method level map (that gets >> initialized each time) in the `tryCanonicalize` method (thus requiring no >> locks/synchronization). I ran a JMH benchmark with the current change >> proposed in this PR and with the alternate approach of using the method >> level map. The performance number from that run showed that the approach of >> using the method level map has relatively big impact on performance (even >> when there's a cache hit in that method). So I decided to not pursue that >> approach. I'll include the benchmark code and the numbers in a comment in >> this PR, for reference. >> >> The commit in this PR also includes a jtreg testcase which (consistently) >> reproduces the deadlock without the fix and passes when this fix is applied. >> Extra manual testing has been done to verify that the classes of interest >> (noted in that testcase) are indeed getting loaded in those concurrent >> threads and not in the main thread. For future maintainers, there's a >> implementation note added on that testcase explaining why it cannot be moved >> into testng test. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Add a comment to instruct future maintainers of the code to avoid calling > DynamicConstantDesc.ofCanonical() from static initialization of > java.lang.constant.ConstantDescs Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings [v3]
On Mon, 15 Mar 2021 21:47:28 GMT, Сергей Цыпанов
wrote:
>> Hello,
>>
>> as of now `java.util.StringJoiner` still uses `char[]` as a storage for
>> joined Strings.
>>
>> This applies for the cases when all joined Strings as well as delimiter,
>> prefix and suffix contain only ASCII symbols.
>>
>> As a result when `StringJoiner.toString()` is called `byte[]` stored in
>> Strings is inflated in order to fill in `char[]` and after that `char[]` is
>> compressed when constructor of String is called:
>> String delimiter = this.delimiter;
>> char[] chars = new char[this.len + addLen];
>> int k = getChars(this.prefix, chars, 0);
>> if (size > 0) {
>> k += getChars(elts[0], chars, k);// inflate byte[]
>>
>> for(int i = 1; i < size; ++i) {
>> k += getChars(delimiter, chars, k);
>> k += getChars(elts[i], chars, k);
>> }
>> }
>>
>> k += getChars(this.suffix, chars, k);
>> return new String(chars);// compress char[] -> byte[]
>> This can be improved by utilizing new method `String.getBytes(byte[], int,
>> int, byte, int)` [introduced](https://github.com/openjdk/jdk/pull/402) in
>> [JDK-8224986](https://bugs.openjdk.java.net/browse/JDK-8254082)
>> covering both cases when resulting String is Latin1 or UTF-16
>>
>> I've prepared a patch along with benchmark proving that this change is
>> correct and brings improvement.
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class StringJoinerBenchmark {
>>
>> @Benchmark
>> public String stringJoiner(Data data) {
>> String[] stringArray = data.stringArray;
>> return Joiner.joinWithStringJoiner(stringArray);
>> }
>>
>> @State(Scope.Thread)
>> public static class Data {
>>
>> @Param({"latin", "cyrillic", "mixed"})
>> private String mode;
>>
>> @Param({"8", "32", "64"})
>> private int length;
>>
>> @Param({"5", "10", "100"})
>> private int count;
>>
>> private String[] stringArray;
>>
>> @Setup
>> public void setup() {
>> RandomStringGenerator generator = new RandomStringGenerator();
>>
>> stringArray = new String[count];
>>
>> for (int i = 0; i < count; i++) {
>> String alphabet = getAlphabet(i, mode);
>> stringArray[i] = generator.randomString(alphabet, length);
>> }
>> }
>>
>> private static String getAlphabet(int index, String mode) {
>> var latin = "abcdefghijklmnopqrstuvwxyz"; //English
>> var cyrillic = "абвгдеёжзиклмнопрстуфхцчшщьыъэюя"; // Russian
>>
>> String alphabet;
>> switch (mode) {
>> case "mixed" -> alphabet = index % 2 == 0 ? cyrillic : latin;
>> case "latin" -> alphabet = latin;
>> case "cyrillic" -> alphabet = cyrillic;
>> default -> throw new RuntimeException("Illegal mode " + mode);
>> }
>> return alphabet;
>> }
>> }
>> }
>>
>> public class Joiner {
>>
>> public static String joinWithStringJoiner(String[] stringArray) {
>> StringJoiner joiner = new StringJoiner(",", "[", "]");
>> for (String str : stringArray) {
>> joiner.add(str);
>> }
>> return joiner.toString();
>> }
>> }
>>
>>
>> (count) (length)(mode)
>> Java 14 patched Units
>> stringJoiner 5 8 latin 78.836
>> ± 0.20867.546 ± 0.500 ns/op
>> stringJoiner 532 latin 92.877
>> ± 0.42266.760 ± 0.498 ns/op
>> stringJoiner 564 latin 115.423
>> ± 0.88373.224 ± 0.289 ns/op
>> stringJoiner 10 8 latin 152.587
>> ± 0.429 161.427 ± 0.635 ns/op
>> stringJoiner 1032 latin 189.998
>> ± 0.478 164.099 ± 0.963 ns/op
>> stringJoiner 1064 latin 238.679
>> ± 1.419 176.825 ± 0.533 ns/op
>> stringJoiner100 8 latin 1215.612
>> ± 17.413 1541.802 ± 126.166 ns/op
>> stringJoiner10032 latin 1699.998
>> ± 28.407 1563.341 ± 4.439 ns/op
>> stringJoiner10064 latin 2289.388
>> ± 45.319 2215.931 ± 137.583 ns/op
>> stringJoiner 5 8 cyrillic 96.692
>> ± 0.94780.946 ± 0.371 ns/op
>> stringJoiner 532 cyrillic 107.806
>> ± 0.42984.717 ± 0.541 ns/op
>> stringJoiner 564 cyrillic 150.762
>> ± 2.26796.214 ± 1.251 ns/op
>> stringJoiner 10 8 cyrillic
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Mon, 15 Mar 2021 08:53:43 GMT, Jaikiran Pai wrote: > If you and others think that we can ignore this case, then your proposed > approach of using this lazy holder for initialization, IMO, is much cleaner > and natural to read and I will go ahead and update this PR to use it. For me, at least, the holder pattern is clearer. I'm happy with that approach. ( I don't have an objection to the alternative, just a mild preference for the holder ) - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263552: Use String.valueOf() for char-to-String conversions
On Sat, 20 Feb 2021 12:17:32 GMT, Сергей Цыпанов wrote: > This is a very simple and trivial improvement about getting rid of pointless > char wrapping into array Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2660
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8263358: Refactored missed equals method Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v3]
On Thu, 11 Mar 2021 16:42:24 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8263358: Refactored equals methods src/java.base/share/classes/java/lang/ProcessHandleImpl.java line 525: > 523: return (pid == other.pid) && > 524: (startTime == other.startTime > 525: || startTime == 0 This one can be a single expression. Otherwise, looks good. - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Fri, 12 Mar 2021 13:23:39 GMT, Peter Levart wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Follow Peter Levart's review suggestion - remove volatile
>
> This looks good to me. Maybe if you wanted the previous performance back
> (when the `cannonicalMap` field was static final and therefore JIT could
> constant-fold the Map instance), you could now annotate the field with
> `@Stable` annotation to allow JIT to produce similar code. I would also move
> the `Map.ofEntries(...)` expression into a separate private static method
> since I believe it has substantial bytecode size. `tryCanonicalize()` method
> bytecode size would then more likely fall below the inlining threshold.
What you have is probably fine, but has any consideration been given to using
the initialization-on-demand holder idiom? e.g.
static private class CanonicalMapHolder {
static final Map,
ConstantDesc>> CANONICAL_MAP
= Map.ofEntries(..);
}
-
PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8248318: Examine the use of boxing in ObjectStreamClass
On Fri, 19 Feb 2021 10:14:54 GMT, Julia Boes wrote: > This change removes some instances of superfluous boxing in > java.io.ObjectStreamClass. > Testing: tier 1-3 all clear. LGTM. Optionally (and trivially), you could update the JIRA description, from "Examine" to "Remove superfluous use of ..." ( or some such ) - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2641
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:38:03 GMT, Сергей Цыпанов wrote: >> Non-static classes hold a link to their parent classes, which in many cases >> can be avoided. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8261880: Remove static from declarations of Holder nested classes The changes in java/net look ok to me. - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261940: Fix references to IOException in BigDecimal javadoc
On Thu, 18 Feb 2021 06:03:41 GMT, Joe Darcy wrote: > Noticed by some of Jon Gibbons's doc linting & checking tooling, this > changeset fixes two javadoc issues for BigDecimal's serialization-related > methods, improving the serial form page. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2623
Integrated: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote: > This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. This pull request has now been integrated. Changeset: 3dc6f52a Author: Chris Hegarty URL: https://git.openjdk.java.net/jdk/commit/3dc6f52a Stats: 620 lines in 12 files changed: 598 ins; 5 del; 17 mod 8261160: Add a deserialization JFR event Co-authored-by: Sean Coffey Co-authored-by: Chris Hegarty Reviewed-by: coffeys, rriggs, dfuchs, egahlin - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v4]
On Thu, 11 Feb 2021 16:02:09 GMT, Roger Riggs wrote: >> Chris Hegarty has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Filter **C**onfigured > > Marked as reviewed by rriggs (Reviewer). Speaking to Erik offline, he suggested to split the `exceptionMessage` into `exceptionType` and `exceptionMessage` ( since the code was somewhat overloading the existing Sting field by using toString to force the exception class name and message into a single sting ). While the exceptionXXX field should rarely be non-null, they add very little overhead. - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v5]
> This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Split exception into type and message - Changes: - all: https://git.openjdk.java.net/jdk/pull/2479/files - new: https://git.openjdk.java.net/jdk/pull/2479/files/d764f75f..8cb8f75e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2479=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2479=03-04 Stats: 48 lines in 4 files changed: 34 ins; 6 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v3]
On Thu, 11 Feb 2021 15:45:33 GMT, Daniel Fuchs wrote:
>> Chris Hegarty has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix failing test
>
> src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 42:
>
>> 40:
>> 41: @Label("Filter configured")
>> 42: public boolean filterConfigured;
>
> Should that be "Filter Configured" for consistency?
Good catch. Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v4]
> This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Filter **C**onfigured - Changes: - all: https://git.openjdk.java.net/jdk/pull/2479/files - new: https://git.openjdk.java.net/jdk/pull/2479/files/5737ee7c..d764f75f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2479=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2479=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v3]
> This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Fix failing test - Changes: - all: https://git.openjdk.java.net/jdk/pull/2479/files - new: https://git.openjdk.java.net/jdk/pull/2479/files/8ecf63ce..5737ee7c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2479=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2479=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v2]
On Thu, 11 Feb 2021 14:27:19 GMT, Roger Riggs wrote: >> Marked as reviewed by rriggs (Reviewer). > > As proposed, events are only created if there is a serialFilter in effect > (and enabled by JFR configuration). > Being able to create the events without a serialFilter in effect would be > useful for monitoring, especially if it could be controlled by a separate JFR > configuration option. (always, never, serial-filter , etc.) I updated the PR and addressed all comments so far. Specifically: @RogerRiggs The generation of the event is independent of whether the filter is set or not. I also added a piece of state to determine if a filter is set or not. I think it could be useful to analyse all Deserialisation events to, say, ensure that there are none operating without a filter ( and the `filterStatus` state is ambitious in the `null` case ). @coffeys I would like GlobalFilterTest to run regardless of whether or not the jfr module is present, but of course running the test with jfr enabled is desirable too, so I added a separate at test tag for that case. @egahlin Excellent suggestions on the naming, etc. I adopted all. And added a test to ensure that the creation and generation of the event does not inadvertently trigger class initialization if the filter rejects the attempt ( thanks @dfuch ) @dfuch Thanks for the improved label suggestion, it is now merged in. - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v2]
> This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/2479/files - new: https://git.openjdk.java.net/jdk/pull/2479/files/ca0bcf44..8ecf63ce Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2479=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2479=00-01 Stats: 124 lines in 5 files changed: 76 ins; 2 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote: > This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. The logging in ObjectInputStream remains conditional on whether a filter is installed, which that seems reasonable since the logging is filter specific, while the JRF event is not (but both carry effectively the same information). The new jdk.Deserialization event uses a String to carry the filterStatus value. The value could be represented by its enum ordinal, but then the tool consuming the event would need to map that back to its string value to be meaningful. - PR: https://git.openjdk.java.net/jdk/pull/2479
RFR: 8261160: Add a deserialization JFR event
This issue adds a new event to improve diagnostic information of Java deserialization. The event captures the details of deserialization activity from ObjectInputStream. The event details are similar to that of the serial filter, but is agnostic of whether a filter is installed or not. The event also captures the filter status, if there is one. - Commit messages: - minor cleanup; all tests passing - Unconditionally fire a deser event regardless of filter, and add test - commit 2 - commit 1 Changes: https://git.openjdk.java.net/jdk/pull/2479/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2479=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261160 Stats: 516 lines in 12 files changed: 496 ins; 5 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261154: Memory leak in Java_java_lang_ClassLoader_defineClass0 with long class names [v2]
On Thu, 4 Feb 2021 16:14:57 GMT, Claes Redestad wrote: >> This patch resolves a potential memory leak in >> Java_java_lang_ClassLoader_defineClass0 >> >> I've not figured a good way to write a regression test. A crude way to >> manually verify the leak and the fix is provided by the added microbenchmark >> that triggers the malloc in ClassLoader.c and the associated leak. >> >> E.g., running this with `/usr/bin/time -v $BUILD_DIR/images/jdk/bin/java >> -Xmx256m -jar $BUILD_DIR/images/test/micro/benchmarks.jar >> LookupDef.*WeakClass.loadLong -f 0 -i N | grep "Maximum resident set"` >> yields: >> >> Baseline: >> N = 20 Maximum resident set size (kbytes): 544860 >> N = 50 Maximum resident set size (kbytes): 818532 >> N = 100Maximum resident set size (kbytes): 1388560 >> N = 200Maximum resident set size (kbytes): 2124296 >> Patch: >> N = 20 Maximum resident set size (kbytes): 480476 >> N = 50 Maximum resident set size (kbytes): 764040 >> N = 100Maximum resident set size (kbytes): 782920 >> N = 200Maximum resident set size (kbytes): 921272 > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Dial back cleanups and focus patch on the bug at hand LGTM. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2407
Integrated: 8257074 Update the ByteBuffers micro benchmark
On Wed, 25 Nov 2020 12:41:40 GMT, Chris Hegarty wrote: > The ByteBuffers micro benchmark seems to be a little dated. > > It should be a useful resource to leverage when analysing the performance > impact of any potential implementation changes in the byte buffer classes. > More specifically, the impact of such changes on the performance of sharp > memory access operations. > > This issue proposes to update the benchmark in the following ways to meet the > aforementioned use-case: > > 1. Remove allocation from the individual benchmarks - it just creates noise. > 2. Consolidate per-thread shared heap and direct buffers. > 3. All scenarios now use absolute memory access operations - so no state of > the shared buffers is considered. > 4. Provide more reasonable default fork, warmup, etc, out-of-the-box. > 5. There seems to have been an existing bug in the test where the size > parameter was not always considered - this is now fixed. This pull request has now been integrated. Changeset: ac276bb3 Author:Chris Hegarty URL: https://git.openjdk.java.net/jdk/commit/ac276bb3 Stats: 2771 lines in 11 files changed: 2612 ins; 9 del; 150 mod 8257074: Update the ByteBuffers micro benchmark Reviewed-by: redestad, dfuchs, jvernee, bpb - PR: https://git.openjdk.java.net/jdk/pull/1430
Re: RFR: 8257074 Update the ByteBuffers micro benchmark [v6]
> The ByteBuffers micro benchmark seems to be a little dated. > > It should be a useful resource to leverage when analysing the performance > impact of any potential implementation changes in the byte buffer classes. > More specifically, the impact of such changes on the performance of sharp > memory access operations. > > This issue proposes to update the benchmark in the following ways to meet the > aforementioned use-case: > > 1. Remove allocation from the individual benchmarks - it just creates noise. > 2. Consolidate per-thread shared heap and direct buffers. > 3. All scenarios now use absolute memory access operations - so no state of > the shared buffers is considered. > 4. Provide more reasonable default fork, warmup, etc, out-of-the-box. > 5. There seems to have been an existing bug in the test where the size > parameter was not always considered - this is now fixed. Chris Hegarty has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision: - Update copyright year range and tailing whitespace - Merge branch 'master' into bb-bench - Template generation of carrier specific micro-benchmarks. * Benchmarks are now split out into carrier specific source files * All variants and views are covered * Test scenario naming is idiomatic Even with the split by carrier, regex expressions can be used to easily run subsets the benchmarks. E.g. test all memory operations related to Short with read-only buffers: $ java -jar benchmarks.jar "org.openjdk.bench.java.nio..*Buffers.testDirect.*Short.*" -l Benchmarks: org.openjdk.bench.java.nio.ByteBuffers.testDirectLoopGetShortRO org.openjdk.bench.java.nio.ByteBuffers.testDirectLoopGetShortSwapRO org.openjdk.bench.java.nio.ShortBuffers.testDirectBulkGetShortViewRO org.openjdk.bench.java.nio.ShortBuffers.testDirectBulkGetShortViewSwapRO org.openjdk.bench.java.nio.ShortBuffers.testDirectLoopGetShortViewRO org.openjdk.bench.java.nio.ShortBuffers.testDirectLoopGetShortViewSwapRO - Merge branch 'master' into bb-bench - Add explicitly allocated heap carrier buffer tests - Replace Single with Loop - whitespace - Add additional carrier views and endianness variants. A large number of variants are now covered. The individual benchmarks conform to the following convention: test(Direct|Heap)(Bulk|Single)(Get|Put)(Byte|Char|Short|Int|Long|Float|Double)(View)?(Swap)? This allows to easily run a subset of particular interest. For example: Direct only :- "org.openjdk.bench.java.nio.ByteBuffers.testDirect.*" Char only :- "org.openjdk.bench.java.nio.ByteBuffers.test.*Char.*" Bulk only :- "org.openjdk.bench.java.nio.ByteBuffers.test.*Bulk.*" Put with Int or Long carrier :- test(Direct|Heap)(Single)(Put)(Int|Long)(View)?(Swap)?" Running all variants together is likely not all that useful, since there will be a lot of data. The param sizes are changed so as to better allow for wider carrier views. There are a lot of individual benchmark methods, but their implementation is trivial, and largely mechanical. Question: where do folk stand on having a `main` method in a benchmark - as a standalone-run sanity? I found this useful to assert the validity of the benchmark code. It can be commented out if it could somehow affect the benchmark runs. ( I omitted read-only views, since they less interesting, and we already have a lot of variants ) - Initial changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/1430/files - new: https://git.openjdk.java.net/jdk/pull/1430/files/a8e81a84..9e8014cf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1430=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1430=04-05 Stats: 123643 lines in 3239 files changed: 61246 ins; 39707 del; 22690 mod Patch: https://git.openjdk.java.net/jdk/pull/1430.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1430/head:pull/1430 PR: https://git.openjdk.java.net/jdk/pull/1430
Re: RFR: 8257074 Update the ByteBuffers micro benchmark [v4]
On Thu, 10 Dec 2020 14:01:56 GMT, Jorn Vernee wrote: >> While the updated set of scenarios covered by this benchmark is >> reasonably (and vastly improves coverage), I find myself reluctant to >> add the last remaining buffer property - read-only views. It's time to >> template the generation of this code! > > If the cases get to be too many, you might also want to consider splitting > the benchmark class into several classes that cover different cases (we did > this for the memory access benchmarks as well). That would also make it > easier to run a subset of cases in isolation. All comments to date have been addressed. Unless there are any further comments, I would like to integrate this change. - PR: https://git.openjdk.java.net/jdk/pull/1430
Re: RFR: 8259947: Optimize UnixPath.encode
On Tue, 19 Jan 2021 00:35:51 GMT, Claes Redestad wrote: > This patch improves `UnixPath.encode` by reusing `JLA.getBytesNoRepl` (which > has fast-paths for common encoding) and avoiding a `toCharArray` call on the > input by refactoring the `normalizeNativePath` code to operate on `String`. > This might have a cost on files on Mac that need additional native > normalization. > > This removes another `ThreadLocal` and a source of `SoftReference`s. Together > with the UTF-8 fast-path my UTF-8 encoded file system see substantial > speed-ups in a trivial `new File(str).toPath()` microbenchmark. I think that this looks good ( I had a similar thought when looking through this code recently, for a separate issue ). - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2135
Re: [jdk16] RFR: 8259636: Check for buffer backed by shared segment kicks in in unexpected places
On Tue, 12 Jan 2021 15:48:18 GMT, Maurizio Cimadamore wrote: > When support for shared segment was added, we decided to disable support for > buffers derived from shared segment in certain async operations, as there's > currently no way to make sure that the memory won't be reclaimed while the IO > operation is still taking place. > > After looking at the code, it seemed like the best place to put the > restriction would be sun.nio.ch.DirectBuffer::address() method, since this > method is used in a lot of places just before jumping into some piece of JNI > code. > > While I still stand by that decision, the Netty team has discovered that this > decision also affected operations such as creating slices from byte buffers > derived from shared segment - this is caused by the fact that one direct > buffer constructor (the one for views and slices) is calling the dreaded > DirectBuffer::address method. > > The fix is simple: just avoid the method call - which is very easy to do in > the case of the buffer constructor: in fact this method just returns the > value of the `address` field inside the `Buffer` class, so we can always cast > to `Buffer` and then access `address` field from there. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk16/pull/110
Re: [jdk16] RFR: 8259634: MemorySegment::asByteBuffer does not respect spatial bounds
On Tue, 12 Jan 2021 15:28:20 GMT, Maurizio Cimadamore wrote: > The byte buffers created from heap segments do not honor the javadoc - which > says that the resulting buffer size should be equal to > MemorySegment::byteSize, and that the buffer position should be zero. > > The issue is that the NIO routine we have added to create heap buffers is > using the wrong constructor, which doesn't do what we need. The fix is to > simply use the proper, more complete constructor. > > I've also re-enabled an unrelated test which was missing the @Test annotation. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk16/pull/109
Re: RFR: 8258422: Cleanup unnecessary null comparison before instanceof check in java.base [v4]
On Fri, 8 Jan 2021 13:20:38 GMT, Aleksei Efimov wrote: >> This issue is blocked by [8258657][1]. It should not be integrated until >> after 8258657 has been resolved. The JIRA issues have been linked up to >> reflect this. >> >> [1]: https://bugs.openjdk.java.net/browse/JDK-8258657 > > [JDK-8258657](https://bugs.openjdk.java.net/browse/JDK-8258657) has been > resolved. The changes reverted by > [JDK-8258696](https://bugs.openjdk.java.net/browse/JDK-8258696) could also be > re-applied to `HttpClientImpl` class. @AlekseiEfimov Yes please. Whatever is easier, include the changes to HttpClientImpl in this PR, or followup separately. Otherwise, I see no reason why this PR cannot proceed. - PR: https://git.openjdk.java.net/jdk/pull/20
