Re: RFR: 8284853: Fix various 'expected' typo [v2]
On Thu, 14 Apr 2022 09:28:17 GMT, Andrey Turbanov wrote: >> Found various typos of expected: `exepected`, `exept`, `epectedly`, >> `expeced`, `Unexpeted`, etc. > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8284853: Fix various 'expected' typo > improve test log I found [yet another typo](https://github.com/kelthuzadx/jdk/commit/acb9e15bc0bf5395d1c0875f36992f692734f948), I wonder if you can merge this into your patch so that I do not need to submit a new PR for it? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem [v2]
On Sat, 25 Sep 2021 03:38:24 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change which proposes to fix the issue >> reported in https://bugs.openjdk.java.net/browse/JDK-8273790? >> >> As noted in that issue, trying to class load >> `sun.util.calendar.CalendarSystem` and `sun.util.calendar.Gregorian` >> concurrently in separate threads can lead to a deadlock because of the >> cyclic nature of the code in their static initialization. More specifically, >> consider this case: >> >> - Thread T1 initiates a classload on the `sun.util.calendar.CalendarSystem`. >> - This gives T1 the implicit class init lock on `CalendarSystem`. >> - Consider thread T2 which at the same time initiates a classload on >> `sun.util.calendar.Gregorian` class. >> - This gives T2 a implicit class init lock on `Gregorian`. >> - T1, still holding a lock on `CalendarSystem` attempts to load `Gregorian` >> since it wants to create a (singleton) instance of `Gregorian` and assign it >> to the `static final GREGORIAN_INSTANCE` member. Since T2 is holding a class >> init lock on `Gregorian`, T1 ends up waiting >> - T2 on the other hand is still loading the `Gregorian` class. `Gregorian` >> itself "is a" `CalendarSystem`, so during this loading of `Gregorian` class, >> T2 starts travelling up the class hierarchy and asks for a lock on >> `CalendarSystem`. However T1 is holding this lock and as a result T2 ends up >> waiting on T1 which is waiting on T2. That triggers this deadlock. >> >> The linked JBS issue has a thread dump which shows this in action. >> >> The commit here delays the instance creation of `Gregorian` by moving that >> instance creation logic from the static initialization of the >> `CalendarSystem` class, to the first call to >> `CalendarSystem#getGregorianCalendar()`. This prevents the `CalendarSystem` >> from needing a lock on `Gregorian` during its static init (of course, unless >> some code in this static init flow calls >> `CalendarSystem#getGregorianCalendar()`, in which case it is back to square >> one. I have verified, both manually and through the jtreg test, that the >> code in question doesn't have such calls) >> >> A new jtreg test has been introduced to reproduce the issue and verify the >> fix. The test in addition to loading these 2 classes in question, also >> additionally loads a few other classes concurrently. These classes have >> specific static initialization which leads the calls to >> `CalendarSystem#getGregorianCalendar()` or `CalendarSystem#forName()`. >> Including these classes in the tests ensures that this deadlock hasn't >> "moved" to a different location. I have run multiple runs (approximately 25) >> of this test with the fix and I haven't seen it deadlock anymore. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Minor test adjustments as suggested by Naoto > - use a holder instead of volatile, as suggested by Roger @jaikiran Thanks for fixing this. Delaying instance creation via a static holder class seems reasonable to me. - Marked as reviewed by yyang (Committer). PR: https://git.openjdk.java.net/jdk/pull/5683
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:22:38 GMT, wxiang wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> With some discussions about the bug, the current priority is to remove the >> JAR index support in URLClassPath, >> and don’t need to do anything to the jar tool in the short term, except just >> to move JarIndex to the jdk.jartool module. >> >> The PR includes: >> 1. remove the JarIndex support in URLClassPath >> 2. move JarIndex into jdk.jartool module. > > wxiang has updated the pull request incrementally with one additional commit > since the last revision: > > Some minor changes > > Include: > 1. remove public for INDEX_NAME in JarFile > 2. recover the definition for INDEX_NAME in JarIndex > 3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar Hi all, Just wondering why we remove JarIndex other than fixing it? Is there any strong motive that drives us to do this? Judging from our internal performance testing and user feedback, JarIndex can significantly reduce the time for class/resource lookup. Although JarIndex is an ancient technology, it is still helpful for many modern scenarios such as serverless applications. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/5383
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 Hi, I've filed https://bugs.openjdk.java.net/browse/JDK-8271396 for this PR, you can change the title to "8271396: Spelling errors", openjdk bot will link this PR to the corresponding issue. Also you should resolve conflicts and pass jcheck before integrating it. - PR: https://git.openjdk.java.net/jdk/pull/2385
[jdk17] Integrated: 8270056: Generated lambda class can not access protected static method of target class
On Tue, 13 Jul 2021 03:06:12 GMT, Yi Yang wrote: > Hi all, > > this pull request contains a backport of commit 07e90524 from the openjdk/jdk > repository. > > The commit being backported was authored by Yi Yang on 13 Jul 2021 and was > reviewed by Mandy Chung. > > Thanks! This pull request has now been integrated. Changeset: 0f547071 Author:Yi Yang URL: https://git.openjdk.java.net/jdk17/commit/0f5470715e98e222474f575abc95457682d5818a Stats: 324 lines in 3 files changed: 181 ins; 141 del; 2 mod 8270056: Generated lambda class can not access protected static method of target class Reviewed-by: mchung Backport-of: 07e90524576f159fc16523430f1db62327c89a3b - PR: https://git.openjdk.java.net/jdk17/pull/245
[jdk17] RFR: 8270056: Generated lambda class can not access protected static method of target class
Hi all, this pull request contains a backport of commit 07e90524 from the openjdk/jdk repository. The commit being backported was authored by Yi Yang on 13 Jul 2021 and was reviewed by Mandy Chung. Thanks! - Commit messages: - Backport 07e90524576f159fc16523430f1db62327c89a3b Changes: https://git.openjdk.java.net/jdk17/pull/245/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=245=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270056 Stats: 324 lines in 3 files changed: 181 ins; 141 del; 2 mod Patch: https://git.openjdk.java.net/jdk17/pull/245.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/245/head:pull/245 PR: https://git.openjdk.java.net/jdk17/pull/245
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v11]
On Thu, 8 Jul 2021 03:12:24 GMT, Yi Yang wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Yi Yang has refreshed the contents of this pull request, and previous commits > have been removed. The incremental views will show differences compared to > the previous content of the PR. Thanks Mandy and Roger for reviews! - PR: https://git.openjdk.java.net/jdk/pull/4507
Integrated: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base
On Wed, 16 Jun 2021 08:08:47 GMT, Yi Yang wrote:
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex
> by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in
> the whole JDK codebase.
This pull request has now been integrated.
Changeset: afe957cd
Author: Yi Yang
URL:
https://git.openjdk.java.net/jdk/commit/afe957cd9741810a113ea165a635a117c0ea556f
Stats: 357 lines in 40 files changed: 73 ins; 171 del; 113 mod
8268698: Use Objects.check{Index,FromToIndex,FromIndexSize} for java.base
Reviewed-by: mchung, rriggs
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Integrated: 8270056: Generated lambda class can not access protected static method of target class
On Thu, 8 Jul 2021 02:32:45 GMT, Yi Yang wrote: > Generated lambda class can not access protected static method of the target > class. The following exception is thrown when executing the attached > reproducible program: > > > Exception in thread "main" java.lang.IllegalAccessError: class > AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 tried to > access protected method 'void AccessProtectedStaticMethodFromSuper$A.func()' > (AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 is in > unnamed module of loader AccessProtectedStaticMethodFromSuper$1Loader > @71dac704; AccessProtectedStaticMethodFromSuper$A is in unnamed module of > loader AccessProtectedStaticMethodFromSuper$1Loader @39ed3c8d) > at > AccessProtectedStaticMethodFromSuper.main(AccessProtectedStaticMethodFromSuper.java:51) > > > This issue is similar to JDK-8254975(#767) with slight differences: generated > lambda proxy calls static protected method rather than protected member > method. > > The proposed fix 1) tries to use MethodHandle instead of invoking forwardee > directly(since the lambda class has no access to the resolved method) and 2) > does not force accepting an implClass as the first argument when invoking a > static method. > > Testing: > - test/jdk/java/ with release mode > - presubmit tests This pull request has now been integrated. Changeset: 07e90524 Author:Yi Yang URL: https://git.openjdk.java.net/jdk/commit/07e90524576f159fc16523430f1db62327c89a3b Stats: 324 lines in 3 files changed: 181 ins; 141 del; 2 mod 8270056: Generated lambda class can not access protected static method of target class Co-authored-by: NekoCaffeine Reviewed-by: mchung - PR: https://git.openjdk.java.net/jdk/pull/4714
Re: RFR: 8270056: Generated lambda class can not access protected static method of target class [v3]
On Mon, 12 Jul 2021 02:57:26 GMT, Yi Yang wrote: >> Generated lambda class can not access protected static method of the target >> class. The following exception is thrown when executing the attached >> reproducible program: >> >> >> Exception in thread "main" java.lang.IllegalAccessError: class >> AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 tried >> to access protected method 'void >> AccessProtectedStaticMethodFromSuper$A.func()' >> (AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 is in >> unnamed module of loader AccessProtectedStaticMethodFromSuper$1Loader >> @71dac704; AccessProtectedStaticMethodFromSuper$A is in unnamed module of >> loader AccessProtectedStaticMethodFromSuper$1Loader @39ed3c8d) >> at >> AccessProtectedStaticMethodFromSuper.main(AccessProtectedStaticMethodFromSuper.java:51) >> >> >> This issue is similar to JDK-8254975(#767) with slight differences: >> generated lambda proxy calls static protected method rather than protected >> member method. >> >> The proposed fix 1) tries to use MethodHandle instead of invoking forwardee >> directly(since the lambda class has no access to the resolved method) and 2) >> does not force accepting an implClass as the first argument when invoking a >> static method. >> >> Testing: >> - test/jdk/java/ with release mode >> - presubmit tests > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > mistake Thanks Mandy for review! - PR: https://git.openjdk.java.net/jdk/pull/4714
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v11]
On Thu, 8 Jul 2021 03:12:24 GMT, Yi Yang wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Yi Yang has refreshed the contents of this pull request, and previous commits > have been removed. The incremental views will show differences compared to > the previous content of the PR. I'm not familiar with the review process of core-lib group. Is it sufficient for merging with one approval? Should I have more reviews for this? 樂 - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8270056: Generated lambda class can not access protected static method of target class [v3]
> Generated lambda class can not access protected static method of the target > class. The following exception is thrown when executing the attached > reproducible program: > > > Exception in thread "main" java.lang.IllegalAccessError: class > AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 tried to > access protected method 'void AccessProtectedStaticMethodFromSuper$A.func()' > (AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 is in > unnamed module of loader AccessProtectedStaticMethodFromSuper$1Loader > @71dac704; AccessProtectedStaticMethodFromSuper$A is in unnamed module of > loader AccessProtectedStaticMethodFromSuper$1Loader @39ed3c8d) > at > AccessProtectedStaticMethodFromSuper.main(AccessProtectedStaticMethodFromSuper.java:51) > > > This issue is similar to JDK-8254975(#767) with slight differences: generated > lambda proxy calls static protected method rather than protected member > method. > > The proposed fix 1) tries to use MethodHandle instead of invoking forwardee > directly(since the lambda class has no access to the resolved method) and 2) > does not force accepting an implClass as the first argument when invoking a > static method. > > Testing: > - test/jdk/java/ with release mode > - presubmit tests Yi Yang has updated the pull request incrementally with one additional commit since the last revision: mistake - Changes: - all: https://git.openjdk.java.net/jdk/pull/4714/files - new: https://git.openjdk.java.net/jdk/pull/4714/files/8aa4e1d7..1d4ad2ca Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4714=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4714=01-02 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4714.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4714/head:pull/4714 PR: https://git.openjdk.java.net/jdk/pull/4714
Re: RFR: 8270056: Generated lambda class can not access protected static method of target class [v2]
> Generated lambda class can not access protected static method of the target > class. The following exception is thrown when executing the attached > reproducible program: > > > Exception in thread "main" java.lang.IllegalAccessError: class > AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 tried to > access protected method 'void AccessProtectedStaticMethodFromSuper$A.func()' > (AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 is in > unnamed module of loader AccessProtectedStaticMethodFromSuper$1Loader > @71dac704; AccessProtectedStaticMethodFromSuper$A is in unnamed module of > loader AccessProtectedStaticMethodFromSuper$1Loader @39ed3c8d) > at > AccessProtectedStaticMethodFromSuper.main(AccessProtectedStaticMethodFromSuper.java:51) > > > This issue is similar to JDK-8254975(#767) with slight differences: generated > lambda proxy calls static protected method rather than protected member > method. > > The proposed fix 1) tries to use MethodHandle instead of invoking forwardee > directly(since the lambda class has no access to the resolved method) and 2) > does not force accepting an implClass as the first argument when invoking a > static method. > > Testing: > - test/jdk/java/ with release mode > - presubmit tests Yi Yang has updated the pull request incrementally with one additional commit since the last revision: rename SuperMethodTest -> ProtectedMethodInOtherPackage; add test case within it; - Changes: - all: https://git.openjdk.java.net/jdk/pull/4714/files - new: https://git.openjdk.java.net/jdk/pull/4714/files/840e39f3..8aa4e1d7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4714=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4714=00-01 Stats: 416 lines in 3 files changed: 178 ins; 238 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4714.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4714/head:pull/4714 PR: https://git.openjdk.java.net/jdk/pull/4714
RFR: 8270057: Use Objects.checkFromToIndex for j.u.c.CopyOnWriteArrayList
After JDK-8265518(#3615), it's possible to replace all variants of checkIndex by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in the whole JDK codebase. As Mandy suggested, I create this PR for changes involving JUC changes. - Commit messages: - replace Changes: https://git.openjdk.java.net/jdk/pull/4723/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4723=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270057 Stats: 6 lines in 1 file changed: 0 ins; 3 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4723.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4723/head:pull/4723 PR: https://git.openjdk.java.net/jdk/pull/4723
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v11]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: restore JSR166; restore java.desktop - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/903f0305..a9e7dbc8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=09-10 Stats: 49 lines in 7 files changed: 24 ins; 7 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v8]
On Wed, 7 Jul 2021 17:08:19 GMT, Mandy Chung wrote: >>> Does "client changes" means changes involving src/java.desktop and >>> test/java/awt? >> >> src/java.desktop, test/java/awt, and test/javax/imageio > >> > src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java >> > needs to be updated in JSR 166 upstream repo. Better to file a separate >> > issue for this change to ensure that gets fixed in the upstream project. >> >> Can you please paste a link for that? I'm not sure where I can find JSR 166 >> upstream repo.. > > What I meant is to file a JBS issue for this change and revert the change in > this patch. That can be fixed when the next JSR 166 changes are imported to > JDK. > > I wasn't sure if this is the right repo: > http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/ Okay. I've filed https://bugs.openjdk.java.net/browse/JDK-8270057 and https://bugs.openjdk.java.net/browse/JDK-8270058 for JSR 166 and client code respectively. These codes have been restored. (Sorry for force-pushing, my mistake..) - PR: https://git.openjdk.java.net/jdk/pull/4507
RFR: 8270056: Generated lambda class can not access protected static method of target class
Generated lambda class can not access protected static method of the target class. The following exception is thrown when executing the attached reproducible program: Exception in thread "main" java.lang.IllegalAccessError: class AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 tried to access protected method 'void AccessProtectedStaticMethodFromSuper$A.func()' (AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 is in unnamed module of loader AccessProtectedStaticMethodFromSuper$1Loader @71dac704; AccessProtectedStaticMethodFromSuper$A is in unnamed module of loader AccessProtectedStaticMethodFromSuper$1Loader @39ed3c8d) at AccessProtectedStaticMethodFromSuper.main(AccessProtectedStaticMethodFromSuper.java:51) This issue is similar to JDK-8254975(#767) with slight differences: generated lambda proxy calls static protected method rather than protected member method. The proposed fix 1) tries to use MethodHandle instead of invoking forwarded directly(since the lambda class has no access to the resolved method) and 2) does not force accepting an implClass as the first argument when invoking a static method. - Commit messages: - proposed fix Changes: https://git.openjdk.java.net/jdk/pull/4714/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4714=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270056 Stats: 102 lines in 2 files changed: 100 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4714.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4714/head:pull/4714 PR: https://git.openjdk.java.net/jdk/pull/4714
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v10]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with two additional commits since the last revision: - restore code format - restore code format - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/f43ffc3a..903f0305 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=08-09 Stats: 36 lines in 2 files changed: 0 ins; 6 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]
On Tue, 6 Jul 2021 19:06:43 GMT, Mandy Chung wrote: >> Yi Yang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> tests rely on IOOBE exception message > > test/jdk/java/lang/StringBuffer/Exceptions.java line 73: > >> 71: new StringBuffer(); >> 72: } >> 73: }); > > Nit: The above formatting (line 70-97) is inconsistent with the formatting in > line 110-124. It'd be good to use the same formatting. Hi Mandy, thanks for reviewing this. > I suggest to separate the client changes (both src and test) in a separate PR > for the client review. Does "client changes" means changes involving src/java.desktop and test/java/awt? > src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java > needs to be updated in JSR 166 upstream repo. Better to file a separate issue > for this change to ensure that gets fixed in the upstream project. Can you please paste a link for that? I'm not sure where I can find JSR 166 upstream repo.. > Nit: The above formatting (line 70-97) is inconsistent with the formatting in > line 110-124. It'd be good to use the same formatting. Restored. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v9]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with four additional commits since the last revision: - restore code format - restore code format - restore code format - restore code format - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/ab1b509d..f43ffc3a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=07-08 Stats: 58 lines in 2 files changed: 0 ins; 10 del; 48 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Mon, 5 Jul 2021 06:01:23 GMT, Yi Yang wrote: >> Class loading order is different to class initialization order. >> >> There are a lot more tests than just tier1. :) I don't expect many, if any, >> tests to be looking for a specific IOOBE message, and I can't see an easy >> way to find such tests without running them. If core-libs folk are okay >> with this update then I guess we can just handle any test failures if they >> arise. But I'll run this through our tier 1-3 testing. >> >> Thanks, >> David > > @dholmes-ora @AlanBateman @PaulSandoz do you have any comments on the latest > version? Thanks. > @kelthuzadx I did not see any response to my query about the change in > initialization (not load) order. I also remain concerned about introducing > cross dependencies between core classes (e.g. String now uses Precondition, > so does that impact Precondition using String?) But these are things for the > core-libs folk to be concerned about, or not. > > Cheers, > David Hi David, the following are initialization orders: $./java -Xlog:class+init -version [0.255s][info][class,init] 0 Initializing 'java/lang/Object'(no method) (0x00080e18) [0.255s][info][class,init] 1 Initializing 'java/lang/CharSequence'(no method) (0x00080006ba68) [0.255s][info][class,init] 2 Initializing 'java/lang/String' (0x0008e978) [0.255s][info][class,init] 3 Initializing 'java/util/Comparator'(no method) (0x0008000fd760) [0.255s][info][class,init] 4 Initializing 'java/lang/String$CaseInsensitiveComparator'(no method) (0x000800130988) [0.255s][info][class,init] 5 Initializing 'java/lang/System' (0x00088c68) [0.256s][info][class,init] 6 Initializing 'java/lang/reflect/AnnotatedElement'(no method) (0x00081f88) [0.256s][info][class,init] 7 Initializing 'java/lang/reflect/Type'(no method) (0x00082930) [0.256s][info][class,init] 8 Initializing 'java/lang/Class' (0x00081830) [0.256s][info][class,init] 9 Initializing 'java/lang/ThreadGroup'(no method) (0x00080001d348) [0.257s][info][class,init] 10 Initializing 'java/lang/Thread' (0x00080001c470) [0.258s][info][class,init] 11 Initializing 'java/security/AccessController' (0x00089a80) [0.258s][info][class,init] 12 Initializing 'java/security/AccessControlContext' (0x0008000743c8) [0.258s][info][class,init] 13 Initializing 'java/lang/Module' (0x0008bab0) [0.259s][info][class,init] 14 Initializing 'java/lang/Module$ArchivedData' (0x0008001a05a8) [0.259s][info][class,init] 15 Initializing 'jdk/internal/misc/CDS' (0x000800018bf8) [0.259s][info][class,init] 16 Initializing 'java/lang/Iterable'(no method) (0x00080008be28) [0.259s][info][class,init] 17 Initializing 'java/util/Collection'(no method) (0x00080008c028) [0.260s][info][class,init] 18 Initializing 'java/util/AbstractCollection'(no method) (0x0008000381e0) [0.260s][info][class,init] 19 Initializing 'java/util/ImmutableCollections$AbstractImmutableCollection'(no method) (0x00080008c2f8) [0.260s][info][class,init] 20 Initializing 'java/util/Set'(no method) (0x0008000189f8) [0.260s][info][class,init] 21 Initializing 'java/util/ImmutableCollections$AbstractImmutableSet'(no method) (0x00080008d018) [0.260s][info][class,init] 22 Initializing 'java/util/ImmutableCollections$Set12'(no method) (0x00080008ba18) [0.369s][info][class,init] 23 Initializing 'jdk/internal/misc/UnsafeConstants' (0x0008000b4ea0) [0.369s][info][class,init] 24 Initializing 'java/lang/reflect/AccessibleObject' (0x00080005b4d8) [0.370s][info][class,init] 25 Initializing 'java/lang/reflect/ReflectAccess'(no method) (0x000800013610) [0.370s][info][class,init] 26 Initializing 'jdk/internal/access/SharedSecrets' (0x000800013408) [0.370s][info][class,init] 27 Initializing 'java/lang/invoke/MethodHandles' (0x000800131720) [0.371s][info][class,init] 28 Initializing 'java/lang/invoke/MemberName' (0x0008000de230) [0.371s][info][class,init] 29 Initializing 'java/lang/invoke/MemberName$Factory' (0x000800023468) [0.372s][info][class,init] 30 Initializing 'java/security/Permission'(no method) (0x000800023020) [0.372s][info][class,init] 31 Initializing 'java/security/BasicPermission'(no method) (0x000800025de8) [0.372s][info][class,init] 32 Initializing 'java/lang/reflect/ReflectPermission'(no method) (0x000800025b98) [0.372s][info][class,init] 33 Initializing 'java/lang/StringLatin1' (0x000800022e18) [0.373s][info][class,init] 34 Initializing 'java/lang/invoke/MethodHandles$Lookup' (0x0008000e3708) [0.373s][info][class,init] 35 Initializing 'java/util/Objects'(no method) (0x00080008b810) [0.374s][info][class,init] 36 Initializing 'jdk/internal/reflect/Reflection' (0x0008fb28) [0.374s][info][class,init] 37 Initializing 'java/util/ImmutableCollections
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Thu, 17 Jun 2021 05:16:14 GMT, David Holmes wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Class loading order is different to class initialization order. > > There are a lot more tests than just tier1. :) I don't expect many, if any, > tests to be looking for a specific IOOBE message, and I can't see an easy way > to find such tests without running them. If core-libs folk are okay with > this update then I guess we can just handle any test failures if they arise. > But I'll run this through our tier 1-3 testing. > > Thanks, > David @dholmes-ora @AlanBateman @PaulSandoz do you have any comments on the latest version? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4507
Withdrawn: 8269384: Determine native byte order for StringUTF16 via ByteOrder
On Fri, 25 Jun 2021 13:40:54 GMT, Yi Yang wrote: > Prefer using ByteOrder to compute byte order for StringUTF16 to determining > byte order by native method StringUTF16.isBigEndian. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4596
Re: RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder
On Fri, 25 Jun 2021 13:40:54 GMT, Yi Yang wrote: > Prefer using ByteOrder to compute byte order for StringUTF16 to determining > byte order by native method StringUTF16.isBigEndian. Thanks for the detailed clarification! The purpose of this PR is to skip the native call and use ByteOrder. Now I'm convinced that we should not change the initialization order casually, this will break some "future code" and make UnsafeConstants unable to use String. - PR: https://git.openjdk.java.net/jdk/pull/4596
Withdrawn: 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! This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4595
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! Okay, it seems that more people do not agree with adding these APIs, I am going to close this proposal. - PR: https://git.openjdk.java.net/jdk/pull/4595
Re: RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder
On Fri, 25 Jun 2021 13:40:54 GMT, Yi Yang wrote:
> Prefer using ByteOrder to compute byte order for StringUTF16 to determining
> byte order by native method StringUTF16.isBigEndian.
Hi Aleksey, do you have a concrete issue/discussion about bootstrapping
problems? I don't see it because I can build JDK and passes tier1 tests w/o
problems. But I admit this may cause potential problems. In order to reduce
potential risks, how about changing the class initialization orders, i.e.
initialize UUnsafeConstants_klas earlier, which seems reasonable:
void Threads::initialize_java_lang_classes(JavaThread* main_thread, TRAPS) {
TraceTime timer("Initialize java.lang classes", TRACETIME_LOG(Info,
startuptime));
if (EagerXrunInit && Arguments::init_libraries_at_startup()) {
create_vm_init_libraries();
}
+#ifdef ASSERT
+ InstanceKlass *k = vmClasses::UnsafeConstants_klass();
+ assert(k->is_not_initialized(), "UnsafeConstants should not already be
initialized");
+#endif
+
+ // initialize the hardware-specific constants needed by Unsafe
+ initialize_class(vmSymbols::jdk_internal_misc_UnsafeConstants(), CHECK);
+ jdk_internal_misc_UnsafeConstants::set_unsafe_constants();
initialize_class(vmSymbols::java_lang_String(), CHECK);
// Inject CompactStrings value after the static initializers for String ran.
java_lang_String::set_compact_strings(CompactStrings);
...
-
PR: https://git.openjdk.java.net/jdk/pull/4596
Re: RFR: 8269383: New java.nio.ByteOrder.isBigEndian and isLittleEndian methods
Hi David, In my humble option, it is reasonable to provide APIs to check whether the underlying platform is big-endian/little-endian.For most cases, we want a checking(byte order) rather than retrieving(byte order). -- From:David Lloyd Send Time:2021 Jun. 25 (Fri.) 21:52 To:Yi Yang Cc:core-libs-dev ; nio-dev Subject:Re: RFR: 8269383: New java.nio.ByteOrder.isBigEndian and isLittleEndian methods Is this better than the current solution of `nativeOrder() == BIG_ENDIAN` other than reducing a few keystrokes? On Fri, Jun 25, 2021 at 8:45 AM 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. There is no reason to only provide > ByteOrder.nativeOrder while leaving big-endian/little-endian checking methods > blank. > > Thanks! > > - > > Commit messages: > - add new methods > > Changes: https://git.openjdk.java.net/jdk/pull/4595/files > Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4595=00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8269383 > Stats: 20 lines in 1 file changed: 20 ins; 0 del; 0 mod > Patch: https://git.openjdk.java.net/jdk/pull/4595.diff > Fetch: git fetch https://git.openjdk.java.net/jdk pull/4595/head:pull/4595 > > PR: https://git.openjdk.java.net/jdk/pull/4595 > -- - DML • he/him
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! Hi Chris, > Is there any other potential benefits, performance or otherwise, that than be > achieved by such an API? Unfortunately not. It's more like the enhancement of API expressiveness. Since these operations are trivial, these APIs will not improve/degrade the performance. Although we can use `@IntrinsicCandidate` to intrinsify it for potential? performance benefit, but I don't think it's necessary at present(and in future...), but I think they are good candidates of `@ForceInline` annotations. - PR: https://git.openjdk.java.net/jdk/pull/4595
RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder
Prefer using ByteOrder to compute byte order for StringUTF16 to determining byte order by native method StringUTF16.isBigEndian. - Commit messages: - replace Changes: https://git.openjdk.java.net/jdk/pull/4596/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4596=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269384 Stats: 17 lines in 2 files changed: 3 ins; 13 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4596.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4596/head:pull/4596 PR: https://git.openjdk.java.net/jdk/pull/4596
RFR: 8269383: New java.nio.ByteOrder.isBigEndian and isLittleEndian methods
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. There is no reason to only provide ByteOrder.nativeOrder while leaving big-endian/little-endian checking methods blank. Thanks! - Commit messages: - add new methods Changes: https://git.openjdk.java.net/jdk/pull/4595/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4595=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269383 Stats: 20 lines in 1 file changed: 20 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4595.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4595/head:pull/4595 PR: https://git.openjdk.java.net/jdk/pull/4595
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: tests rely on IOOBE exception message - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/2330ee38..ab1b509d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=06-07 Stats: 104 lines in 2 files changed: 18 ins; 2 del; 84 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v7]
On Tue, 22 Jun 2021 02:39:01 GMT, Yi Yang wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > correct exception type; use anonymous inner classes I found that after solving the problem that Preconditions cannot be used during the VM startup, a series of functions such as String.checkIndex/checkOffset/.. can also be harmlessly replaced, but this changeset is somewhat large and may overwrite the previous review progress. If it will confuse the current review progress for reviewers involving in this PR, I'd like to file a new PR to do this change later. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v7]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: correct exception type; use anonymous inner classes - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/3a8875ec..2330ee38 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=05-06 Stats: 21 lines in 1 file changed: 0 ins; 9 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v6]
On Mon, 21 Jun 2021 20:49:56 GMT, Paul Sandoz wrote: >> Yi Yang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> more replacement 2 > > src/java.base/share/classes/jdk/internal/util/Preconditions.java line 78: > >> 76: = Preconditions.outOfBoundsExceptionFormatter(new >> StringIndexOutOfBoundsExceptionProducer()); >> 77: >> 78: public static final BiFunction, >> StringIndexOutOfBoundsException> AIOOBE_FORMATTER > > Using incorrect exception type. Suggest you embed as inner class rather than > separate declaration, since they are only used in one place. Fixed. FYI: Current exception message looks like this: Exception in thread "main" java.lang.StringIndexOutOfBoundsException: Range [3, 1) out of bounds for length 6 at CheckIndex$StringIndexOutOfBoundsExceptionProducer.apply(CheckIndex.java:77) at CheckIndex$StringIndexOutOfBoundsExceptionProducer.apply(CheckIndex.java:72) at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:159) at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:156) at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:62) at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckFromToIndex(Preconditions.java:76) at java.base/jdk.internal.util.Preconditions.checkFromToIndex(Preconditions.java:295) at CheckIndex.main(CheckIndex.java:110) I think now it expresses more exception information than before(and more consistent). - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v6]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: more replacement 2 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/c8b2106e..3a8875ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=04-05 Stats: 32 lines in 2 files changed: 1 ins; 20 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v5]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: more replacements - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/154989a0..c8b2106e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=03-04 Stats: 59 lines in 8 files changed: 11 ins; 37 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v4]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: format - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/c1dd2f76..154989a0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=02-03 Stats: 15 lines in 1 file changed: 4 ins; 1 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v3]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: new Preconditions.IOOBE_FORMATTER - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/ec0a4d44..c1dd2f76 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=01-02 Stats: 129 lines in 13 files changed: 43 ins; 40 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Fri, 18 Jun 2021 18:03:44 GMT, Paul Sandoz wrote:
>> Yi Yang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> restore IndexOfOufBoundsException; split exception line
>
> src/java.base/share/classes/java/util/Base64.java line 935:
>
>> 933: throw new IOException("Stream is closed");
>> 934: Preconditions.checkFromIndexSize(len, off, b.length,
>> 935:
>> Preconditions.outOfBoundsExceptionFormatter(ArrayIndexOutOfBoundsException::new));
>
> `outOfBoundsExceptionFormatter` will allocate. Store the result of
> `Preconditions.outOfBoundsExceptionFormatter(ArrayIndexOutOfBoundsException::new))`
> in a public static final on `Preconditions`, and replace the method ref with
> a inner class (thereby making it usable earlier at VM startup.
Thanks for the clarification! Fixed.
This incremental change does many stuff:
- Create inner classes and public static final fields within Preconditions
- Use Preconditions.check* in j.l.String
- Use Preconditions.*IOOBE_FORMATTER in java.util.zip.* classes
- Use Preconditions.*IOOBE_FORMATTER in java.util.Base64
- Use Preconditions.*IOOBE_FORMATTER in X-VarHandle.java.template and
X-VarHandleByteArrayView.java.template
- Use Preconditions.*IOOBE_FORMATTER in sun.security.provider.DigestBase
- Use Preconditions.*IOOBE_FORMATTER in sun.security.util.ArrayUtil
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Fri, 18 Jun 2021 05:54:01 GMT, Yi Yang wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > restore IndexOfOufBoundsException; split exception line > _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on > [serviceability-dev](mailto:serviceability-...@mail.openjdk.java.net):_ > > On 17/06/2021 8:50 pm, Alan Bateman wrote: > > > On Thu, 17 Jun 2021 05:16:14 GMT, David Holmes > > wrote: > > > There are a lot more tests than just tier1. :) I don't expect many, if > > > any, tests to be looking for a specific IOOBE message, and I can't see an > > > easy way to find such tests without running them. If core-libs folk are > > > okay with this update then I guess we can just handle any test failures > > > if they arise. But I'll run this through our tier 1-3 testing. > > > > > > It would be good to run tier 1-3. Off hand I can't think of any tests that > > are dependent on the exception message, I think I'm more concerned about > > changing behavior (once you throw a more specific IOOBE is some of the very > > core classes then it's hard to change it because libraries get dependent on > > the more specific exception). > > tiers 1-3 on Linux-x64 passed okay. > > Any change to the exact type of exception thrown should be affirmed > through a CSR request. > > Cheers, > David Thank you David for running these tests! - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Thu, 17 Jun 2021 15:45:47 GMT, Paul Sandoz wrote:
>> src/java.base/share/classes/java/util/Base64.java line 934:
>>
>>> 932: if (closed)
>>> 933: throw new IOException("Stream is closed");
>>> 934: Preconditions.checkFromIndexSize(len, off, b.length, (xa,
>>> xb) -> new ArrayIndexOutOfBoundsException());
>>
>> You might want to split this really long line too, to avoid inconsistent
>> line length in this source file.
>
> I would suggest factoring out `(xa, xb) -> new
> ArrayIndexOutOfBoundsException()` as a reusable component on `Preconditions`,
> and maybe even supplying an exception message (since it is rather useful, and
> way better than no message).
>
> See the usages of `Preconditions.outOfBoundsExceptionFormatter` (where there
> just happens to be many repeated cases of supplying AIOOBE with a message,
> that could also be consolidated, separate fix perhaps).
Ok, I've replaced
Preconditions.checkFromIndexSize(len, off, b.length, (xa, xb) -> new
ArrayIndexOutOfBoundsException());
with
Preconditions.checkFromIndexSize(len, off, b.length,
Preconditions.outOfBoundsExceptionFormatter(ArrayIndexOutOfBoundsException::new));
I am curious about the motivations of current APIs:
public static
int checkIndex(int index, int length,
BiFunction, X> oobef) {
if (index < 0 || index >= length)
throw outOfBoundsCheckIndex(oobef, index, length);
return index;
}
Are they over-engineered? When I checked all checkIndex-like patterns in the
whole jdk codebase, I found that in most cases, providing an API that accepts
custom exception should be enough for scalability:
int checkIndex(int index, int length, IndexOutOfBoundException iooe) {
if (index < 0 || index >= length)
throw iooe;
return index;
}
In addition to the ease-of-use problem, there is another problem with the
current API design.
Some methods in j.l.String are good replacement candidates for
Preconditions.check{Index,...}:
https://github.com/openjdk/jdk/blob/a051e735cda0d5ee5cb6ce0738aa549a7319a28c/src/java.base/share/classes/java/lang/String.java#L4558-L4604
But we **can not** do such replacement after my practice.
The third parameter of Preconditions.checkIndex is a BiFunction, which can be
passed a lambda as its argument. The generated lambda method exercises many
other codes like MethodHandles, j.l.Package, etc, eventually it called
j.l.String.checkIndex itself, so if we replace j.l.String.checkIndex with
`Preconditions.checkIndex(a,b,(x1,x2)->)`, a StackOverflowError would occur
at VM startup.
If there is an API that accepts user-defined exceptions, I think we can apply
Preconditions in more places.
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: restore IndexOfOufBoundsException; split exception line - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/593bf995..ec0a4d44 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=00-01 Stats: 30 lines in 8 files changed: 15 ins; 2 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Thu, 17 Jun 2021 10:19:43 GMT, Alan Bateman wrote:
>> Yi Yang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> restore IndexOfOufBoundsException; split exception line
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 471:
>
>> 469: */
>> 470: public int offsetByCodePoints(int index, int codePointOffset) {
>> 471: checkOffset(index, count);
>
> String.offsetByCodePoints is specified to throw IOOBE. checkOffset may throw
> the more specific StringIndexOutOfBoundsException. That's a compatible change
> but I worry that we might want to throw the less specific exception in the
> future. I only mention it because there have been cases in java.lang where
> IOOBE was specified and AIOOBE by the implementation and it has been
> problematic to touch the implementation as a result.
Yes, changing the type of thrown exception may break something. And as David
said, this also requires a CSR approval, which is a relatively long process, so
I restored the original code.
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Thu, 17 Jun 2021 01:51:41 GMT, David Holmes wrote: > I skimmed through all these and the changes seem fine in principal. > I have two mild concerns: > > 1. How does this change the class initialization order on VM startup? > 2. Do any tests need adjusting due to potential changes in the exact message > used by the IndexOutOfBoundsException? > > Thanks, > David Hi David, your concerns are reasonable. I think this change would not affect the class initialization order, because regardless of whether the patch is applied or not, `java -Xlog:class+load -version` prints identical class initialization order(for j.l.Objects and jdk.internal.util.Preconditions) as far as I can see. [class_load.log](https://github.com/openjdk/jdk/files/6667168/class_load.log). And tier1 tests are all passed w/o any modifications. - PR: https://git.openjdk.java.net/jdk/pull/4507
RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
After JDK-8265518, it's possible to replace all variants of checkIndex by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in the whole JDK codebase. - Commit messages: - use checkIndex globally Changes: https://git.openjdk.java.net/jdk/pull/4507/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4507=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268698 Stats: 206 lines in 34 files changed: 31 ins; 111 del; 64 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v13]
On Sat, 12 Jun 2021 08:22:32 GMT, Thomas Stuefe wrote: > Hi Yi, > > you may need to add the option to the obsolete-flags-table though as > described in arguments.cpp: > > https://github.com/openjdk/jdk/blob/5cee23a9ed0b7fe2657be7492d9c1f78fcd02ebf/src/hotspot/share/runtime/arguments.cpp#L489-L490 > > I think the point is to give a customer a grace period where the option is > still accepted on the command line. I am not sure if that step is optional > though, if one is reasonably sure that the option is unused. Maybe > @dholmes-ora can chime in. > > Cheers, Thomas Hi Thomas, I think what you said is right. It does not take too much time to do this but it can give users a smooth transition for unavailable options! I will create a new PR to do this stuff if there are no objections. Thanks, Yang - PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v13]
On Sat, 12 Jun 2021 06:50:48 GMT, Thomas Stuefe wrote: > This change removed a product flag so I wonder how it could be integrated > without a CSR? It's a diagnostic product flag, so it’ okay to remove it without issuing CSR. But I am not 100% sure. @dholmes-ora, do you have any comment about this? Thanks! - PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v13]
On Fri, 11 Jun 2021 18:05:45 GMT, Igor Veresov wrote: > I guess you need to do the "integrate" command again. Okay，thank you all for taking time to look at this - PR: https://git.openjdk.java.net/jdk/pull/3615
Integrated: 8265518: C1: Intrinsic support for Preconditions.checkIndex
On Thu, 22 Apr 2021 06:55:41 GMT, Yi Yang wrote: > The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk This pull request has now been integrated. Changeset: 5cee23a9 Author:Yi Yang URL: https://git.openjdk.java.net/jdk/commit/5cee23a9ed0b7fe2657be7492d9c1f78fcd02ebf Stats: 347 lines in 11 files changed: 250 ins; 78 del; 19 mod 8265518: C1: Intrinsic support for Preconditions.checkIndex Reviewed-by: dfuchs, iveresov - PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v13]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk Yi Yang has updated the pull request incrementally with one additional commit since the last revision: more comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/3615/files - new: https://git.openjdk.java.net/jdk/pull/3615/files/63f1c30d..87d8b399 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3615=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3615=11-12 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v12]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk Yi Yang has updated the pull request incrementally with two additional commits since the last revision: - c1 can not handle 0 constant value when using cmp - fix - Changes: - all: https://git.openjdk.java.net/jdk/pull/3615/files - new: https://git.openjdk.java.net/jdk/pull/3615/files/289d752c..63f1c30d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3615=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3615=10-11 Stats: 51 lines in 1 file changed: 23 ins; 17 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v6]
On Tue, 1 Jun 2021 17:43:45 GMT, Igor Veresov wrote: >> Thank you @veresov! >> >> I'm glad to have more comments from hotspot-compiler group. >> >> Later, I'd like to integrate it if there are no more comments/objections. >> >> Thanks! >> Yang > > @kelthuzadx Sorry about the delay. Could you please rebase this to the > current master and I'll push it. Thanks! @veresov I've rebased to the latest commit and resolved all conflicts. Please take a look at this. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v11]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk Yi Yang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - x86_32 fails - build failed - cmp clobbers its left argument on x86_32 - better check1-4 - AssertionError when expected exception was not thrown - remove InlineNIOCheckIndex flag - remove java_nio_Buffer in javaClasses.hpp - consolidate - Changes: https://git.openjdk.java.net/jdk/pull/3615/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3615=10 Stats: 338 lines in 11 files changed: 242 ins; 78 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v10]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk Yi Yang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - x86_32 fails - build failed - cmp clobbers its left argument on x86_32 - better check1-4 - AssertionError when expected exception was not thrown - remove extra newline - remove InlineNIOCheckIndex flag - remove java_nio_Buffer in javaClasses.hpp - consolidate - Changes: https://git.openjdk.java.net/jdk/pull/3615/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3615=09 Stats: 338 lines in 11 files changed: 242 ins; 78 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v6]
On Fri, 30 Apr 2021 19:20:54 GMT, Igor Veresov wrote: >> Yi Yang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> better check1-4 > > Looks like now the test fails in the pre-submit tests? Thank you @veresov! I'm glad to have more comments from hotspot-compiler group. Later, I'd like to integrate it if there are no more comments/objections. Thanks! Yang - PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v6]
On Fri, 30 Apr 2021 19:20:54 GMT, Igor Veresov wrote: >> Yi Yang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> better check1-4 > > Looks like now the test fails in the pre-submit tests? Hi @veresov, may I ask your help to review this patch? Thanks a lot. - PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v6]
On Fri, 30 Apr 2021 19:20:54 GMT, Igor Veresov wrote: > Looks like now the test fails in the pre-submit tests? Hi Igor, Can you take a look at the latest version? Now it passes all pre-submit tests. Best Regards, Yang - PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v6]
On Fri, 30 Apr 2021 17:30:33 GMT, Paul Sandoz wrote: > It was my hope this would eventually happen when we added > `Objects.checkIndex` and the underlying intrinsic. Very good! Hi Paul, Thank you for noticing this PR. > It was my hope this would eventually happen when we added > `Objects.checkIndex` and the underlying intrinsic. Yes, this patch adds C1 intrinsic supports for checkIndex, I will replace all variants of checkIndex with Objects.checkIndex in follow-up PRs. It seems that Object.checkIndex can not meet our needs because it implicitly passes null to Preconditions.checkIndex, but we want to customize exception messages, so we might add extra APIs in Objects while doing the replacement. > Very good! Thank you Paul~ Best Regards, Yang - PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v9]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk Yi Yang has updated the pull request incrementally with one additional commit since the last revision: x86_32 fails - Changes: - all: https://git.openjdk.java.net/jdk/pull/3615/files - new: https://git.openjdk.java.net/jdk/pull/3615/files/f996c99f..307d7a10 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3615=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3615=07-08 Stats: 9 lines in 1 file changed: 7 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v8]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk Yi Yang has updated the pull request incrementally with one additional commit since the last revision: build failed - Changes: - all: https://git.openjdk.java.net/jdk/pull/3615/files - new: https://git.openjdk.java.net/jdk/pull/3615/files/e4959148..f996c99f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3615=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3615=06-07 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v7]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk Yi Yang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - cmp clobbers its left argument on x86_32 - Merge branch 'master' into consolidate_checkindex - better check1-4 - AssertionError when expected exception was not thrown - remove extra newline - remove InlineNIOCheckIndex flag - remove java_nio_Buffer in javaClasses.hpp - consolidate - Changes: https://git.openjdk.java.net/jdk/pull/3615/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3615=06 Stats: 331 lines in 11 files changed: 235 ins; 78 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v6]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk Yi Yang has updated the pull request incrementally with one additional commit since the last revision: better check1-4 - Changes: - all: https://git.openjdk.java.net/jdk/pull/3615/files - new: https://git.openjdk.java.net/jdk/pull/3615/files/1a96be7e..954abc6e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3615=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3615=04-05 Stats: 32 lines in 1 file changed: 31 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v5]
On Thu, 29 Apr 2021 10:13:05 GMT, Daniel Fuchs wrote:
>> Yi Yang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> AssertionError when expected exception was not thrown
>
> test/hotspot/jtreg/compiler/c1/TestCheckIndexC1Intrinsic.java line 94:
>
>> 92: static void check1(int i) {
>> 93: try {
>> 94: Preconditions.checkIndex(i, , (s, integers) -> new
>> RuntimeException("ex"));
>
> I believe
>
> throw new AssertionError("Expected IndexOutOfBoundsException not thrown");
>
>
> should be added in `check1`...`check4` as well...
Hmm.. They would throw desired exceptions only if i==.
-
PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v5]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk Yi Yang has updated the pull request incrementally with one additional commit since the last revision: AssertionError when expected exception was not thrown - Changes: - all: https://git.openjdk.java.net/jdk/pull/3615/files - new: https://git.openjdk.java.net/jdk/pull/3615/files/bbdf4b9e..1a96be7e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3615=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3615=03-04 Stats: 8 lines in 1 file changed: 5 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v4]
On Thu, 29 Apr 2021 09:30:50 GMT, Daniel Fuchs wrote:
>> Yi Yang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove extra newline
>
> test/hotspot/jtreg/compiler/c1/TestCheckIndexC1Intrinsic.java line 60:
>
>> 58: } catch (IndexOutOfBoundsException e) {
>> 59: // got it!
>> 60: }
>
> In all places where `Precondition.checkIndex` is expected to throw, an
> AssertionError should be generated if it doesn't throw:
>
>
> try {
> Preconditions.checkIndex(1, 1, null);
> throw new AssertionError("Expected IndexOutOfBoundsException not
> thrown");
> } catch (IndexOutOfBoundsException e) {
> // got it!
> }
Yes, it does make sense
-
PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v4]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk Yi Yang has updated the pull request incrementally with one additional commit since the last revision: remove extra newline - Changes: - all: https://git.openjdk.java.net/jdk/pull/3615/files - new: https://git.openjdk.java.net/jdk/pull/3615/files/db8b6ef4..bbdf4b9e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3615=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3615=02-03 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v3]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: cds, compiler and jdk Yi Yang has updated the pull request incrementally with one additional commit since the last revision: remove InlineNIOCheckIndex flag - Changes: - all: https://git.openjdk.java.net/jdk/pull/3615/files - new: https://git.openjdk.java.net/jdk/pull/3615/files/7f30dc48..db8b6ef4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3615=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3615=01-02 Stats: 23 lines in 3 files changed: 11 ins; 11 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v2]
On Wed, 28 Apr 2021 17:32:18 GMT, Igor Veresov wrote: > Do we need to keep this flag? Exactly, the flag should be removed. Thanks, Yang - PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v2]
On Fri, 23 Apr 2021 03:50:54 GMT, Yi Yang wrote: >> The JDK codebase re-created many variants of checkIndex(`grep -I -r >> 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which >> annotated with @IntrinsicCandidate and it only has a corresponding C1 >> intrinsic version. >> >> In fact, there is an utility method >> `jdk.internal.util.Preconditions.checkIndex`(wrapped by >> java.lang.Objects.checkIndex) that behaves the same as these variants of >> checkIndex, we can replace these re-created variants of checkIndex by >> Objects.checkIndex, it would significantly reduce duplicated code and enjoys >> performance improvement because Preconditions.checkIndex is >> @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. >> >> But, the problem is currently HotSpot only implements the C2 version of >> Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we >> can firstly implement its C1 counterpart. There are also a few kinds of >> stuff we can do later: >> >> 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK >> codebase. >> 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag >> >> Testing: cds, compiler and jdk > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > remove java_nio_Buffer in javaClasses.hpp Can I have a review of this PR? Thanks! - PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v2]
> The JDK codebase re-created many variants of checkIndex(`grep -I -r > 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which > annotated with @IntrinsicCandidate and it only has a corresponding C1 > intrinsic version. > > In fact, there is an utility method > `jdk.internal.util.Preconditions.checkIndex`(wrapped by > java.lang.Objects.checkIndex) that behaves the same as these variants of > checkIndex, we can replace these re-created variants of checkIndex by > Objects.checkIndex, it would significantly reduce duplicated code and enjoys > performance improvement because Preconditions.checkIndex is > @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. > > But, the problem is currently HotSpot only implements the C2 version of > Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we > can firstly implement its C1 counterpart. There are also a few kinds of stuff > we can do later: > > 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK > codebase. > 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag > > Testing: compiler and jdk Yi Yang has updated the pull request incrementally with one additional commit since the last revision: remove java_nio_Buffer in javaClasses.hpp - Changes: - all: https://git.openjdk.java.net/jdk/pull/3615/files - new: https://git.openjdk.java.net/jdk/pull/3615/files/a6affc54..7f30dc48 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3615=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3615=00-01 Stats: 27 lines in 2 files changed: 0 ins; 27 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615 PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8265039: Adjust javadoc for ByteArray*Stream and InputStream
On Mon, 5 Apr 2021 08:37:15 GMT, Сергей Цыпанов
wrote:
> Hello,
>
> to avoid cases detected in
> [https://github.com/openjdk/jdk/pull/2992](https://github.com/openjdk/jdk/pull/2992)
> I propose to modify JavaDoc of `ByteArray*Stream` to explicitly mention
> redundancy of wrapping with `BufferedInputStream`.
>
> As of `InputStream` I think in notation `It is never correct to use the
> return value of this method to allocate a buffer intended to hold all data in
> this stream.` the word 'never' should be replaced with 'usually': apart from
> `ByteArrayInputStream` e.g. `FileInputStream.available()` often returns the
> count of remaining bytes (the only exclusion I'm aware of is the files
> located under `/proc/`) and indeed this count can be used to allocate a
> buffer to read all the bytes in one call.
>
> Consider benchmark
>
>
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class ByteArrayInputStreamBenchmark {
>
> @Benchmark
> public void read(Data data, Blackhole bh) {
> int value;
> var in = data.bais;
> while ((value = in.read()) != -1) {
> bh.consume(value);
> }
> }
>
> @Benchmark
> public void readBuffered(Data data, Blackhole bh) throws IOException {
> int value;
> var in = new BufferedInputStream(data.bais);
> while ((value = in.read()) != -1) {
> bh.consume(value);
> }
> }
>
> @Benchmark
> public Object readAllBytes(Data data) {
> var in = data.bais;
> return in.readAllBytes();
> }
>
> @Benchmark
> public Object readAllBytesBuffered(Data data) throws IOException {
> var in = data.bais;
> return new BufferedInputStream(in).readAllBytes();
> }
>
> @State(Scope.Thread)
> public static class Data {
>
> @Param({"8", "128", "512", "1024"})
> private int length;
>
> private byte[] bytes;
> private ByteArrayInputStream bais;
>
> @Setup(Level.Iteration)
> public void setUp() {
> bytes = new byte[length];
> ThreadLocalRandom.current().nextBytes(bytes);
> }
>
> @Setup(Level.Invocation)
> public void setUpBais() {
> bais = new ByteArrayInputStream(bytes);
> }
> }
> }
>
>
> giving
>
>
> (length) Score Error
> Units
> read8 55.737 ± 0.431
> ns/op
> read 128 533.172 ± 1.613
> ns/op
> read 5122066.238 ± 23.989
> ns/op
> read 10244335.570 ± 20.137
> ns/op
>
> readBuffered8 521.936 ± 2.454
> ns/op
> readBuffered 128 971.617 ± 100.469
> ns/op
> readBuffered 5122284.472 ± 251.390
> ns/op
> readBuffered 10244168.598 ± 77.980
> ns/op
>
> readAllBytes8 34.850 ± 0.072
> ns/op
> readAllBytes 128 36.751 ± 0.133
> ns/op
> readAllBytes 512 45.304 ± 0.699
> ns/op
> readAllBytes 1024 61.790 ± 0.386
> ns/op
>
> readAllBytesBuffered8 870.454 ± 4.406
> ns/op
> readAllBytesBuffered 128 910.176 ± 32.258
> ns/op
> readAllBytesBuffered 512 896.155 ± 6.005
> ns/op
> readAllBytesBuffered 1024 965.596 ± 29.225
> ns/op
>
> read:·gc.alloc.rate.norm8 32.006 ± 0.001
> B/op
> read:·gc.alloc.rate.norm 128 32.007 ± 0.004
> B/op
> read:·gc.alloc.rate.norm 512 32.010 ± 0.010
> B/op
> read:·gc.alloc.rate.norm 1024 32.011 ± 0.008
> B/op
>
> readBuffered:·gc.alloc.rate.norm88280.354 ± 0.016
> B/op
> readBuffered:·gc.alloc.rate.norm 1288240.484 ± 0.015
> B/op
> readBuffered:·gc.alloc.rate.norm 5128240.599 ± 0.056
> B/op
> readBuffered:·gc.alloc.rate.norm 10248280.978 ± 0.024
> B/op
>
> readAllBytes:·gc.alloc.rate.norm8 56.008 ± 0.001
> B/op
> readAllBytes:·gc.alloc.rate.norm 128 176.017 ± 0.001
> B/op
> readAllBytes:·gc.alloc.rate.norm 512 560.035 ± 0.002
> B/op
> readAllBytes:·gc.alloc.rate.norm 10241072.057 ± 0.002
> B/op
>
> readAllBytesBuffered:·gc.alloc.rate.norm8 16512.660 ± 0.026
> B/op
> readAllBytesBuffered:·gc.alloc.rate.norm 128 16632.684 ± 0.008
> B/op
> readAllBytesBuffered:·gc.alloc.rate.norm 512 17016.694 ± 0.017
> B/op
>
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов wrote: > The usage of `LinkedList` is senseless and can be replaced with either > `ArrayList` or `ArrayDeque` which are both more compact and effective. > > jdk:tier1 and jdk:tier2 are both ok src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 220: > 218: return Collections.emptyList(); > 219: } > 220: List result = new ArrayList<>(); We'd better be cautious about this replacement since its [caller](https://github.com/openjdk/jdk/blob/73029fe10a8a814a8c5f5221f2e667fd14a5b379/src/java.base/share/classes/java/net/URLClassLoader.java#L363) will remove the first element of this array, that's one of the scenarios where LinkedList usually has better performance than ArrayList. Just IMHO, I suggest replacing them only if there is a performance improvement(e.g. benchmark reports). Changing field types will break users' existing application code, they might reflectively modify these values. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263552: Use String.valueOf() for char-to-String conversion in ObjectStreamClass
On Sat, 13 Mar 2021 11:35:48 GMT, Сергей Цыпанов wrote: >> Nice cleanup. I can help file a JBS issue if @c-cleary doesn't notice your >> comment. > > @kelthuzadx hi! I'd appreciate this, as there's no JBS issue for this ( Hi @stsypanov, I've created it https://bugs.openjdk.java.net/browse/JDK-8263552. Good luck :-) - PR: https://git.openjdk.java.net/jdk/pull/2660
Re: RFR: 8263552: Use String.valueOf() for char-to-String conversion in ObjectStreamClass
On Mon, 22 Feb 2021 12:04:14 GMT, Conor Cleary wrote: >> This is a very simple and trivial improvement about getting rid of pointless >> char wrapping into array > > src/java.base/share/classes/java/io/ObjectStreamClass.java line 833: > >> 831: String fname = in.readUTF(); >> 832: String signature = ((tcode == 'L') || (tcode == '[')) ? >> 833: in.readTypeString() : String.valueOf(tcode); > > Certainly more readable and it seems that the call to valueOf is equivalent > to whay takes place with the original code. I can't see any difference > semantically or performance-wise at a glance. LGTM Nice cleanup. I can help file a JBS issue if @c-cleary doesn't notice your comment. - PR: https://git.openjdk.java.net/jdk/pull/2660
