Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root [v2]
On Mon, 17 Jun 2024 18:46:34 GMT, Naoto Sato wrote:
>> SendaoYan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> change the excption meassges to: Unable to create an unreadable properties
>> file
>
> test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java
> line 60:
>
>> 58: public static void main(String[] args) throws Throwable {
>> 59: if(Platform.isRoot() && !Platform.isWindows()) {
>> 60: throw new SkippedException("root user has privileged will
>> make this test fail.");
>
> The exception message can be improved. How about "Unable to create an
> unreadable properties file"?
Thanks for the suggestion. The exception message has been change.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19732#discussion_r1643627352
Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root [v2]
> Hi all, > Testcase > `test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java` > run fails with root user privileged. I think it's necessary to skip this > testcase when user is root. > Why run the jtreg test by root user? It's because during rpmbuild process for > linux distribution of JDK, root user is the default user to build the > openjdk, also is the default user to run the `make test-tier1`, this PR make > this testcase more robustness. > The change has been verified, only change the testcase, no risk. SendaoYan has updated the pull request incrementally with one additional commit since the last revision: change the excption meassges to: Unable to create an unreadable properties file - Changes: - all: https://git.openjdk.org/jdk/pull/19732/files - new: https://git.openjdk.org/jdk/pull/19732/files/5cf26a11..9b8a0bcb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19732=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19732=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19732.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19732/head:pull/19732 PR: https://git.openjdk.org/jdk/pull/19732
Re: RFR: 8334342: Add MergeStore JMH benchmarks [v5]
On Mon, 17 Jun 2024 23:02:47 GMT, Shaojin Wen wrote: >> [8318446](https://github.com/openjdk/jdk/pull/16245) brings MergeStore. We >> need a JMH Benchmark to evaluate the performance of various batch operations >> and the effect of MergeStore. > > Shaojin Wen has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 13 additional > commits since the last revision: > > - Merge remote-tracking branch 'upstream/master' into merge_store_bench > - bug fix for `putChars4C` > - bug fix for `putChars4C` and `putChars4S` > - use VarHandler CHAR_L & CHAR_B > - copyright > - bug fix for putIntBU > - add cases for `getChar` & `putChar` > - code format > - add `setIntRL` & `setIntRLU` > - add comments > - ... and 3 more: https://git.openjdk.org/jdk/compare/d4942ac0...4c9b9418 I re-ran the performance test based on WebRevs 04: [Full](https://webrevs.openjdk.org/?repo=jdk=19734=04) - [Incremental](https://webrevs.openjdk.org/?repo=jdk=19734=03-04) ([4c9b9418](https://git.openjdk.org/jdk/pull/19734/files/4c9b9418fc4a95504867b6019b3e94605917f747)) . # 1. Cases MergeStore does not work @eme64 I found `putChars4BV` and `putChars4LV` to be two cases where MergeStore didn't work, if support can be enhanced, it would be useful for people using VarHandle. putChars4BV putChars4LV I also found that the performance of the case using VarHandle is particularly good. Why? For example: setIntBV setIntLV setLongBV setLongLV # 2. Performance numbers The names of these cases have the following B/L/V/U suffixes, which are: B BigEndian L LittleEndian V VarHandle U Unsafe R reverseBytes C Unsafe.getChar & putChar S Unsafe.getShort & putShort ## 2.1 MacBook M1 Pro (aarch64) BenchmarkMode Cnt ScoreError Units MergeStoreBench.getCharB avgt 15 5340.200 ? 7.038 ns/op MergeStoreBench.getCharBUavgt 15 5482.163 ? 7.922 ns/op MergeStoreBench.getCharBVavgt 15 5074.165 ? 6.759 ns/op MergeStoreBench.getCharC avgt 15 5051.763 ? 6.552 ns/op MergeStoreBench.getCharL avgt 15 5374.464 ? 9.783 ns/op MergeStoreBench.getCharLUavgt 15 5487.532 ? 6.368 ns/op MergeStoreBench.getCharLVavgt 15 5071.263 ? 9.717 ns/op MergeStoreBench.getIntB avgt 15 6277.984 ? 6.284 ns/op MergeStoreBench.getIntBU avgt 15 5232.984 ? 10.384 ns/op MergeStoreBench.getIntBV avgt 15 1206.264 ? 1.193 ns/op MergeStoreBench.getIntL avgt 15 6172.779 ? 1.962 ns/op MergeStoreBench.getIntLU avgt 15 5157.317 ? 16.077 ns/op MergeStoreBench.getIntLV avgt 15 2558.110 ? 3.402 ns/op MergeStoreBench.getIntRB avgt 15 6889.916 ? 36.955 ns/op MergeStoreBench.getIntRBUavgt 15 5769.950 ? 11.499 ns/op MergeStoreBench.getIntRL avgt 15 6625.605 ? 10.662 ns/op MergeStoreBench.getIntRLUavgt 15 5746.742 ? 11.945 ns/op MergeStoreBench.getIntRU avgt 15 2544.586 ? 2.769 ns/op MergeStoreBench.getIntU avgt 15 2541.119 ? 3.252 ns/op MergeStoreBench.getLongB avgt 15 12098.129 ? 31.451 ns/op MergeStoreBench.getLongBUavgt 15 9760.621 ? 16.427 ns/op MergeStoreBench.getLongBVavgt 15 2593.635 ? 4.698 ns/op MergeStoreBench.getLongL avgt 15 12031.065 ? 19.820 ns/op MergeStoreBench.getLongLUavgt 15 9653.938 ? 18.372 ns/op MergeStoreBench.getLongLVavgt 15 2557.521 ? 3.338 ns/op MergeStoreBench.getLongRBavgt 15 12092.061 ? 18.026 ns/op MergeStoreBench.getLongRBU avgt 15 9763.489 ? 17.347 ns/op MergeStoreBench.getLongRLavgt 15 12027.686 ? 17.472 ns/op MergeStoreBench.getLongRLU avgt 15 9649.433 ? 8.384 ns/op MergeStoreBench.getLongRUavgt 15 2546.239 ? 2.088 ns/op MergeStoreBench.getLongU avgt 15 2539.762 ? 1.439 ns/op MergeStoreBench.putChars4B avgt 15 8487.381 ? 23.170 ns/op MergeStoreBench.putChars4BU avgt 15 3830.198 ? 7.083 ns/op MergeStoreBench.putChars4BV avgt 15 5154.819 ? 10.348 ns/op MergeStoreBench.putChars4C avgt 15 5162.766 ? 15.041 ns/op MergeStoreBench.putChars4L avgt 15 8381.231 ? 20.135 ns/op MergeStoreBench.putChars4LU avgt 15 3827.784 ? 3.163 ns/op MergeStoreBench.putChars4LV avgt 15 5151.508 ? 4.907 ns/op MergeStoreBench.putChars4S avgt 15 5152.123 ? 7.407 ns/op MergeStoreBench.setCharBSavgt 15 5317.319 ? 28.445 ns/op MergeStoreBench.setCharBVavgt 15 5175.400 ? 7.110 ns/op MergeStoreBench.setCharC avgt 15 5085.752 ? 6.222 ns/op MergeStoreBench.setCharLSavgt 15 5294.766 ? 9.742 ns/op MergeStoreBench.setCharLVavgt 15 5108.269 ? 6.692 ns/op MergeStoreBench.setIntB avgt 15 5095.236 ? 2.838 ns/op MergeStoreBench.setIntBU avgt 15 5097.007 ? 4.249 ns/op MergeStoreBench.setIntBV avgt 15 1224.506 ?
Re: RFR: 8334342: Add MergeStore JMH benchmarks [v5]
> [8318446](https://github.com/openjdk/jdk/pull/16245) brings MergeStore. We > need a JMH Benchmark to evaluate the performance of various batch operations > and the effect of MergeStore. Shaojin Wen has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into merge_store_bench - bug fix for `putChars4C` - bug fix for `putChars4C` and `putChars4S` - use VarHandler CHAR_L & CHAR_B - copyright - bug fix for putIntBU - add cases for `getChar` & `putChar` - code format - add `setIntRL` & `setIntRLU` - add comments - ... and 3 more: https://git.openjdk.org/jdk/compare/fc9315ff...4c9b9418 - Changes: - all: https://git.openjdk.org/jdk/pull/19734/files - new: https://git.openjdk.org/jdk/pull/19734/files/1140bd81..4c9b9418 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19734=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19734=03-04 Stats: 1440 lines in 77 files changed: 795 ins; 459 del; 186 mod Patch: https://git.openjdk.org/jdk/pull/19734.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19734/head:pull/19734 PR: https://git.openjdk.org/jdk/pull/19734
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v17]
On Mon, 17 Jun 2024 19:56:38 GMT, Sean Mullan wrote: > AFAICT, the only test modified in this PR that actually enables a Security > Manager is test/jdk/javax/management/security/AuthorizationTest.java. Is that > test sufficient to exercise the code changes in this PR when an SM is enabled? While this code change has done its best to avoid any behavior change when SM is allowed, I agree with Sean to run tests in both modes. When these tests were originally introduced they were meant to test the old implementation. Keep testing eith both implementations help us to avoid any future (might not be long) regression. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2174568617
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v6]
On Mon, 17 Jun 2024 20:10:18 GMT, Chen Liang wrote:
>> For StringBuf proxy, is it acceptible for us to introduce a new jdk.internal
>> public interface (accessible only within java.base module) to expose common
>> public methods in AbstractStringBuilder? We have public types extending or
>> implementing non-public-types in the JDK (AbstractStringBuilder,
>> NamedPackage) so I guess having a new module-specific superinterface would
>> be fine? Need verification from API experts.
>
>> Hi @liach Do you know any other places within java.base where we would need
>> the same proxy for StringBuffer?
>
> Good question! I looked at
> https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/class-use/StringBuffer.html.
> I think such a new interface indeed is of limited usefulness, as we don't
> really have that many non-java.lang APIs closely tied to StringBuffer.
> Matcher is like one, but it lives mostly fine without the shadowing because
> it is using `Appendable`. And this has enlightened me.
>
> In fact, we can use `Appendable` too, as we just need 2 `append` from
> `Appendable` and `subSequence` (replacing `substring`) and `length` from
> `CharSequence`. We can declare method like:
>
> T format(double number, T toAppendTo,
> FieldPosition status) {
>
> This signature accepts both `StringBuilder` and `StringBuffer`; all use sites
> can be according updated. The only thing need to change is that `substring`
> should now become `subSequence`, but it's just used in
> `CharacterIteratorFieldDelegate` so the impact is small.
> > Hi @liach Do you know any other places within java.base where we would need
> > the same proxy for StringBuffer?
>
> Good question! I looked at
> https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/class-use/StringBuffer.html.
> I think such a new interface indeed is of limited usefulness, as we don't
> really have that many non-java.lang APIs closely tied to StringBuffer.
> Matcher is like one, but it lives mostly fine without the shadowing because
> it is using `Appendable`. And this has enlightened me.
>
> In fact, we can use `Appendable` too, as we just need 2 `append` from
> `Appendable` and `subSequence` (replacing `substring`) and `length` from
> `CharSequence`. We can declare method like:
>
> ```java
> T format(double number, T toAppendTo,
> FieldPosition status) {
> ```
>
> This signature accepts both `StringBuilder` and `StringBuffer`; all use sites
> can be according updated. The only thing need to change is that `substring`
> should now become `subSequence`, but it's just used in
> `CharacterIteratorFieldDelegate` so the impact is small.
That looks promising! Much cleaner than having dual methods
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174509435
RFR: 8334418: Update IANA Language Subtag Registry to Version 2024-06-14
Please review this PR, which is a trivial update of the IANA subtag registry to version 2024-06-14. Announcement: https://mm.icann.org/pipermail/ietf-languages-announcements/2024-June/92.html - Commit messages: - init Changes: https://git.openjdk.org/jdk/pull/19757/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19757=00 Issue: https://bugs.openjdk.org/browse/JDK-8334418 Stats: 7 lines in 2 files changed: 4 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19757.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19757/head:pull/19757 PR: https://git.openjdk.org/jdk/pull/19757
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v18]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: braces - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/67aca736..384a3a19 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19624=17 - incr: https://webrevs.openjdk.org/?repo=jdk=19624=16-17 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Mon, 17 Jun 2024 15:53:19 GMT, Kevin Walls wrote: >> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java >> line 1314: >> >>> 1312: return AccessController.doPrivileged(action, acc); >>> 1313: } >>> 1314: } >> >> This piece of code is repeated at several places in similar but different >> forms - sometime we check for >> `SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime for `! >> SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime we check >> for `acc == null`, sometime for `acc != null` etc.. >> I wonder if we could abstract that to some utility method somewhere. Would >> that make sense? Maybe that's not possible due to using sometime >> `PrivelegedAction` and sometimes `PrivilegedExceptionAction` - but maybe we >> could use `PrivilegedExceptionAction` everywhere at the cost of handling >> `PrivilegedActionException`? >> If that doesn't prove useful (if handling `PrivilegedExceptionAction` adds >> again an equivalent amount of clutter) then maybe we could at least make >> sure we do the same checks in the same way (typically `acc == null` vs `acc >> != null`)? > > Hi, > We almost always check > !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the > RMIConnectionImpl contstructor) > > This keeps the "modern" case first (except in that constructor, where it is > only one line for each side of the if...). > > This extra checking of > SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the > modern and old style cases distinct, based on other comments here. It keeps > it simple to read I hope, but yes it is a little more verbose than it could > be. > > I'm hoping you'll agree that as these checks will be removed anyway, probably > with a release, we should delay more extensive cleanup until then. (I've > also rolled back my noPermissionsACC removal to keep this change away from > being a general cleanup.) > > I had a bunch of additional Exception handling for e.g. > PrivilegedActionException I think in here, and it wasn't very pretty. But, > it might look better when there are fewer nested ifs in these methods, and > when we are properly in the post-SecurityManager world... 8-) > > Whether it checks acc != null or acc == null is based on the existing code. > I think that makes this diff easier to read, but also could be reworked and > be made more consistent after SM removal. Agree with Kevin. To minimize risk, especially since this is to fix a significant regression and we are in RDP1, we are really trying to preserve the existing code as much as possible, even though it could be improved. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1643395182
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v17]
On Mon, 17 Jun 2024 15:59:37 GMT, Kevin Walls wrote: >> JMX uses APIs related to the Security Mananger which are deprecated. Use of >> AccessControlContext will be removed when Security Manager is removed. >> >> Until then, updates are needed to not require setting >> -Djava.security.manager=allow to use JMX authentication. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Remove import src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java line 1306: > 1304: return action.run(); > 1305: else > 1306: return Subject.doAs(subject, action); Code style comment: put braces around the if/else to be consistent with lines below. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1643390138
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v6]
On Mon, 17 Jun 2024 18:47:11 GMT, Chen Liang wrote:
>> lingjun-cg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 896: Performance regression of DecimalFormat.format
>
> For StringBuf proxy, is it acceptible for us to introduce a new jdk.internal
> public interface (accessible only within java.base module) to expose common
> public methods in AbstractStringBuilder? We have public types extending or
> implementing non-public-types in the JDK (AbstractStringBuilder,
> NamedPackage) so I guess having a new module-specific superinterface would be
> fine? Need verification from API experts.
> Hi @liach Do you know any other places within java.base where we would need
> the same proxy for StringBuffer?
Good question! I looked at
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/class-use/StringBuffer.html.
I think such a new interface indeed is of limited usefulness, as we don't
really have that many non-java.lang APIs closely tied to StringBuffer. Matcher
is like one, but it lives mostly fine without the shadowing because it is using
`Appendable`. And this has enlightened me.
In fact, we can use `Appendable` too, as we just need 2 `append` from
`Appendable` and `subSequence` (replacing `substring`) and `length` from
`CharSequence`. We can declare method like:
T format(double number, T toAppendTo,
FieldPosition status) {
This signature accepts both `StringBuilder` and `StringBuffer`; all use sites
can be according updated. The only thing need to change is that `substring`
should now become `subSequence`, but it's just used in
`CharacterIteratorFieldDelegate` so the impact is small.
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174336411
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v17]
On Mon, 17 Jun 2024 15:59:37 GMT, Kevin Walls wrote: >> JMX uses APIs related to the Security Mananger which are deprecated. Use of >> AccessControlContext will be removed when Security Manager is removed. >> >> Until then, updates are needed to not require setting >> -Djava.security.manager=allow to use JMX authentication. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Remove import AFAICT, the only test modified in this PR that actually enables a Security Manager is test/jdk/javax/management/security/AuthorizationTest.java. Is that test sufficient to exercise the code changes in this PR when an SM is enabled? - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2174313063
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v6]
On Mon, 17 Jun 2024 18:47:11 GMT, Chen Liang wrote: >> lingjun-cg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 896: Performance regression of DecimalFormat.format > > For StringBuf proxy, is it acceptible for us to introduce a new jdk.internal > public interface (accessible only within java.base module) to expose common > public methods in AbstractStringBuilder? We have public types extending or > implementing non-public-types in the JDK (AbstractStringBuilder, > NamedPackage) so I guess having a new module-specific superinterface would be > fine? Need verification from API experts. Hi @liach Do you know any other places within java.base where we would need the same proxy for StringBuffer? - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174283589
Re: RFR: 8334333: MissingResourceCauseTestRun.java fails if run by root
On Sat, 15 Jun 2024 09:56:53 GMT, SendaoYan wrote:
> Hi all,
> Testcase
> `test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java`
> run fails with root user privileged. I think it's necessary to skip this
> testcase when user is root.
> Why run the jtreg test by root user? It's because during rpmbuild process for
> linux distribution of JDK, root user is the default user to build the
> openjdk, also is the default user to run the `make test-tier1`, this PR make
> this testcase more robustness.
> The change has been verified, only change the testcase, no risk.
test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java line
60:
> 58: public static void main(String[] args) throws Throwable {
> 59: if(Platform.isRoot() && !Platform.isWindows()) {
> 60: throw new SkippedException("root user has privileged will
> make this test fail.");
The exception message can be improved. How about "Unable to create an
unreadable properties file"?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19732#discussion_r1643295899
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v6]
On Fri, 14 Jun 2024 03:19:48 GMT, lingjun-cg wrote:
>> ### Performance regression of DecimalFormat.format
>> From the output of perf, we can see the hottest regions contain atomic
>> instructions. But when run with JDK 11, there is no such problem. The
>> reason is the removed biased locking.
>> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
>> contains many synchronized methods.
>> So I added support for some new methods that accept StringBuilder which is
>> lock-free.
>>
>> ### Benchmark testcase
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class JmhDecimalFormat {
>>
>> private DecimalFormat format;
>>
>> @Setup(Level.Trial)
>> public void setup() {
>> format = new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>>
>> ### Test result
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>>
>>
>>
>> Current JDK after optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>>
>>
>> ### JDK 11
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
> lingjun-cg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 896: Performance regression of DecimalFormat.format
For StringBuf proxy, is it acceptible for us to introduce a new jdk.internal
public interface (accessible only within java.base module) to expose common
public methods in AbstractStringBuilder? We have public types extending or
implementing non-public-types in the JDK (AbstractStringBuilder, NamedPackage)
so I guess having a new module-specific superinterface would be fine? Need
verification from API experts.
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174193760
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Fri, 14 Jun 2024 03:28:58 GMT, lingjun-cg wrote: >> I second Justin's suggestion here. The change should benefit every >> implementation in the JDK, not only NumberFormat. Also, I might suggest >> renaming the new class, as it is kind of long/redundant. How about using >> something as simple as "StringBuf"? > >> I second Justin's suggestion here. The change should benefit every >> implementation in the JDK, not only NumberFormat. Also, I might suggest >> renaming the new class, as it is kind of long/redundant. How about using >> something as simple as "StringBuf"? > > Thanks for your comment. The long name bother me for a while. I have changed > it to ""StringBuf". > Hi @lingjun-cg > > Thanks for your work here. > > While this would benefit the performance of NumberFormat subclasses, we are > hoping that if we are going to make the internal switch to StringBuilder, we > can also make similar changes to other Format subclasses as well where > applicable. So, we would want `isInternalSubclass()` to be defined at the > Format level. Some methods that I can immediately think of would be `String > DateFormat.format(Date date)`, `String ListFormat.format(List input)` > and, `String MessageFormat.format(Object obj)`. > > I believe @naotoj shares the same sentiment. Yes, that is what I would expect in this change. - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174184666
Re: RFR: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS [v4]
On Mon, 17 Jun 2024 17:38:56 GMT, Chen Liang wrote: >> Currently, javap crashes for class files that have set non-zero values for >> undefined access flag bits, as >> `java.lang.reflect.AccessFlag.maskToAccessFlag` and >> `java.lang.classfile.AccessFlags.flags` fail. In contrast, the JVMS, though >> asking for these bits to be set to 0, requires VM to proceed and ignore >> these bits. javap should emulate the VM behavior and proceed rendering, >> ignoring these undefined bits. >> >> In addition, a few bytecode generation utilities in the JDK set unused bits, >> such as in >> `java.lang.invoke.MethodHandleImpl.BindCaller#generateInvokerTemplate` and >> `java.lang.invoke.GenerateJLIClassesHelper#generateCodeBytesForLFs`. Those >> can be updated in a later cleanup. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Improved error message I have created a CSR for this patch at https://bugs.openjdk.org/browse/JDK-8334406; please review. Also I am not sure about the correct interface kind and scope for this CSR: is "other" and "implementation" correct? - PR Comment: https://git.openjdk.org/jdk/pull/19708#issuecomment-2174128620
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Fri, 14 Jun 2024 03:28:58 GMT, lingjun-cg wrote: >> I second Justin's suggestion here. The change should benefit every >> implementation in the JDK, not only NumberFormat. Also, I might suggest >> renaming the new class, as it is kind of long/redundant. How about using >> something as simple as "StringBuf"? > >> I second Justin's suggestion here. The change should benefit every >> implementation in the JDK, not only NumberFormat. Also, I might suggest >> renaming the new class, as it is kind of long/redundant. How about using >> something as simple as "StringBuf"? > > Thanks for your comment. The long name bother me for a while. I have changed > it to ""StringBuf". Hi @lingjun-cg Thanks for your work here. While this would benefit the performance of NumberFormat subclasses, we are hoping that if we are going to make the internal switch to StringBuilder, we can also make similar changes to other Format subclasses as well where applicable. So, we would want `isInternalSubclass()` to be defined at the Format level. Some methods that I can immediately think of would be `String DateFormat.format(Date date)`, `String ListFormat.format(List input)` and, `String MessageFormat.format(Object obj)`. I believe @naotoj shares the same sentiment. - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174040047
Re: RFR: 8334394: Race condition in Class::protectionDomain [v2]
On Mon, 17 Jun 2024 15:24:27 GMT, Weijun Wang wrote: >> Make sure `pd` is always the same object when `getProtectionDomain0` is null. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > explain why the test is related to the fix The update to Class looks okay. At some point I think we see if the protection domain field can be changed to be an explicit field (not injected) so that the native method and JVM_GetProtectionDomain can be removed. - PR Comment: https://git.openjdk.org/jdk/pull/19752#issuecomment-2173980108
Re: RFR: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS [v4]
On Mon, 17 Jun 2024 17:38:56 GMT, Chen Liang wrote: >> Currently, javap crashes for class files that have set non-zero values for >> undefined access flag bits, as >> `java.lang.reflect.AccessFlag.maskToAccessFlag` and >> `java.lang.classfile.AccessFlags.flags` fail. In contrast, the JVMS, though >> asking for these bits to be set to 0, requires VM to proceed and ignore >> these bits. javap should emulate the VM behavior and proceed rendering, >> ignoring these undefined bits. >> >> In addition, a few bytecode generation utilities in the JDK set unused bits, >> such as in >> `java.lang.invoke.MethodHandleImpl.BindCaller#generateInvokerTemplate` and >> `java.lang.invoke.GenerateJLIClassesHelper#generateCodeBytesForLFs`. Those >> can be updated in a later cleanup. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Improved error message Now it looks good to me, thanks. - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19708#pullrequestreview-2123504238
Re: RFR: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS [v4]
> Currently, javap crashes for class files that have set non-zero values for > undefined access flag bits, as > `java.lang.reflect.AccessFlag.maskToAccessFlag` and > `java.lang.classfile.AccessFlags.flags` fail. In contrast, the JVMS, though > asking for these bits to be set to 0, requires VM to proceed and ignore these > bits. javap should emulate the VM behavior and proceed rendering, ignoring > these undefined bits. > > In addition, a few bytecode generation utilities in the JDK set unused bits, > such as in > `java.lang.invoke.MethodHandleImpl.BindCaller#generateInvokerTemplate` and > `java.lang.invoke.GenerateJLIClassesHelper#generateCodeBytesForLFs`. Those > can be updated in a later cleanup. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Improved error message - Changes: - all: https://git.openjdk.org/jdk/pull/19708/files - new: https://git.openjdk.org/jdk/pull/19708/files/0f3e1a97..c8eee028 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19708=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19708=02-03 Stats: 20 lines in 2 files changed: 16 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19708.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19708/head:pull/19708 PR: https://git.openjdk.org/jdk/pull/19708
Re: RFR: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS [v2]
On Mon, 17 Jun 2024 13:55:50 GMT, Chen Liang wrote: >> `javap` should never silently ignore erroneous class file content. >> If the flag value violates JVMS - it should be reported as an error and >> `javap` should reflect that in the return value. >> On the other hand `javap` should handle such situations, provide relevant >> error message and continue with the report. >> It is similar case as `javap` detection of inappropriate CP entry types. > > @asotona Thanks for the note, I have updated the code so javap now behaves > like your suggestion. @liach , please file a CSR for this issue to discuss the behavioral changes. - PR Comment: https://git.openjdk.org/jdk/pull/19708#issuecomment-2173914440
Re: RFR: 8334342: Add MergeStore JMH benchmarks [v4]
On Mon, 17 Jun 2024 08:07:40 GMT, Shaojin Wen wrote: >> test/micro/org/openjdk/bench/java/lang/MergeStoreBench.java line 656: >> >>> 654: array[offset + 2] = (byte) (value >> 8); >>> 655: array[offset + 3] = (byte) (value ); >>> 656: } >> >> You say that here `MergeStore` does not work. That is because the indices >> are increasing, but the shifts decreasing. So that does not work on >> little-endian machines (most architectures), but I would expect it to work >> on big-endian machines with https://github.com/openjdk/jdk/pull/19218. > > Big endian is often used in network data transmission scenarios, and it is > common to process big endian data on a little endian machine. In this case, > can it be optimized to Integer.reverseBytes & putIntLittleEndian on a > LittleEndian machine? `setIntB -> setIntRL` I had already suggested that here: https://github.com/openjdk/jdk/pull/19218#pullrequestreview-2076196539 Feel free to file an RFE. Maybe someone wants to work on it. I think it would not be that hard to make it work given all the code that is already there now. And it would be helpful in for both big/little endian to be able to do both orders. - PR Review Comment: https://git.openjdk.org/jdk/pull/19734#discussion_r1643088147
Re: RFR: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS [v3]
On Mon, 17 Jun 2024 15:56:56 GMT, Adam Sotona wrote:
>> Chen Liang has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Improve tests to check unmatched bit position and failure for
>> non-inner-classes
>> - Report error for flag problems
>
> src/jdk.jdeps/share/classes/com/sun/tools/javap/BasicWriter.java line 84:
>
>> 82: } catch (IllegalArgumentException ex) {
>> 83: mask &= LOCATION_MASKS.get(location);
>> 84: report(ex);
>
> Unfortunately the original exception message is missing any info that it is
> related to access flags and it is not very clear what "Error: Unmatched bit
> position 0x2 for location CLASS" means.
> I would add at least a message prefix, for example "Error: Access Flags:
> Unmatched bit position 0x2 for location CLASS".
Agreed, even though this message is already printed right by the access
modifiers.
> test/langtools/tools/javap/UndefinedAccessFlagTest.java line 105:
>
>> 103: .writeAll()
>> 104: .getOutputLines(Task.OutputKind.DIRECT);
>> 105: assertTrue(lines.stream().anyMatch(l -> l.contains("Unmatched
>> bit position")), () -> String.join("\n", lines));
>
> I would add a check the "fatal" error does not occur or any other way to
> confirm correct recovery.
Great suggestion! I will filter the lines starting with `Error:` and assert
they `allMatch(st -> st.contains("Access Flags:"))`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19708#discussion_r1643072918
PR Review Comment: https://git.openjdk.org/jdk/pull/19708#discussion_r1643073763
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v17]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Remove import - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/597264b9..67aca736 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19624=16 - incr: https://webrevs.openjdk.org/?repo=jdk=19624=15-16 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Mon, 17 Jun 2024 13:58:11 GMT, Daniel Fuchs wrote: >> Kevin Walls has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - style update >> - whitespace > > src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java > line 1314: > >> 1312: return AccessController.doPrivileged(action, acc); >> 1313: } >> 1314: } > > This piece of code is repeated at several places in similar but different > forms - sometime we check for > `SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime for `! > SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime we check > for `acc == null`, sometime for `acc != null` etc.. > I wonder if we could abstract that to some utility method somewhere. Would > that make sense? Maybe that's not possible due to using sometime > `PrivelegedAction` and sometimes `PrivilegedExceptionAction` - but maybe we > could use `PrivilegedExceptionAction` everywhere at the cost of handling > `PrivilegedActionException`? > If that doesn't prove useful (if handling `PrivilegedExceptionAction` adds > again an equivalent amount of clutter) then maybe we could at least make sure > we do the same checks in the same way (typically `acc == null` vs `acc != > null`)? Hi, We almost always check !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the RMIConnectionImpl contstructor) This keeps the "modern" case first (except in that constructor, where it is only one line for each side of the if...). This extra checking of SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the modern and old style cases distinct, based on other comments here. It keeps it simple to read I hope, but yes it is a little more verbose than it could be. I'm hoping you'll agree that as these checks will be removed anyway, probably with a release, we should delay more extensive cleanup until then. (I've also rolled back my noPermissionsACC removal to keep this change away from being a general cleanup.) I had a bunch of additional Exception handling for e.g. PrivilegedActionException I think in here, and it wasn't very pretty. But, it might look better when there are fewer nested ifs in these methods, and when we are properly in the post-SecurityManager world... 8-) Whether it checks acc != null or acc == null is based on the existing code. I think that makes this diff easier to read, but also could be reworked and be made more consistent after SM removal. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1643036701
Re: RFR: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS [v3]
On Mon, 17 Jun 2024 13:55:41 GMT, Chen Liang wrote:
>> Currently, javap crashes for class files that have set non-zero values for
>> undefined access flag bits, as
>> `java.lang.reflect.AccessFlag.maskToAccessFlag` and
>> `java.lang.classfile.AccessFlags.flags` fail. In contrast, the JVMS, though
>> asking for these bits to be set to 0, requires VM to proceed and ignore
>> these bits. javap should emulate the VM behavior and proceed rendering,
>> ignoring these undefined bits.
>>
>> In addition, a few bytecode generation utilities in the JDK set unused bits,
>> such as in
>> `java.lang.invoke.MethodHandleImpl.BindCaller#generateInvokerTemplate` and
>> `java.lang.invoke.GenerateJLIClassesHelper#generateCodeBytesForLFs`. Those
>> can be updated in a later cleanup.
>
> Chen Liang has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Improve tests to check unmatched bit position and failure for
> non-inner-classes
> - Report error for flag problems
src/jdk.jdeps/share/classes/com/sun/tools/javap/BasicWriter.java line 84:
> 82: } catch (IllegalArgumentException ex) {
> 83: mask &= LOCATION_MASKS.get(location);
> 84: report(ex);
Unfortunately the original exception message is missing any info that it is
related to access flags and it is not very clear what "Error: Unmatched bit
position 0x2 for location CLASS" means.
I would add at least a message prefix, for example "Error: Access Flags:
Unmatched bit position 0x2 for location CLASS".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19708#discussion_r1643041633
Integrated: 8333962: Obsolete OldSize
On Tue, 11 Jun 2024 08:17:02 GMT, Albert Mingkun Yang wrote: > Obsolete OldSize and related code. An internal variable `OldSize` is kept to > capture the capacity of old-gen size. This pull request has now been integrated. Changeset: c94af6f9 Author:Albert Mingkun Yang URL: https://git.openjdk.org/jdk/commit/c94af6f943c179553d1827550847b93491d47506 Stats: 192 lines in 15 files changed: 7 ins; 168 del; 17 mod 8333962: Obsolete OldSize Reviewed-by: dholmes, zgu - PR: https://git.openjdk.org/jdk/pull/19647
Re: RFR: 8333962: Obsolete OldSize [v3]
On Mon, 17 Jun 2024 14:30:30 GMT, Albert Mingkun Yang wrote: >> Obsolete OldSize and related code. An internal variable `OldSize` is kept to >> capture the capacity of old-gen size. > > Albert Mingkun Yang has updated the pull request with a new target base due > to a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains one additional > commit since the last revision: > > obsolete-old-size Tests pass. Thanks for review. - PR Comment: https://git.openjdk.org/jdk/pull/19647#issuecomment-2173767317
Re: RFR: 8334394: Race condition in Class::protectionDomain [v2]
> Make sure `pd` is always the same object when `getProtectionDomain0` is null. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: explain why the test is related to the fix - Changes: - all: https://git.openjdk.org/jdk/pull/19752/files - new: https://git.openjdk.org/jdk/pull/19752/files/637cf0df..4ce5bc24 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19752=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19752=00-01 Stats: 25 lines in 1 file changed: 25 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19752.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19752/head:pull/19752 PR: https://git.openjdk.org/jdk/pull/19752
Re: RFR: 8334394: Race condition in Class::protectionDomain
On Mon, 17 Jun 2024 15:11:29 GMT, Weijun Wang wrote:
>> test/jdk/java/lang/Class/ProtectionDomainRace.java line 42:
>>
>>> 40: try {
>>> 41: Subject.doAs(null, ac);
>>> 42: } catch (Throwable t) {
>>
>> This can test for the racy NPE, but it cannot detect if there's distinct
>> allPermDomain objects written by races. Should we add another check to
>> defend against that?
>
> Actually it can. This bug originally breaks the `assert` statement at
> https://github.com/openjdk/jdk/blob/5cea53d372744ddf1bedaae4667415e6525ef82f/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java#L1209.
So this test might look like unrelated to the fix at first sight, but somewhere
deep inside the call stack it does. I should add a comment. Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19752#discussion_r1642978163
Re: RFR: 8334394: Race condition in Class::protectionDomain
On Mon, 17 Jun 2024 15:08:26 GMT, Chen Liang wrote:
>> Make sure `pd` is always the same object when `getProtectionDomain0` is null.
>
> test/jdk/java/lang/Class/ProtectionDomainRace.java line 42:
>
>> 40: try {
>> 41: Subject.doAs(null, ac);
>> 42: } catch (Throwable t) {
>
> This can test for the racy NPE, but it cannot detect if there's distinct
> allPermDomain objects written by races. Should we add another check to defend
> against that?
Actually it can. This bug originally breaks the `assert` statement at
https://github.com/openjdk/jdk/blob/5cea53d372744ddf1bedaae4667415e6525ef82f/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java#L1209.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19752#discussion_r1642973826
Re: RFR: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS [v3]
On Mon, 17 Jun 2024 13:55:41 GMT, Chen Liang wrote:
>> Currently, javap crashes for class files that have set non-zero values for
>> undefined access flag bits, as
>> `java.lang.reflect.AccessFlag.maskToAccessFlag` and
>> `java.lang.classfile.AccessFlags.flags` fail. In contrast, the JVMS, though
>> asking for these bits to be set to 0, requires VM to proceed and ignore
>> these bits. javap should emulate the VM behavior and proceed rendering,
>> ignoring these undefined bits.
>>
>> In addition, a few bytecode generation utilities in the JDK set unused bits,
>> such as in
>> `java.lang.invoke.MethodHandleImpl.BindCaller#generateInvokerTemplate` and
>> `java.lang.invoke.GenerateJLIClassesHelper#generateCodeBytesForLFs`. Those
>> can be updated in a later cleanup.
>
> Chen Liang has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Improve tests to check unmatched bit position and failure for
> non-inner-classes
> - Report error for flag problems
test/langtools/tools/javap/UndefinedAccessFlagTest.java line 105:
> 103: .writeAll()
> 104: .getOutputLines(Task.OutputKind.DIRECT);
> 105: assertTrue(lines.stream().anyMatch(l -> l.contains("Unmatched
> bit position")), () -> String.join("\n", lines));
I would add a check the "fatal" error does not occur or any other way to
confirm correct recovery.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19708#discussion_r1642972527
Re: RFR: 8334394: Race condition in Class::protectionDomain
On Mon, 17 Jun 2024 14:51:07 GMT, Weijun Wang wrote:
> Make sure `pd` is always the same object when `getProtectionDomain0` is null.
test/jdk/java/lang/Class/ProtectionDomainRace.java line 42:
> 40: try {
> 41: Subject.doAs(null, ac);
> 42: } catch (Throwable t) {
This can test for the racy NPE, but it cannot detect if there's distinct
allPermDomain objects written by races. Should we add another check to defend
against that?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19752#discussion_r1642969479
Re: RFR: 8334394: Race condition in Class::protectionDomain
On Mon, 17 Jun 2024 14:51:07 GMT, Weijun Wang wrote: > Make sure `pd` is always the same object when `getProtectionDomain0` is null. Looks great! I just wonder about how we usually write tests for such races, as these races may be platform-dependent and the test might not be able to always reproduce the race (such as with number of threads). - PR Comment: https://git.openjdk.org/jdk/pull/19752#issuecomment-2173658541
RFR: 8334394: Race condition in Class::protectionDomain
Make sure `pd` is always the same object when `getProtectionDomain0` is null. - Commit messages: - chmod - the fix Changes: https://git.openjdk.org/jdk/pull/19752/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19752=00 Issue: https://bugs.openjdk.org/browse/JDK-8334394 Stats: 84 lines in 2 files changed: 67 ins; 10 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19752.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19752/head:pull/19752 PR: https://git.openjdk.org/jdk/pull/19752
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v20]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 32 commits: - Merge branch 'master' into JDK-8294960-invoke # Conflicts: #src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java # src/java.base/share/classes/jdk/internal/classfile/impl/StackMapGenerator.java - applied suggested changes - Update src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java Co-authored-by: Chen Liang - reverted static initialization of ConstantPoolBuilder and CP entries - fixed naming conventions - use of jdk.internal.constant to improve performance - fixed imports - Apply suggestions from code review There are new possibilities with decoupled constants implementation, thank you for the reminder. Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Merge branch 'master' into JDK-8294960-invoke # Conflicts: # src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java - Merge branch 'master' into JDK-8294960-invoke # Conflicts: #src/java.base/share/classes/java/lang/classfile/Attributes.java - ... and 22 more: https://git.openjdk.org/jdk/compare/cdf22b13...3aaf246e - Changes: https://git.openjdk.org/jdk/pull/17108/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17108=19 Stats: 2166 lines in 10 files changed: 462 ins; 881 del; 823 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8329335: HttpsURLConnectionTest fails due to network firewall rules
On Mon, 17 Jun 2024 09:16:45 GMT, Fernando Guallini wrote: > Since HttpsURLConnectionTest attempts to reach external servers, it can fail > if run on hosts without outbound traffic allowed. Therefore, it should not be > executed in CI pipelines but rather manually on a host with no firewall rules > preventing egress traffic. Hi @fguallini , what if NAT Gateway of CI machines VCN is configured to allow one way initiation traffic to specific external server. Then this change is not required. - PR Comment: https://git.openjdk.org/jdk/pull/19745#issuecomment-2173618927
Re: RFR: 8333962: Obsolete OldSize [v2]
On Fri, 14 Jun 2024 10:19:47 GMT, Albert Mingkun Yang wrote: >> Obsolete OldSize and related code. An internal variable `OldSize` is kept to >> capture the capacity of old-gen size. > > Albert Mingkun Yang has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains one commit: > > obsolete-old-size Synced with master. Running some tests before merging. - PR Comment: https://git.openjdk.org/jdk/pull/19647#issuecomment-2173573858
Re: RFR: 8333962: Obsolete OldSize [v3]
> Obsolete OldSize and related code. An internal variable `OldSize` is kept to > capture the capacity of old-gen size. Albert Mingkun Yang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains one additional commit since the last revision: obsolete-old-size - Changes: - all: https://git.openjdk.org/jdk/pull/19647/files - new: https://git.openjdk.org/jdk/pull/19647/files/3e820b6f..a27f5172 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19647=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19647=01-02 Stats: 1483 lines in 67 files changed: 966 ins; 310 del; 207 mod Patch: https://git.openjdk.org/jdk/pull/19647.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19647/head:pull/19647 PR: https://git.openjdk.org/jdk/pull/19647
Re: RFR: 8334342: Add MergeStore JMH benchmarks [v4]
> [8318446](https://github.com/openjdk/jdk/pull/16245) brings MergeStore. We > need a JMH Benchmark to evaluate the performance of various batch operations > and the effect of MergeStore. Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: bug fix for `putChars4C` - Changes: - all: https://git.openjdk.org/jdk/pull/19734/files - new: https://git.openjdk.org/jdk/pull/19734/files/1ed9b22e..1140bd81 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19734=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19734=02-03 Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19734.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19734/head:pull/19734 PR: https://git.openjdk.org/jdk/pull/19734
Re: RFR: 8333962: Obsolete OldSize [v2]
On Fri, 14 Jun 2024 10:19:47 GMT, Albert Mingkun Yang wrote: >> Obsolete OldSize and related code. An internal variable `OldSize` is kept to >> capture the capacity of old-gen size. > > Albert Mingkun Yang has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains one commit: > > obsolete-old-size LGTM - Marked as reviewed by zgu (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19647#pullrequestreview-2123055719
RFR: 8329335: HttpsURLConnectionTest fails due to network firewall rules
Since HttpsURLConnectionTest attempts to reach external servers, it can fail if run on hosts without outbound traffic allowed. Therefore, it should not be executed in CI pipelines but rather manually on a host with no firewall rules preventing egress traffic. - Commit messages: - test should be run manually to avoid proxy problems Changes: https://git.openjdk.org/jdk/pull/19745/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19745=00 Issue: https://bugs.openjdk.org/browse/JDK-8329335 Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19745.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19745/head:pull/19745 PR: https://git.openjdk.org/jdk/pull/19745
Re: RFR: 8333962: Obsolete OldSize [v2]
On Mon, 17 Jun 2024 13:25:00 GMT, Zhengyu Gu wrote: >> Albert Mingkun Yang has updated the pull request with a new target base due >> to a merge or a rebase. The pull request now contains one commit: >> >> obsolete-old-size > > src/hotspot/share/gc/shared/genArguments.hpp line 36: > >> 34: extern size_t MaxOldSize; >> 35: >> 36: extern size_t OldSize; > > Any reason we still want to keep `OldSize` variable? becase GCs really care > about are `init`, `min` and `max` values. The concept of "initial old-gen size" will always be there. If `OldSize` is removed, all readers need to be updated to `InitialHeapSize - NewSize`. It's not obvious that is definitely better/more readable. - PR Review Comment: https://git.openjdk.org/jdk/pull/19647#discussion_r1642885640
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Mon, 17 Jun 2024 12:54:44 GMT, Kevin Walls wrote: >> JMX uses APIs related to the Security Mananger which are deprecated. Use of >> AccessControlContext will be removed when Security Manager is removed. >> >> Until then, updates are needed to not require setting >> -Djava.security.manager=allow to use JMX authentication. > > Kevin Walls has updated the pull request incrementally with two additional > commits since the last revision: > > - style update > - whitespace The new version looks much better to me. I wonder if we can abstract away some of the repeated logic though. src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java line 1314: > 1312: return AccessController.doPrivileged(action, acc); > 1313: } > 1314: } This piece of code is repeated at several places in similar but different forms - sometime we check for `SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime for `! SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime we check for `acc == null`, sometime for `acc != null` etc.. I wonder if we could abstract that to some utility method somewhere. Would that make sense? Maybe that's not possible due to using sometime `PrivelegedAction` and sometimes `PrivilegedExceptionAction` - but maybe we could use `PrivilegedExceptionAction` everywhere at the cost of handling `PrivilegedActionException`? If that doesn't prove useful (if handling `PrivilegedExceptionAction` adds again an equivalent amount of clutter) then maybe we could at least make sure we do the same checks in the same way (typically `acc == null` vs `acc != null`)? - PR Review: https://git.openjdk.org/jdk/pull/19624#pullrequestreview-2118725434 PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642864523
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v13]
On Fri, 14 Jun 2024 15:26:54 GMT, Kevin Walls wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Unnecessary catches to remove
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1437:
> 1435: } catch (Exception e) {
> 1436: if (e instanceof RuntimeException)
> 1437:throw (RuntimeException) e;
Suggestion:
if (e instanceof RuntimeException rte)
throw rte;
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1450:
> 1448: } catch (Exception e) {
> 1449: if (e instanceof RuntimeException)
> 1450:throw (RuntimeException) e;
Suggestion:
if (e instanceof RuntimeException rte)
throw rte;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1640033632
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1640034429
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v19]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java Co-authored-by: Chen Liang - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/f870a8df..9d105694 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=18 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=17-18 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Wed, 12 Jun 2024 20:27:05 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - reverted static initialization of ConstantPoolBuilder and CP entries
>> - fixed naming conventions
>
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 1148:
>
>> 1146: }
>> 1147:
>> 1148: private void emitPopInsn(CodeBuilder cob, BasicType type) {
>
> We need a TypeKind-aware pop in CodeBuilder too :(
Yes, it would be helpful.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642870775
Re: RFR: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS [v2]
On Mon, 17 Jun 2024 09:19:22 GMT, Adam Sotona wrote: >> Chen Liang has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - I am a dumbass >> - Retain strict flag for Method, even though it's obsolete > > `javap` should never silently ignore erroneous class file content. > If the flag value violates JVMS - it should be reported as an error and > `javap` should reflect that in the return value. > On the other hand `javap` should handle such situations, provide relevant > error message and continue with the report. > It is similar case as `javap` detection of inappropriate CP entry types. @asotona Thanks for the note, I have updated the code so javap now behaves like your suggestion. - PR Comment: https://git.openjdk.org/jdk/pull/19708#issuecomment-2173462470
Re: RFR: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS [v3]
> Currently, javap crashes for class files that have set non-zero values for > undefined access flag bits, as > `java.lang.reflect.AccessFlag.maskToAccessFlag` and > `java.lang.classfile.AccessFlags.flags` fail. In contrast, the JVMS, though > asking for these bits to be set to 0, requires VM to proceed and ignore these > bits. javap should emulate the VM behavior and proceed rendering, ignoring > these undefined bits. > > In addition, a few bytecode generation utilities in the JDK set unused bits, > such as in > `java.lang.invoke.MethodHandleImpl.BindCaller#generateInvokerTemplate` and > `java.lang.invoke.GenerateJLIClassesHelper#generateCodeBytesForLFs`. Those > can be updated in a later cleanup. Chen Liang has updated the pull request incrementally with two additional commits since the last revision: - Improve tests to check unmatched bit position and failure for non-inner-classes - Report error for flag problems - Changes: - all: https://git.openjdk.org/jdk/pull/19708/files - new: https://git.openjdk.org/jdk/pull/19708/files/84506788..0f3e1a97 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19708=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19708=01-02 Stats: 124 lines in 4 files changed: 55 ins; 34 del; 35 mod Patch: https://git.openjdk.org/jdk/pull/19708.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19708/head:pull/19708 PR: https://git.openjdk.org/jdk/pull/19708
Re: [External] : Re: New candidate JEP: 471: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal
On 17/06/2024 11:43, Andrew Dinn wrote: On 13/06/2024 14:39, Alan Bateman wrote: Good to hear you've got a prototype to discuss. I don't think I can look at what you have in your own repo but I do have a question. Do the defaultXXX methods return a method handle or do they fail/null when there are read/writeObject methods? Asking if they will bypass these methods. Why can you not look at David's repo? He is a JDK author (dmlloyd) and this work is being done as part of his employment by Red Hat. He has created a draft PR against openjdk/jdk so that removes any question as to whether there is an issue looking at the change in another github repo. -Alan
Re: RFR: 8333962: Obsolete OldSize [v2]
On Fri, 14 Jun 2024 10:19:47 GMT, Albert Mingkun Yang wrote: >> Obsolete OldSize and related code. An internal variable `OldSize` is kept to >> capture the capacity of old-gen size. > > Albert Mingkun Yang has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains one commit: > > obsolete-old-size src/hotspot/share/gc/shared/genArguments.hpp line 36: > 34: extern size_t MaxOldSize; > 35: > 36: extern size_t OldSize; Any reason we still want to keep `OldSize` variable? becase GCs really care about are `init`, `min` and `max` values. - PR Review Comment: https://git.openjdk.org/jdk/pull/19647#discussion_r1642814306
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 6 Jun 2024 13:17:20 GMT, Chen Liang wrote: >> Adam Sotona has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - reverted static initialization of ConstantPoolBuilder and CP entries >> - fixed naming conventions > > src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java > line 470: > >> 468: MethodVisitor mv = cw.visitMethod(ACC_PRIVATE + ACC_FINAL, >> 469: NAME_METHOD_WRITE_OBJECT, >> DESCR_METHOD_WRITE_OBJECT, >> 470: null, SER_HOSTILE_EXCEPTIONS); > > The exceptions attribute is now lost on the migrated methods. Is that ok? > It's private and only accessed by reflection so I think there's no real > impact. Good catch, they were lost in translation. I think we should keep them. - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642800017
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 6 Jun 2024 12:22:41 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - reverted static initialization of ConstantPoolBuilder and CP entries
>> - fixed naming conventions
>
> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
> line 96:
>
>> 94: }
>> 95: };
>> 96: record MethodBody(Consumer code) implements
>> Consumer {
>
> Why do we have these 2 instead of a noop record field builder consumer (flags
> is already set in withField, and MethodBody should just be direct usage of
> withMethodBody)
>
> Seems the problem is in CF implemetnation side. Then these should be part of
> CF implementation details.
There is no problem if you can rely on lambdas ;)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642793485
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 6 Jun 2024 12:17:14 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - reverted static initialization of ConstantPoolBuilder and CP entries
>> - fixed naming conventions
>
> src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java
> line 564:
>
>> 562: clb.withFlags(ACC_PRIVATE | ACC_FINAL | ACC_SUPER)
>> 563:
>> .withSuperclass(InvokerBytecodeGenerator.INVOKER_SUPER_DESC)
>> 564:
>> .with(SourceFileAttribute.of(className.substring(className.lastIndexOf('/')
>> + 1)));
>
> Maybe we can keep the classDesc from ofInternalName and use its displayName.
I don't see any benefits, both ways are sub-stringing.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642789436
Re: RFR: 8334342: Add MergeStore JMH benchmarks [v3]
> [8318446](https://github.com/openjdk/jdk/pull/16245) brings MergeStore. We > need a JMH Benchmark to evaluate the performance of various batch operations > and the effect of MergeStore. Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: bug fix for `putChars4C` and `putChars4S` - Changes: - all: https://git.openjdk.org/jdk/pull/19734/files - new: https://git.openjdk.org/jdk/pull/19734/files/65bb597d..1ed9b22e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19734=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19734=01-02 Stats: 10 lines in 1 file changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/19734.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19734/head:pull/19734 PR: https://git.openjdk.org/jdk/pull/19734
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v15]
On Mon, 17 Jun 2024 12:33:08 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - leave noPermissionsACC in place for now
>> - leave noPermissionsACC in place for now
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1436:
>
>> 1434: return op.run();
>> 1435: } catch (Exception e) {
>> 1436: if (e instanceof RuntimeException)
>
> Enclose the next line in braces.
Yes this file is full of pre-existing style problems like that. I can update
this and its friend at 1451.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642763028
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with two additional commits since the last revision: - style update - whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/2f2179e7..597264b9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19624=15 - incr: https://webrevs.openjdk.org/?repo=jdk=19624=14-15 Stats: 11 lines in 2 files changed: 4 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 6 Jun 2024 12:13:44 GMT, Chen Liang wrote: >> Adam Sotona has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - reverted static initialization of ConstantPoolBuilder and CP entries >> - fixed naming conventions > > src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 784: > >> 782: // mix them up and load them for the >> transform helper: >> 783: List helperArgs = >> speciesData.deriveTransformHelperArguments(transformMethods.get(whichtm), >> whichtm, targs, tfields); >> 784: List> helperTypes = new >> ArrayList<>(helperArgs.size()); > > Can we convert helperTypes here to List so we construct > invokeBasicType as MethodTypeDesc below? This part can be simplified to a directly used validated array of ClassDesc and many conversions can be skipped. - PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642756354
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v15]
On Mon, 17 Jun 2024 12:33:47 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - leave noPermissionsACC in place for now
>> - leave noPermissionsACC in place for now
>
> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
> line 349:
>
>> 347: @SuppressWarnings("removal")
>> 348: private Subject getSubject() {
>> 349:return Subject.current();
>
> Add a leading whitespace.
got it
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642749038
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v15]
On Mon, 17 Jun 2024 10:03:27 GMT, Kevin Walls wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> Kevin Walls has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - leave noPermissionsACC in place for now
> - leave noPermissionsACC in place for now
Looks good to me. Just 2 tiny coding style issues.
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1436:
> 1434: return op.run();
> 1435: } catch (Exception e) {
> 1436: if (e instanceof RuntimeException)
Enclose the next line in braces.
src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
line 349:
> 347: @SuppressWarnings("removal")
> 348: private Subject getSubject() {
> 349:return Subject.current();
Add a leading whitespace.
-
Marked as reviewed by weijun (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19624#pullrequestreview-2122787201
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642742989
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642743676
[jdk23] Integrated: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
On Mon, 17 Jun 2024 08:19:18 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8211847](https://bugs.openjdk.org/browse/JDK-8211847), commit > [f5213671](https://github.com/openjdk/jdk/commit/f5213671f7b636b32bb93c78e43696a61cd69bae) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Christoph Langer on 13 Jun 2024 > and was reviewed by Thomas Stuefe. > > Thanks! This pull request has now been integrated. Changeset: 4e3bfc92 Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/4e3bfc926ebdf512c7b9dbddc8caa7b66e2777f7 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected" Reviewed-by: mbaesken Backport-of: f5213671f7b636b32bb93c78e43696a61cd69bae - PR: https://git.openjdk.org/jdk/pull/19742
Re: RFR: 8334342: Add MergeStore JMH benchmarks [v2]
> [8318446](https://github.com/openjdk/jdk/pull/16245) brings MergeStore. We > need a JMH Benchmark to evaluate the performance of various batch operations > and the effect of MergeStore. Shaojin Wen has updated the pull request incrementally with four additional commits since the last revision: - use VarHandler CHAR_L & CHAR_B - copyright - bug fix for putIntBU - add cases for `getChar` & `putChar` - Changes: - all: https://git.openjdk.org/jdk/pull/19734/files - new: https://git.openjdk.org/jdk/pull/19734/files/72a6256a..65bb597d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19734=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19734=00-01 Stats: 322 lines in 1 file changed: 276 ins; 14 del; 32 mod Patch: https://git.openjdk.org/jdk/pull/19734.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19734/head:pull/19734 PR: https://git.openjdk.org/jdk/pull/19734
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
On Thu, 13 Jun 2024 09:27:44 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 616:
>>
>>> 614: final ClassDesc classDesc = ClassDesc.of(className0);
>>> 615: final ClassDesc superClassDesc =
>>> classDesc(speciesData.deriveSuperClass());
>>> 616: return ClassFile.of().build(classDesc, clb -> {
>>
>> We need a confirmation here that these BMH species are only initialized
>> after LMF is ready. @cl4es Is a dependency on LMF from BMH safe?
>
> LMF will BMH and `ClassSpecializer`, but does not call into this code
> normally as the simple bound method handle needed by LMF is statically
> defined (`BoundMethodHandle.Species_L`). To be perfectly safe - and to keep
> lambda/indy/condy bootstrap overheads to a minumum - I'll continue
> recommending the desugaring of lambdas in java.lang.invoke
It results in not very nice code, but makes sense.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1642653369
Re: [External] : Re: New candidate JEP: 471: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal
On 13/06/2024 14:39, Alan Bateman wrote: Good to hear you've got a prototype to discuss. I don't think I can look at what you have in your own repo but I do have a question. Do the defaultXXX methods return a method handle or do they fail/null when there are read/writeObject methods? Asking if they will bypass these methods. Why can you not look at David's repo? He is a JDK author (dmlloyd) and this work is being done as part of his employment by Red Hat. regards, Andrew Dinn ---
Re: [jdk23] RFR: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
On Mon, 17 Jun 2024 08:19:18 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8211847](https://bugs.openjdk.org/browse/JDK-8211847), commit > [f5213671](https://github.com/openjdk/jdk/commit/f5213671f7b636b32bb93c78e43696a61cd69bae) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Christoph Langer on 13 Jun 2024 > and was reviewed by Thomas Stuefe. > > Thanks! Looks okay, but please reconsider the description text of the bug, is it still what you do? - Marked as reviewed by mbaesken (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19742#pullrequestreview-2122517377
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 14:48:14 GMT, Weijun Wang wrote: > > Does noPermissionsACC add anything? > > I don't know. My principal for this code change is that nothing is changed > for the SM-is-allowed case. I've put back the noPermissionsACC for this change, it does not have to be removed in this change. It was introduced in jdk6, and we will remove it next time we are here when we drop ACC usage. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2172948152
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v15]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request incrementally with two additional commits since the last revision: - leave noPermissionsACC in place for now - leave noPermissionsACC in place for now - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/ee2dd12e..2f2179e7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19624=14 - incr: https://webrevs.openjdk.org/?repo=jdk=19624=13-14 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v13]
On Sun, 16 Jun 2024 01:54:34 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Unnecessary catches to remove
>
> src/java.management/share/classes/javax/management/monitor/Monitor.java line
> 1542:
>
>> 1540: if
>> (!SharedSecrets.getJavaLangAccess().allowSecurityManager()) {
>> 1541: // No SecurityManager permitted:
>> 1542: Subject.doAs(s, action); // s is permitted to be null
>
> While `s` is permitted to be null, calling `Subject.doAs(null, action)`
> actually sets the current subject to null while calling `action`. This is not
> same as directly calling `action` where the current subject (could be non
> null) is used.
Yes, good point, got it. 8-)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642504749
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v14]
> JMX uses APIs related to the Security Mananger which are deprecated. Use of > AccessControlContext will be removed when Security Manager is removed. > > Until then, updates are needed to not require setting > -Djava.security.manager=allow to use JMX authentication. Kevin Walls has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 19 additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 844_JMX_Subject - Monitor: do not call doAs with an explicityly null Subject - Unnecessary catches to remove - Monitor update - ServerNotifForwarder simplification - Separate SM allowed and not allowed cases - ServerNotifForwarder getSubject update - Exception handling per Daniel comment - Annotation in checkAccess - Undo test policy updates - ... and 9 more: https://git.openjdk.org/jdk/compare/c0b2e87b...ee2dd12e - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/13d9202f..ee2dd12e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19624=13 - incr: https://webrevs.openjdk.org/?repo=jdk=19624=12-13 Stats: 15038 lines in 603 files changed: 6231 ins; 7274 del; 1533 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
Re: RFR: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS [v2]
On Fri, 14 Jun 2024 17:02:40 GMT, Chen Liang wrote: >> Currently, javap crashes for class files that have set non-zero values for >> undefined access flag bits, as >> `java.lang.reflect.AccessFlag.maskToAccessFlag` and >> `java.lang.classfile.AccessFlags.flags` fail. In contrast, the JVMS, though >> asking for these bits to be set to 0, requires VM to proceed and ignore >> these bits. javap should emulate the VM behavior and proceed rendering, >> ignoring these undefined bits. >> >> In addition, a few bytecode generation utilities in the JDK set unused bits, >> such as in >> `java.lang.invoke.MethodHandleImpl.BindCaller#generateInvokerTemplate` and >> `java.lang.invoke.GenerateJLIClassesHelper#generateCodeBytesForLFs`. Those >> can be updated in a later cleanup. > > Chen Liang has updated the pull request incrementally with two additional > commits since the last revision: > > - I am a dumbass > - Retain strict flag for Method, even though it's obsolete `javap` should never silently ignore erroneous class file content. If the flag value violates JVMS - it should be reported as an error and `javap` should reflect that in the return value. On the other hand `javap` should handle such situations, provide relevant error message and continue with the report. It is similar case as `javap` detection of inappropriate CP entry types. - PR Comment: https://git.openjdk.org/jdk/pull/19708#issuecomment-2172842079
Re: RFR: 8307818: Convert Indify tool to Classfile API [v10]
On Fri, 24 May 2024 17:12:34 GMT, Oussama Louati wrote: >> test/jdk/java/lang/invoke/indify/Indify.java line 503: >> >>> 501: >>> 502: Iterator instructionIterator >>> =getInstructions(m).iterator(); >>> 503: final Stack shouldProceedAfterIndyAdded = new >>> Stack<>(); >> >> What this stack of booleans suppose to do? > > We need a boolean value to determine if we should proceed after replacing the > appropriate "pop" instruction with an "invokedynamic" instruction. However, > instead of using just a boolean field, we use a stack. The reason for this is > that within the lambda expression, we can only use final variables. By using > a stack, we can update its value as needed, which is why this approach is > chosen. I see here an iteration over instructions of a method, where the whole class is retransformed in certain situations and some status is passed back in a stack of booleans. The whole conversion should be implemented in a single transformation. Original code repeatedly replaced instructions inline (that is BTW reason why added nops below), however architecture of ClassFile API is different, you are transforming one class into completely new class (free to remove and add as many elements as you need). You can compose transformations into complex trees and you can also collect information before the transformation, however the class transformation should be executed only once. - PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1642418788
[jdk23] RFR: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
Hi all, This pull request contains a backport of [JDK-8211847](https://bugs.openjdk.org/browse/JDK-8211847), commit [f5213671](https://github.com/openjdk/jdk/commit/f5213671f7b636b32bb93c78e43696a61cd69bae) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Christoph Langer on 13 Jun 2024 and was reviewed by Thomas Stuefe. Thanks! - Commit messages: - Backport f5213671f7b636b32bb93c78e43696a61cd69bae Changes: https://git.openjdk.org/jdk/pull/19742/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19742=00 Issue: https://bugs.openjdk.org/browse/JDK-8211847 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19742.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19742/head:pull/19742 PR: https://git.openjdk.org/jdk/pull/19742
Re: RFR: 8307818: Convert Indify tool to Classfile API [v10]
On Tue, 28 May 2024 11:03:18 GMT, Oussama Louati wrote: >> An indify tool in j.l.i tests (also in vmTestBase) convert some source-code >> private static methods with MT_ MH_, and INDY_ prefixes into MethodHandle, >> MethodType, and CallSite constants. >> It currently uses ad-hoc code to process class files and intends to migrate >> to ASM; but since we have the Classfile API, we can migrate to Classfile API >> instead. > > Oussama Louati has updated the pull request incrementally with one additional > commit since the last revision: > > chore: changed pool marking initialization to be done in one pass test/jdk/java/lang/invoke/indify/Indify.java line 197: > 195: break; > 196: case "-v": case "--verbose": case "--verbose=": > 197: verbose = booleanOption(a2); // more output Why the comments have been removed? - PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1642379144
Re: RFR: 8334342: Add MergeStore JMH benchmarks
On Mon, 17 Jun 2024 07:44:33 GMT, Emanuel Peter wrote: >> [8318446](https://github.com/openjdk/jdk/pull/16245) brings MergeStore. We >> need a JMH Benchmark to evaluate the performance of various batch operations >> and the effect of MergeStore. > > test/micro/org/openjdk/bench/java/lang/MergeStoreBench.java line 656: > >> 654: array[offset + 2] = (byte) (value >> 8); >> 655: array[offset + 3] = (byte) (value ); >> 656: } > > You say that here `MergeStore` does not work. That is because the indices are > increasing, but the shifts decreasing. So that does not work on little-endian > machines (most architectures), but I would expect it to work on big-endian > machines with https://github.com/openjdk/jdk/pull/19218. Big endian is often used in network data transmission scenarios, and it is common to process big endian data on a little endian machine. In this case, can it be optimized to Integer.reverseBytes & putInt on a LittleEndian machine? `setIntB -> setIntRL` - PR Review Comment: https://git.openjdk.org/jdk/pull/19734#discussion_r1642373495
Re: RFR: 8307818: Convert Indify tool to Classfile API [v10]
On Fri, 24 May 2024 17:12:24 GMT, Oussama Louati wrote: >> test/jdk/java/lang/invoke/indify/Indify.java line 578: >> >>> 576: classTransform = >>> ClassTransform.transformingMethodBodies(filter, codeTransform); >>> 577: classModel = of().parse( >>> 578: of().transform(classModel, >>> classTransform)); >> >> What this transform (in the middle of instructions iteration) does? > > It updates the current classfile so when moving to a next action we avoid > this runtime error: > >> STATUS:Failed.`main' threw exception: java.lang.VerifyError: Bad type on >> operand stack Injected transform to avoid verify error, that seems to me conceptually wrong. What is the root cause of the verify error and how the transform suppose to fix it? - PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1642372748
Re: RFR: 8334342: Add MergeStore JMH benchmarks
On Sun, 16 Jun 2024 07:17:16 GMT, Shaojin Wen wrote: > [8318446](https://github.com/openjdk/jdk/pull/16245) brings MergeStore. We > need a JMH Benchmark to evaluate the performance of various batch operations > and the effect of MergeStore. A few extra comments: There is already a `MergeStore` benchmark. I would prefer if you put yours next to it, unless if you have a good reason. About your list: getIntB getIntBU getIntL getIntLU getIntRB getIntRBU getIntRL getIntRLU getLongB getLongBU getLongL getLongLU getLongRB getLongRBU getLongRL getLongRLU -> Obviously a "MergeStore" optimization does not work for loads. But if it is important, then maybe we could generalize the optimizations from stores to loads. putChars4UC -> Does the putChars4UC get inlined? Because here you are storing 4 variables, this pattern is not handled by MergeStore. setIntB -> You say that here MergeStore does not work. That is because the indices are increasing, but the shifts decreasing. So that does not work on little-endian machines (most architectures), but I would expect it to work on big-endian machines with https://github.com/openjdk/jdk/pull/19218. setIntBU -> order seems messed up setIntRB setIntRBU setLongB setLongBU setLongRB setLongRBU -> I leave the rest for you to investigate. test/micro/org/openjdk/bench/java/lang/MergeStoreBench.java line 2: > 1: /* > 2: * Copyright (c) 2014, 2024, Oracle and/or its affiliates. All rights > reserved. I think for a new file, you don't want to backdate the copyright to `2014` ;) test/micro/org/openjdk/bench/java/lang/MergeStoreBench.java line 57: > 55: HI_BYTE_SHIFT = 0; > 56: LO_BYTE_SHIFT = 8; > 57: } I wonder if it would just be better to duplicate the tests. Because we may extend the `MergeStore` optimization such that it allows either order, and just adds a `Reverse` operations on the bytes/chars/ints. Then it would be nice to benchmark both orders on both little and big endian architectures. What do you think? test/micro/org/openjdk/bench/java/lang/MergeStoreBench.java line 656: > 654: array[offset + 2] = (byte) (value >> 8); > 655: array[offset + 3] = (byte) (value ); > 656: } You say that here `MergeStore` does not work. That is because the indices are increasing, but the shifts decreasing. So that does not work on little-endian machines (most architectures), but I would expect it to work on big-endian machines with https://github.com/openjdk/jdk/pull/19218. test/micro/org/openjdk/bench/java/lang/MergeStoreBench.java line 664: > 662: UNSAFE.putByte(array, address + 2, (byte) (value >> 16)); > 663: UNSAFE.putByte(array, address + 3, (byte) (value )); > 664: } Order messed up? test/micro/org/openjdk/bench/java/lang/MergeStoreBench.java line 850: > 848: UNSAFE.putChar(array, address + 4, c2); > 849: UNSAFE.putChar(array, address + 6, c3); > 850: } Does the `putChars4UC` get inlined? Because here you are storing 4 variables, this pattern is not handled by `MergeStore`. - PR Comment: https://git.openjdk.org/jdk/pull/19734#issuecomment-2172548153 PR Review Comment: https://git.openjdk.org/jdk/pull/19734#discussion_r1642327120 PR Review Comment: https://git.openjdk.org/jdk/pull/19734#discussion_r1642329554 PR Review Comment: https://git.openjdk.org/jdk/pull/19734#discussion_r1642341934 PR Review Comment: https://git.openjdk.org/jdk/pull/19734#discussion_r1642350184 PR Review Comment: https://git.openjdk.org/jdk/pull/19734#discussion_r1642343781
Re: RFR: 8225763: Inflater and Deflater should implement AutoCloseable [v3]
On Sun, 16 Jun 2024 06:36:00 GMT, Jaikiran Pai wrote: >> Can I please get a review of this enhancement which proposes to enhance >> `java.util.zip.Deflater/Inflater` classes to now implement `AutoCloseable`? >> >> The actual work for this was done a few years back when we discussed the >> proposed approaches and then I raised a RFR. At that time I couldn't take >> this to completion. The current changes in this PR involve the >> implementation that was discussed at that time and also have implemented the >> review suggestions from that time. Here are those previous discussions and >> reviews: >> >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-June/061079.html >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061177.html >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061229.html >> >> To summarize those discussions, we had concluded that: >> - `Deflater` and `Inflater` will implement the `AutoCloseable` interface >> - In the `close()` implementation we will invoke the `end()` method >> (`end()` can be potentially overridden by subclasses). >> - `close()` will be specified and implemented to be idempotent. Calling >> `close()` a second time or more will be a no-op. >> - Calling `end()` and then `close()`, although uncommon, will also support >> idempotency and that `close()` call will be a no-op. >> - However, calling `close()` and then `end()` will not guarantee idempotency >> and depending on the implementing subclass, the `end()` may throw an >> exception. >> >> New tests have been included as part of these changes and they continue to >> pass along with existing tests in tier1, tier2 and tier3. When I had >> originally added these new tests, I hadn't used junit. I can convert them to >> junit if that's preferable. >> >> I'll file a CSR shortly. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Chen's suggestion - improve code comment Hi Jai, thanks for picking this up. Several points of discussion here. Nothing fatal, but as I said in 2019 :-) just a few wrinkles to iron out. I looked mainly at Deflater, but I think the issues also all apply to Inflater too. 1) class specification I think the class specification needs to be clearer about the positioning of end() and close(). The end() method has done the real work of "closing" since the classes were introduced. We're retrofitting them to to have a close() method that's essentially just a synonym for end(), and it's still perfectly legal for clients and subclasses to call end() instead of close(). I think we need to say that close() is present mainly to support AutoCloseable and try-with-resources. 2) end() specification I don't think that "may throw an exception" is strong enough. Also, on these classes (not subclasses) end() is idemopotent, isn't it? If so, this should be specified. The end() specs need to say something like, closes and releases resources, etc., henceforth operations will throw an exception (which? -- see below) but that subsequent calls to end() do nothing. 3) Which exceptions on closed Deflater/Inflater? Oddly, it doesn't seem to be specified anywhere what exceptions are thrown if operations are attempted on a closed object. And the implementation actually throws NPE??!? Ideally we should specify what exception is thrown in this circumstance, but NPE seems wrong. Maybe we want to change the behavior to throw a different exception type. IllegalStateException seems like a reasonable candidate. 4) close() specification The 2019 discussion kind of assumed that we want close() to be idempotent. I'm not sure this is necessary. Alan said the implementation might need to be "heroic" but Peter Levart's arrangement (carried through here, apparently) isn't terribly bad. And @liach suggests more comments on this code, quite reasonably... but the comment begs the question, why do we need to avoid calling end() more than once? I'm rethinking the need for close() to be idempotent. Maybe close() should simply call end(). It would simplify the implementation and the specs -- the implSpec would simply say that it calls the end() method. This would only be a problem if a) some arrangement of TWR ends up calling close() twice, which apparently it doesn't; b) there is a subclass; and c) the subclass's end() method is not idempotent. As far as I can tell we haven't found any example of any of these. It thus seems to me that having extra logic in close() to avoid calling end() more than once is solving a non-problem. It also fits more closely to the model of "close() is simply a synonym for end()". - PR Comment: https://git.openjdk.org/jdk/pull/19675#issuecomment-2172433871
Re: RFR: 8331431: Update to use jtreg 7.4
On Thu, 2 May 2024 09:48:51 GMT, Christian Stein wrote: > Please review the change to update to using `jtreg` **7.4**. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > Tested: tier 1 — tier 5 Update the title to use the correct JBS issue number. > Looks good to me. I assume that you have run an extensive set of tests to > verify that this does not break, even in higher tiers? Yes. See below. > Hello Christian, it would be better to merge latest master branch into this > PR before integrating this. It looks like it currently uses master from more > than a month back. Latest tier 1-5 tests were run against late last week's HEAD revision. So, all looks good. - PR Comment: https://git.openjdk.org/jdk/pull/19052#issuecomment-2172374402
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Fri, 14 Jun 2024 04:04:48 GMT, lingjun-cg wrote:
>> src/java.base/share/classes/java/text/CompactNumberFormat.java line 885:
>>
>>> 883: }
>>> 884:
>>> 885: private StringBuilderBufferProxy format(BigInteger number,
>>> StringBuilderBufferProxy result,
>>
>> While the DecimalFormat and CompactNumberFormat BigInteger and BigDecimal
>> `format` methods will always return a StringBuffer, we must change the
>> method signatures to use the proxy, otherwise we would need to define
>> alternate methods all the way at the `Format.FieldDelegate` level. So I
>> think this is a lesser of two evils.
>
> I'm not sure what you mean. Would you suggest add two new methods that accept
> StringBuilderBufferProxy and keep the origin methods that accept
> StringBuffer? Like this:
>
> interface FieldDelegate {
>
> public void formatted(Format.Field attr, Object value, int start,
> int end, StringBuffer buffer);
>
> public void formatted(int fieldID, Format.Field attr, Object value,
> int start, int end, StringBuffer buffer);
>
> public void formatted(Format.Field attr, Object value, int start,
> int end, StringBuilderBufferProxy buffer);
>
> public void formatted(int fieldID, Format.Field attr, Object value,
> int start, int end, StringBuilderBufferProxy
> buffer);
> }
>
> But DecimalFormat#subformatNumber is a common method that only accept
> StringBuilderBufferProxy, and CompactNumberFormat#format(BigDecimal) call it,
> so CompactNumberFormat#format(BigDecimal) need change signature.
Ah, no, I was only making a observation. I agree that adding another set of
methods to FieldDelegate to support both buffer and proxy is not ideal. And as
you pointed out, `format(BigDecimal...` calls other methods that are reliant on
proxy anyways.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1642223921
