Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]
On Thu, 31 Mar 2022 08:03:23 GMT, Andrey Turbanov wrote:
>> Method `Class.isAssignableFrom` is often used in form of:
>>
>> if (clazz.isAssignableFrom(obj.getClass())) {
>> Such condition could be simplified to more shorter and performarnt code
>>
>> if (clazz.isInstance(obj)) {
>>
>> Replacement is equivalent if it's known that `obj != null`.
>> In JDK codebase there are many such places.
>>
>> I tried to measure performance via JMH
>>
>> Class integerClass = Integer.class;
>> Class numberClass = Number.class;
>>
>> Object integerObject = 45666;
>> Object doubleObject = 8777d;
>>
>> @Benchmark
>> public boolean integerInteger_isInstance() {
>> return integerClass.isInstance(integerObject);
>> }
>>
>> @Benchmark
>> public boolean integerInteger_isAssignableFrom() {
>> return integerClass.isAssignableFrom(integerObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean integerDouble_isInstance() {
>> return integerClass.isInstance(doubleObject);
>> }
>>
>> @Benchmark
>> public boolean integerDouble_isAssignableFrom() {
>> return integerClass.isAssignableFrom(doubleObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean numberDouble_isInstance() {
>> return numberClass.isInstance(doubleObject);
>> }
>>
>> @Benchmark
>> public boolean numberDouble_isAssignableFrom() {
>> return numberClass.isAssignableFrom(doubleObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean numberInteger_isInstance() {
>> return numberClass.isInstance(integerObject);
>> }
>>
>> @Benchmark
>> public boolean numberInteger_isAssignableFrom() {
>> return numberClass.isAssignableFrom(integerObject.getClass());
>> }
>>
>> @Benchmark
>> public void numberIntegerDouble_isInstance(Blackhole bh) {
>> bh.consume(numberClass.isInstance(integerObject));
>> bh.consume(numberClass.isInstance(doubleObject));
>> }
>>
>> @Benchmark
>> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
>> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
>> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
>> }
>>
>> `isInstance` is a bit faster than `isAssignableFrom`
>>
>> Benchmark Mode Cnt Score Error Units
>> integerDouble_isAssignableFrom avgt5 1,173 ± 0,026 ns/op
>> integerDouble_isInstance avgt5 0,939 ± 0,038 ns/op
>> integerIntegerDouble_isAssignableFrom avgt5 2,106 ± 0,068 ns/op
>> numberIntegerDouble_isInstance avgt5 1,516 ± 0,046 ns/op
>> integerInteger_isAssignableFromavgt5 1,175 ± 0,029 ns/op
>> integerInteger_isInstance avgt5 0,886 ± 0,017 ns/op
>> numberDouble_isAssignableFrom avgt5 1,172 ± 0,007 ns/op
>> numberDouble_isInstanceavgt5 0,891 ± 0,030 ns/op
>> numberInteger_isAssignableFrom avgt5 1,169 ± 0,014 ns/op
>> numberInteger_isInstance avgt5 0,887 ± 0,016 ns/op
>
> Andrey Turbanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8280035: Use Class.isInstance instead of Class.isAssignableFrom where
> applicable
> apply suggestion to avoid second Method.invoke call
Marked as reviewed by prr (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/7061
Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments
On Mon, 9 May 2022 15:56:35 GMT, Adam Sotona wrote:
> Please review this patch adding new lint option, **lossy-conversions**, to
> javac to warn about type casts in compound assignments with possible lossy
> conversions.
>
> The new lint warning is shown if the type of the right-hand operand of a
> compound assignment is not assignment compatible with the type of the
> variable.
>
> The implementation of the warning is based on similar check performed to emit
> "possible lossy conversion" compilation error for simple assignments.
>
> Proposed patch also include complex matrix-style test with positive and
> negative test cases of lossy conversions in compound assignments.
>
> Proposed patch also disables this new lint option in all affected JDK modules
> and libraries to allow smooth JDK build. Individual cases to address possibly
> lossy conversions warnings in JDK are already addressed in a separate
> umbrella issue and its sub-tasks.
>
> Thanks for your review,
> Adam
Marked as reviewed by prr (Reviewer).
test/langtools/tools/javac/lint/LossyConversions.java line 131:
> 129: @SuppressWarnings("lossy-conversions")
> 130: public void supressedLossyConversions() {
> 131: byte a = 0;
you might want to spell suppressed correctly.
-
PR: https://git.openjdk.java.net/jdk/pull/8599
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]
On Thu, 28 Apr 2022 01:31:13 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8285149: Using HashMap.newHashMap to replace new HashMap(int) [v4]
On Sat, 23 Apr 2022 14:34:58 GMT, XenoAmess wrote: >> These are the changes that too many to be reviewed in 8186958, thus split >> some of them out. > > XenoAmess has updated the pull request incrementally with two additional > commits since the last revision: > > - maintains compatibility with jdk1.4 for TextTests > - fix missed space after '=' I'm getting a bit tired of seeing these JDK wide changes with lots of files touched. Delete all changes in desktop from this PR and re-submit them as a separate PR. Also do not change J2DBench. We deliberately avoid using new API so that we can take it and run it on very old JDK versions to help track regressions. - PR: https://git.openjdk.java.net/jdk/pull/8301
Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines
On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan wrote: > Hi > > I have reviewed the code for removing double semicolons at the end of lines > > all the best > matteo Marked as reviewed by prr (Reviewer). Looks like there's only one client source code file touched Most of the client changes are only in jtreg tests - and this is very trivial, so I'm OK with them being included here. - PR: https://git.openjdk.java.net/jdk/pull/7268
Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable
On Thu, 13 Jan 2022 08:25:22 GMT, Andrey Turbanov wrote:
> Method `Class.isAssignableFrom` is often used in form of:
>
> if (clazz.isAssignableFrom(obj.getClass())) {
> Such condition could be simplified to more shorter and performarnt code
>
> if (clazz.isInstance(obj)) {
>
> Replacement is equivalent if it's known that `obj != null`.
> In JDK codebase there are many such places.
>
> I tried to measure performance via JMH
>
> Class integerClass = Integer.class;
> Class numberClass = Number.class;
>
> Object integerObject = 45666;
> Object doubleObject = 8777d;
>
> @Benchmark
> public boolean integerInteger_isInstance() {
> return integerClass.isInstance(integerObject);
> }
>
> @Benchmark
> public boolean integerInteger_isAssignableFrom() {
> return integerClass.isAssignableFrom(integerObject.getClass());
> }
>
> @Benchmark
> public boolean integerDouble_isInstance() {
> return integerClass.isInstance(doubleObject);
> }
>
> @Benchmark
> public boolean integerDouble_isAssignableFrom() {
> return integerClass.isAssignableFrom(doubleObject.getClass());
> }
>
> @Benchmark
> public boolean numberDouble_isInstance() {
> return numberClass.isInstance(doubleObject);
> }
>
> @Benchmark
> public boolean numberDouble_isAssignableFrom() {
> return numberClass.isAssignableFrom(doubleObject.getClass());
> }
>
> @Benchmark
> public boolean numberInteger_isInstance() {
> return numberClass.isInstance(integerObject);
> }
>
> @Benchmark
> public boolean numberInteger_isAssignableFrom() {
> return numberClass.isAssignableFrom(integerObject.getClass());
> }
>
> @Benchmark
> public void numberIntegerDouble_isInstance(Blackhole bh) {
> bh.consume(numberClass.isInstance(integerObject));
> bh.consume(numberClass.isInstance(doubleObject));
> }
>
> @Benchmark
> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
> }
>
> `isInstance` is a bit faster than `isAssignableFrom`
>
> Benchmark Mode Cnt Score Error Units
> integerDouble_isAssignableFrom avgt5 1,173 ± 0,026 ns/op
> integerDouble_isInstance avgt5 0,939 ± 0,038 ns/op
> integerIntegerDouble_isAssignableFrom avgt5 2,106 ± 0,068 ns/op
> numberIntegerDouble_isInstance avgt5 1,516 ± 0,046 ns/op
> integerInteger_isAssignableFromavgt5 1,175 ± 0,029 ns/op
> integerInteger_isInstance avgt5 0,886 ± 0,017 ns/op
> numberDouble_isAssignableFrom avgt5 1,172 ± 0,007 ns/op
> numberDouble_isInstanceavgt5 0,891 ± 0,030 ns/op
> numberInteger_isAssignableFrom avgt5 1,169 ± 0,014 ns/op
> numberInteger_isInstance avgt5 0,887 ± 0,016 ns/op
I've stared at the javadoc for Class.isAssignableFrom and Class.isInstance and
if a non-null instance is used to get a non-null class they are PROBABLY the
same but it is far from clear. The implementations of both are at least native
and may be instrinsicified. The doc for Class.isAssignableFrom cites JLS 5.1.4
which in what I found is about primitives so I suspect it is woefully out of
date
https://docs.oracle.com/javase/specs/jls/se17/html/jls-5.html#jls-5.1.4
What client tests have you run that touch the code you are changing ?
In short I see insufficient value in the changes here and would prefer you drop
the client part so I don't have to worry about it.
-
PR: https://git.openjdk.java.net/jdk/pull/7061
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal [v3]
On Wed, 1 Dec 2021 19:23:59 GMT, Brent Christian wrote:
>> Here are the code changes for the "Deprecate finalizers in the standard Java
>> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
>> review.
>>
>> This change makes the indicated deprecations, and updates the API spec for
>> JEP 421. It also updates the relevant @SuppressWarning annotations.
>>
>> The CSR has been approved.
>> An automated test build+test run passes cleanly (FWIW :D ).
>
> Brent Christian has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 33 commits:
>
> - Merge branch 'master' into 8276447
> - Merge branch 'master' into 8276447
> - merge
> - @SuppressWarnings(removal) in Windows CKey.java
> - Add jls tag to runFinalization methods
> - Update wording of Object.finalize, new paragraph is now bold
> - update Object.finalize javadoc
> - update Object.finalize JavaDoc and @deprecated tag
> - Update Object.finalize deprecation message
> - Indicate that runFinalizers does nothing if finalization is disabled or
> removed
> - ... and 23 more:
> https://git.openjdk.java.net/jdk/compare/0dfb3a70...8cde0674
Marked as reviewed by prr (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang wrote:
>> The code change refactors classes that have a `SuppressWarnings("removal")`
>> annotation that covers more than 50KB of code. The big annotation is often
>> quite faraway from the actual deprecated API usage it is suppressing, and
>> with the annotation covering too big a portion it's easy to call other
>> deprecated methods without being noticed.
>>
>> The code change shows some common solutions to avoid such too wide
>> annotations:
>>
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local
>> variable and add annotation on the declaration, and then assign that value
>> to the original l-value if not void. The local variable will be called `tmp`
>> if later reassigned or `dummy` if it will be discarded.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>>
>> I'll add a copyright year update commit before integration.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update FtpClient.java
Marked as reviewed by prr (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Fri, 28 May 2021 02:50:55 GMT, Weijun Wang wrote:
>> src/java.desktop/share/classes/java/awt/Component.java line 630:
>>
>>> 628: }
>>> 629:
>>> 630: @SuppressWarnings("removal")
>>
>> I'm confused. I thought the reason this wasn't done in the JEP
>> implementation PR is because of refactoring
>> that was needed because of the usage in this static block and you could not
>> apply the annotation here.
>> Yet it seems you are doing exactly what was supposed to be impossible with
>> no refactoring.
>> Can you explain ?
>
> There *is* a tiny refactoring here: a new variable `s2` is introduced so the
> 2nd `doPrivileged` call on line 636 is now also in a declaration statement
> (for `s2`) and therefore annotatable. Without this I cannot add the
> annotation on line 635.
Ok. But I will quote you
"This happens when a deprecated method is called inside a static block. The
annotation can only be added to a declaration and here it must be the whole
class"
So the way you explained this before made it sound like any time there was any
SM API usage in a static block, the entire class needed to be annotated.
Why has the explanation changed ?
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang wrote:
>> The code change refactors classes that have a `SuppressWarnings("removal")`
>> annotation that covers more than 50KB of code. The big annotation is often
>> quite faraway from the actual deprecated API usage it is suppressing, and
>> with the annotation covering too big a portion it's easy to call other
>> deprecated methods without being noticed.
>>
>> The code change shows some common solutions to avoid such too wide
>> annotations:
>>
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local
>> variable and add annotation on the declaration, and then assign that value
>> to the original l-value if not void. The local variable will be called `tmp`
>> if later reassigned or `dummy` if it will be discarded.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>>
>> I'll add a copyright year update commit before integration.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update FtpClient.java
src/java.desktop/share/classes/java/awt/Component.java line 630:
> 628: }
> 629:
> 630: @SuppressWarnings("removal")
I'm confused. I thought the reason this wasn't done in the JEP implementation
PR is because of refactoring
that was needed because of the usage in this static block and you could not
apply the annotation here.
Yet it seems you are doing exactly what was supposed to be impossible with no
refactoring.
Can you explain ?
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]
On Mon, 24 May 2021 13:53:34 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > keep only one systemProperty tag Marked as reviewed by prr (Reviewer). I'm OK with this now given that the refactoring is already underway at https://github.com/openjdk/jdk/pull/4138 - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang wrote:
>> Please review this implementation of [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> The code change is divided into 3 commits. Please review them one by one.
>>
>> 1.
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>> The essential change for this JEP, including the `@Deprecate` annotations
>> and spec change. It also update the default value of the
>> `java.security.manager` system property to "disallow", and necessary test
>> change following this update.
>> 2.
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>> Manual changes to several files so that the next commit can be generated
>> programatically.
>> 3.
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>> Automatic changes to other source files to avoid javac warnings on
>> deprecation for removal
>>
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
>> generated programmatically, see the comment below for more details. If you
>> are only interested in a portion of the 3rd commit and would like to review
>> it as a separate file, please comment here and I'll generate an individual
>> webrev.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> any file to minimize unnecessary merge conflict.
>>
>> Furthermore, since the default value of `java.security.manager` system
>> property is now "disallow", most of the tests calling
>> `System.setSecurityManager()` need to launched with
>> `-Djava.security.manager=allow`. This is covered in a different PR at
>> https://github.com/openjdk/jdk/pull/4071.
>>
>> Update: the deprecation annotations and javadoc tags, build, compiler,
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
I haven't seen any response to this comment I made a couple of days ago and I
suspect it got missed
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {
Who or what prompted this change ?
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]
On Tue, 18 May 2021 21:44:43 GMT, Weijun Wang wrote:
>> Please review the test changes for [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> With JEP 411 and the default value of `-Djava.security.manager` becoming
>> `disallow`, tests calling `System.setSecurityManager()` need
>> `-Djava.security.manager=allow` when launched. This PR covers such changes
>> for tier1 to tier3 (except for the JCK tests).
>>
>> To make it easier to focus your review on the tests in your area, this PR is
>> divided into multiple commits for different areas (~~serviceability~~,
>> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
>> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
>> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is
>> the same as how Skara adds labels, but there are several small tweaks:
>>
>> 1. When a file is covered by multiple labels, only one is chosen. I make my
>> best to avoid putting too many tests into `core-libs`. If a file is covered
>> by `core-libs` and another label, I categorized it into the other label.
>> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
>> `xml` commit.
>> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
>> `rmi` commit.
>> 4. One file not covered by any label --
>> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
>> the `swing` commit.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> all files to minimize unnecessary merge conflict.
>>
>> Please note that this PR can be integrated before the source changes for JEP
>> 411, as the possible values of this system property was already defined long
>> time ago in JDK 9.
>>
>> Most of the change in this PR is a simple adding of
>> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes
>> it was not `othervm` and we add one. Sometimes there's no `@run` at all and
>> we add the line.
>>
>> There are several tests that launch another Java process that needs to call
>> the `System.setSecurityManager()` method, and the system property is added
>> to `ProcessBuilder`, `ProcessTools`, or the java command line (if the test
>> is a shell test).
>>
>> 3 langtools tests are added into problem list due to
>> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>>
>> 2 SQL tests are moved because they need different options on the `@run` line
>> but they are inside a directory that has a `TEST.properties`:
>>
>> rename test/jdk/java/sql/{testng/test/sql/othervm =>
>> permissionTests}/DriverManagerPermissionsTests.java (93%)
>> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
>> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>> ```
>>
>> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> adjust order of VM options
The client changes are fine except for the one misplaced (c)
test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3:
> 1: /*
> 2: * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights
> reserved.
> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
Probably the (c) update was meant to be on the .sh file that is actually
modified.
-
Marked as reviewed by prr (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 07:06:00 GMT, Alan Bateman wrote: >> The JEP isn't PTT for 17 so there's plenty of time isn't there ? > > There are 827 files in this patch. Phil is right that adding SW at the class > level is introducing technical debt but if addressing that requires > refactoring and re-testing of AWT or font code then it would be better to > have someone from that area working on. Is there any reason why this can't be > going on now on awt-dev/2d-dev and integrated immediately after JEP 411? I > don't think we should put Max through the wringer here as there are too many > areas to cover. Are you suggesting that the patch doesn't need testing as it is ? It should be the same either way. It is very straight forward to run the automated tests across all platforms these days. I get the impression that no one is guaranteeing to do this straight after integration. It sounds like it is up for deferral if time runs out. The amount of follow-on work that I am hearing about here, and may be for tests, does not make it sound like this JEP is nearly as done as first presented. If there was some expectation that groups responsible for an area would get involved with this issue which I am assured was already known about, then why was it not raised before and made part of the plan ? - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 04:05:23 GMT, Phil Race wrote: >> By converting JDK-8267432 to a bug, there is an extra benefit that we can >> fix it after RDP. So I'll convert it now. > > That is unfortunate, but nonetheless I think required to be done. > We have acknowledeged this can't reasonably be called an RFE, so the JEP is > introducing bugs/technical debt/call it what you will. This should generally > be part of a sandbox for the JEP and fixed before integration of the JEP. > From my point of view it is a blocker. The JEP isn't PTT for 17 so there's plenty of time isn't there ? - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 02:09:57 GMT, Weijun Wang wrote: >> I can make it a bug. >> >> I don't want to do it here is because it involves indefinite amount of >> manual work and we will see everyone having their preferences. The more time >> we spend on this PR the more likely there will be merge conflict. There are >> just too many files here. >> >> And no matter if we include that change in this PR or after it, it will be >> after the automatic conversion. In fact, the data about which cases are more >> worth fixing come from the automatic conversion itself. Also, as the manual >> work will be done part by part, if the automatic conversion is after it, >> there will be rounds and rounds of history rewriting and force push. This is >> quite unfriendly to the reviewers. >> >> Altogether, there are 117 class-level annotations. Unfortunately, >> `java.awt.Component` is the one with the biggest class -- estimated number >> of bytes that the annotation covers is over 380K. In the client area, the >> 2nd place is `java.awt.Container`, and then we have >> `sun.font.SunFontManager`, `java.awt.Window`, and `java.awt.Toolkit` which >> are over 100KB, and other 25 classes over 10KB, and other 11 classes smaller >> than 10KB. > > By converting JDK-8267432 to a bug, there is an extra benefit that we can fix > it after RDP. So I'll convert it now. That is unfortunate, but nonetheless I think required to be done. We have acknowledeged this can't reasonably be called an RFE, so the JEP is introducing bugs/technical debt/call it what you will. This should generally be part of a sandbox for the JEP and fixed before integration of the JEP. >From my point of view it is a blocker. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 22:14:20 GMT, Weijun Wang wrote: >> I don't think it is a separate P4 enhancement (?) that someone will (maybe) >> do next release. >> I think it should all be taken care of as part of this proposed change. > > I just made it P3 (P4 was the default value), and I will target it to 17 once > JEP 411 is targeted 17. But I think it's probably not a good idea to include > it inside *this* PR. There are some middle ground where it's debatable if a > change is worth doing (Ex: which is uglier between an > a-liitle-faraway-annotation and a temporary variable?) so it's not obvious > what the scope of the refactoring can be. Thus it will be divided into > smaller sub-tasks. It's not totally impossible that part of the work will be > delayed to next release. Well .. as an enhancement (P3 or otherwise) I can see it being dropped and definitely if it misses the fork, and I don't get why you can't do it here. And if it isn't done the JEP isn't really ready. I already pasted the patch for Component.java and it took about 60 seconds to do that. Then yes, you would have to deal with the fact that now you need to reapply your automated tool to the file but this is just work you'd have to have done anyway if it was already refactored. I only *noticed* Component and Container. And stopped there to raise the question. How many more cases are there ? - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 21:53:35 GMT, Weijun Wang wrote:
>> That's a sad limitation of the annotation stuff then, but I don't think that
>> it is insurmountable.
>> You can define a static private method to contain this and call it from the
>> static initializer block.
>> Much better than applying the annotation to an entire class.
>>
>> --- a/src/java.desktop/share/classes/java/awt/Component.java
>> +++ b/src/java.desktop/share/classes/java/awt/Component.java
>> @@ -618,6 +618,17 @@ public abstract class Component implements
>> ImageObserver, MenuContainer,
>> */
>> static boolean isInc;
>> static int incRate;
>> +
>> +private static void initIncRate() {
>> +String s = java.security.AccessController.doPrivileged(
>> + new
>> GetPropertyAction("awt.image.incrementaldraw"));
>> +isInc = (s == null || s.equals("true"));
>> +
>> +s = java.security.AccessController.doPrivileged(
>> + new GetPropertyAction("awt.image.redrawrate"));
>> +incRate = (s != null) ? Integer.parseInt(s) : 100;
>> +}
>> +
>> static {
>> /* ensure that the necessary native libraries are loaded */
>> Toolkit.loadLibraries();
>> @@ -625,14 +636,7 @@ public abstract class Component implements
>> ImageObserver, MenuContainer,
>> if (!GraphicsEnvironment.isHeadless()) {
>> initIDs();
>> }
>> -
>> -String s = java.security.AccessController.doPrivileged(
>> - new
>> GetPropertyAction("awt.image.incrementaldraw"));
>> -isInc = (s == null || s.equals("true"));
>> -
>> -s = java.security.AccessController.doPrivileged(
>> -new
>> GetPropertyAction("awt.image.redrawrate"));
>> -incRate = (s != null) ? Integer.parseInt(s) : 100;
>> +initIncRate();
>> }
>
> Correct, there are ways to modify the code to make it more
> annotation-friendly. We thought about whether it's good to do it before
> adding the annotations or after it. Our decision now is to do it after
> because it will be more easy to see why it's necessary and we can take time
> to do them little by little. A new enhancement at
> https://bugs.openjdk.java.net/browse/JDK-8267432 is filed.
I don't think it is a separate P4 enhancement (?) that someone will (maybe) do
next release.
I think it should all be taken care of as part of this proposed change.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 18:38:39 GMT, Weijun Wang wrote:
>> src/java.desktop/share/classes/java/awt/Component.java line 217:
>>
>>> 215: * @author Sami Shaio
>>> 216: */
>>> 217: @SuppressWarnings("removal")
>>
>> Why is this placed on the *entire class* ??
>> This class is 10535 lines long and mentions AccessControl something like 8
>> places it uses AccessController or AcessControlContext.
>
> This happens when a deprecated method is called inside a static block. The
> annotation can only be added to a declaration and here it must be the whole
> class. The call in this file is
>
> s = java.security.AccessController.doPrivileged(
> new
> GetPropertyAction("awt.image.redrawrate"));
That's a sad limitation of the annotation stuff then, but I don't think that it
is insurmountable.
You can define a static private method to contain this and call it from the
static initializer block.
Much better than applying the annotation to an entire class.
--- a/src/java.desktop/share/classes/java/awt/Component.java
+++ b/src/java.desktop/share/classes/java/awt/Component.java
@@ -618,6 +618,17 @@ public abstract class Component implements ImageObserver,
MenuContainer,
*/
static boolean isInc;
static int incRate;
+
+private static void initIncRate() {
+String s = java.security.AccessController.doPrivileged(
+ new
GetPropertyAction("awt.image.incrementaldraw"));
+isInc = (s == null || s.equals("true"));
+
+s = java.security.AccessController.doPrivileged(
+ new GetPropertyAction("awt.image.redrawrate"));
+incRate = (s != null) ? Integer.parseInt(s) : 100;
+}
+
static {
/* ensure that the necessary native libraries are loaded */
Toolkit.loadLibraries();
@@ -625,14 +636,7 @@ public abstract class Component implements ImageObserver,
MenuContainer,
if (!GraphicsEnvironment.isHeadless()) {
initIDs();
}
-
-String s = java.security.AccessController.doPrivileged(
- new
GetPropertyAction("awt.image.incrementaldraw"));
-isInc = (s == null || s.equals("true"));
-
-s = java.security.AccessController.doPrivileged(
-new
GetPropertyAction("awt.image.redrawrate"));
-incRate = (s != null) ? Integer.parseInt(s) : 100;
+initIncRate();
}
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang wrote:
>> Please review this implementation of [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> The code change is divided into 3 commits. Please review them one by one.
>>
>> 1.
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>> The essential change for this JEP, including the `@Deprecate` annotations
>> and spec change. It also update the default value of the
>> `java.security.manager` system property to "disallow", and necessary test
>> change following this update.
>> 2.
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>> Manual changes to several files so that the next commit can be generated
>> programatically.
>> 3.
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>> Automatic changes to other source files to avoid javac warnings on
>> deprecation for removal
>>
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
>> generated programmatically, see the comment below for more details. If you
>> are only interested in a portion of the 3rd commit and would like to review
>> it as a separate file, please comment here and I'll generate an individual
>> webrev.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> any file to minimize unnecessary merge conflict.
>>
>> Furthermore, since the default value of `java.security.manager` system
>> property is now "disallow", most of the tests calling
>> `System.setSecurityManager()` need to launched with
>> `-Djava.security.manager=allow`. This is covered in a different PR at
>> https://github.com/openjdk/jdk/pull/4071.
>>
>> Update: the deprecation annotations and javadoc tags, build, compiler,
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
src/java.desktop/share/classes/java/awt/Component.java line 217:
> 215: * @author Sami Shaio
> 216: */
> 217: @SuppressWarnings("removal")
Why is this placed on the *entire class* ??
This class is 10535 lines long and mentions AccessControl something like 8
places it uses AccessController or AcessControlContext.
src/java.desktop/share/classes/java/awt/Container.java line 97:
> 95: * @since 1.0
> 96: */
> 97: @SuppressWarnings("removal")
Same issue as with Component. a > 5,000 line file that uses AccessController in
just 4 places.
Where else are you adding this to entire classes instead of the specific site ?
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java This change is so large that github won't even display the list of files so I can't jump to where I want to go ! And when I try to scroll I get just a blank page. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang wrote:
>> Please review this implementation of [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> The code change is divided into 3 commits. Please review them one by one.
>>
>> 1.
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>> The essential change for this JEP, including the `@Deprecate` annotations
>> and spec change. It also update the default value of the
>> `java.security.manager` system property to "disallow", and necessary test
>> change following this update.
>> 2.
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>> Manual changes to several files so that the next commit can be generated
>> programatically.
>> 3.
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>> Automatic changes to other source files to avoid javac warnings on
>> deprecation for removal
>>
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
>> generated programmatically, see the comment below for more details. If you
>> are only interested in a portion of the 3rd commit and would like to review
>> it as a separate file, please comment here and I'll generate an individual
>> webrev.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> any file to minimize unnecessary merge conflict.
>>
>> Furthermore, since the default value of `java.security.manager` system
>> property is now "disallow", most of the tests calling
>> `System.setSecurityManager()` need to launched with
>> `-Djava.security.manager=allow`. This is covered in a different PR at
>> https://github.com/openjdk/jdk/pull/4071.
>>
>> Update: the deprecation annotations and javadoc tags, build, compiler,
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {
Who or what prompted this change ?
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8263140: Japanese chars garble in console window in HSDB [v3]
On Tue, 9 Mar 2021 00:54:25 GMT, Yasumasa Suenaga wrote: >> We can command line debugger via "Windows" -> "Console" menu on HSDB. If the >> command shows error message in localized string (e.g. Japanese), it might >> garble as following: >> >>  >> >> Command line debugger and Debugger Console (WinDbg on Windows) will use >> Courier font on their console, but it does not show Japanese chars. I guess >> it would happen on CJK chars because monospaced font for Chinese, Japanese, >> Korean are different from Courier. >> >> After this change on Windows which set to Japanese locale, it uses MS Gothic >> and we can see localized message as following: >> >>  > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Get rid of using Courier font Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2862
Re: RFR: 8263140: Japanese chars garble in console window in HSDB [v2]
On Mon, 8 Mar 2021 19:51:40 GMT, Chris Plummer wrote:
> There are two additional locations that use `lookupFont("Courier")`. Any
> reason not to replace them also? If you do replace them, then I think you can
> get rid of `lookupFont`.
I think you really, really should get rid that ! Courier is a Postscript font
that you will be hard pressed to find on modern systems. On Windows we might
still have the "special case" code that remaps it to Courier New. Linux
probably doesn't have it because it is a proprietary name which is why JDK used
it only in 1.0 and stopped in 1.1 some 25 (!) years ago. And using a logical
font you will get much better code point support
-
PR: https://git.openjdk.java.net/jdk/pull/2862
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v18]
On Wed, 17 Feb 2021 12:36:10 GMT, Anton Kozlov wrote: >> Please review the implementation of JEP 391: macOS/AArch64 Port. >> >> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and >> windows/aarch64. >> >> Major changes are in: >> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks >> JDK-8253817, JDK-8253818) >> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with >> necessary adjustments (JDK-8253819) >> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), >> required on macOS/AArch64 platform. It's implemented with >> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a >> thread, so W^X mode change relates to the java thread state change (for java >> threads). In most cases, JVM executes in write-only mode, except when >> calling a generated stub like SafeFetch, which requires a temporary switch >> to execute-only mode. The same execute-only mode is enabled when a java >> thread executes in java or native states. This approach of managing W^X mode >> turned out to be simple and efficient enough. >> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) > > Anton Kozlov has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 88 commits: > > - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos > - Re-do safefetch.hpp > - Merge remote-tracking branch 'origin/jdk/8261075-stubroutines-inline' into > jdk-macos > - stubRoutines.inline.hpp -> safefetch.hpp > - Update copyrights > - Merge remote-tracking branch 'upstream/jdk/master' into > 8261075-stubroutines-inline > - Merge remote-tracking branch 'upstream/jdk/master' into > 8261075-stubroutines-inline > - Extract SafeFetch32/N to stubRoutines.inline.hpp > - Revert "Extract SafeFetch32/N to stubRoutines.inline.hpp" > >This reverts commit b873c25f31dd21349d140b790713cc9ccb5f2dc0. > - Merge pull request #9 from VladimirKempik/pull/2200 > >Removed unused variables > - ... and 78 more: > https://git.openjdk.java.net/jdk/compare/b955f85e...ab72613c Looks like the compiler warning changess are now the only desktop changes. That is fine by me. - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2200
Integrated: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Thu, 28 Jan 2021 22:40:57 GMT, Phil Race wrote: > This removes the JNF dependency from the jdk.hotspot.agent module. > The macro expansions are the same as already used in the Java desktop module > - which actually uses a macro > still but there there are hundreds of uses. > The function of this macro code is to prevent NSExceptions escaping to Java > and also to drain the auto-release pool. > What test group would be good for verifying this change ? This pull request has now been integrated. Changeset: 2be60e37 Author:Phil Race URL: https://git.openjdk.java.net/jdk/commit/2be60e37 Stats: 45 lines in 2 files changed: 35 ins; 2 del; 8 mod 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m Reviewed-by: ihse, cjplummer - PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m [v2]
> This removes the JNF dependency from the jdk.hotspot.agent module. > The macro expansions are the same as already used in the Java desktop module > - which actually uses a macro > still but there there are hundreds of uses. > The function of this macro code is to prevent NSExceptions escaping to Java > and also to drain the auto-release pool. > What test group would be good for verifying this change ? Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m - Changes: - all: https://git.openjdk.java.net/jdk/pull/2304/files - new: https://git.openjdk.java.net/jdk/pull/2304/files/9919a02c..d7ed0158 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2304=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2304=00-01 Stats: 39 lines in 1 file changed: 18 ins; 15 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2304/head:pull/2304 PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m [v2]
On Tue, 2 Feb 2021 20:27:17 GMT, Chris Plummer wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
>
> src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 294:
>
>> 292:
>> 293: NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
>> 294: @try {
>
> Although there are only 3 places where the `JNF_COCOA_ENTER/EXIT` macros are
> used, it seems it would still be worth taking the same approach you did for
> `java.desktop` and add the replacement macros instead of inlining them. So
> just copy what you added to
> `src/java.desktop/macosx/native/libosxapp/JNIUtilities.h`, and replace
> `JNF_COCOA_ENTER` with `JNI_COCOA_ENTER/EXIT`. Otherwise the
> `JNF_COCOA_ENTER/EXIT` changes look fine to me, but I'm just basing this on a
> comparison with what you've done with `java.desktop`. I'm no expert in this
> area.
OK .. I don't really mind either way and if this helps gets it pushed .. so
I've updated.
> src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 296:
>
>> 294: @try {
>> 295:
>> 296: NSString *symbolNameString = JavaStringToNSString(env, symbolName);
>
> Is there a reason why `java.desktop` continues to use `JNFJavaToNSString`? I
> was looking to see how this was handled in other places, but I couldn't find
> an example where `JNFJavaToNSString` was converted to call a newly
> implemented `JavaStringToNSString`.
As Magnus said that is in progress. Hoping it will be pushed very soon.
-
PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Fri, 29 Jan 2021 17:22:49 GMT, Phil Race wrote: >> Build changes look good. > >> Is it because they are not in a place that can be accessed from this file? > Right 99% of JNF usage was the desktop module many, many files, 1300 > references .. the entire rest of the JDK had just 3 files each in a different > module ! It did not seem worth creating a JDK-wide platform-specific library > to support this. > For testing you want test/jdk/sun/tools/jhsdb/ and > test/hotspot/jtreg/serviceability/sa These tests passed with my changes - PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Fri, 29 Jan 2021 10:01:42 GMT, Magnus Ihse Bursie wrote: >> This removes the JNF dependency from the jdk.hotspot.agent module. >> The macro expansions are the same as already used in the Java desktop module >> - which actually uses a macro >> still but there there are hundreds of uses. >> The function of this macro code is to prevent NSExceptions escaping to Java >> and also to drain the auto-release pool. >> What test group would be good for verifying this change ? > > Build changes look good. > Is it because they are not in a place that can be accessed from this file? Right 99% of JNF usage was the desktop module many, many files, 1300 references .. the entire rest of the JDK had just 3 files each in a different module ! It did not seem worth creating a JDK-wide platform-specific library to support this. - PR: https://git.openjdk.java.net/jdk/pull/2304
RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
This removes the JNF dependency from the jdk.hotspot.agent module. The macro expansions are the same as already used in the Java desktop module - which actually uses a macro still but there there are hundreds of uses. The function of this macro code is to prevent NSExceptions escaping to Java and also to drain the auto-release pool. What test group would be good for verifying this change ? - Commit messages: - 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m - 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m Changes: https://git.openjdk.java.net/jdk/pull/2304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2304=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257988 Stats: 42 lines in 2 files changed: 32 ins; 2 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2304/head:pull/2304 PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 23:34:04 GMT, Phil Race wrote: >> that sounds good to me, I am just afraid to break intel mac on older macos >> versions with this change. > > That may actually be a valid concern. Both say macOS 10.12+ ... which might > conflict with the 10.9 target. Maybe you should just file a bug after all for this to be dealt with separately. Certainly if it is NOT fixed now such a bug needs to be filed. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 22:47:33 GMT, Vladimir Kempik wrote: >> 1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask >> 2) So maybe rather than the deprecation suppression you could change both >> constants to the new ones. >> Ordinarily I'd say let someone else do that but this looks like a simple >> obvious change and is dwarfed by all the other changes going on for Mac ARM >> ... > > that sounds good to me, I am just afraid to break intel mac on older macos > versions with this change. That may actually be a valid concern. Both say macOS 10.12+ ... which might conflict with the 10.9 target. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 22:25:48 GMT, Vladimir Kempik wrote: >> Are you doing something somewhere to change the target version of macOS or >> SDK ? I had no such problem. >> I think we currently target a macOS 10.9 and if you are changing that it >> would need discussion. >> If you are changing it only for Mac ARM that may make more sense .. >> >> And these appear to be just API churn by Apple >> NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst >> >> https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc >> >> NSBorderlessWindowMask is replaced by NSWindowStyleMask >> >> https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ > > Min_macos version is changed to 11.0 for macos_aarch64 > > https://github.com/openjdk/jdk/pull/2200/files/0c2cb0a372bf1a8607810d773b53d6959616a816#diff-7cd97cdbeb3053597e5d6659016cdf0f60b2c412bd39934a43817ee0b717b7a7R136 1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask 2) So maybe rather than the deprecation suppression you could change both constants to the new ones. Ordinarily I'd say let someone else do that but this looks like a simple obvious change and is dwarfed by all the other changes going on for Mac ARM ... - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 21:18:59 GMT, Vladimir Kempik wrote: >> Hello >> I believe it was a workaround for issues with xcode 12.2 in early beta days. >> Those issues were fixed later in upstream jdk, so most likely we need to >> remove these workarounds. > > It seems these workarounds are still needed: > > jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:300:39: > error: 'NSAlphaFirstBitmapFormat' is deprecated: first deprecated in macOS > 10.14 [-Werror,-Wdeprecated-declarations] > bitmapFormat: NSAlphaFirstBitmapFormat | > NSAlphaNonpremultipliedBitmapFormat > ^~~~ > NSBitmapFormatAlphaFirst > > jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:438:34: > error: 'NSBorderlessWindowMask' is deprecated: first deprecated in macOS > 10.12 [-Werror,-Wdeprecated-declarations] > styleMask: NSBorderlessWindowMask > ^~ > NSWindowStyleMaskBorderless Are you doing something somewhere to change the target version of macOS or SDK ? I had no such problem. I think we currently target a macOS 10.9 and if you are changing that it would need discussion. If you are changing it only for Mac ARM that may make more sense .. And these appear to be just API churn by Apple NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc NSBorderlessWindowMask is replaced by NSWindowStyleMask https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 19:38:16 GMT, Anton Kozlov wrote: >> Please review the implementation of JEP 391: macOS/AArch64 Port. >> >> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and >> windows/aarch64. >> >> Major changes are in: >> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks >> JDK-8253817, JDK-8253818) >> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with >> necessary adjustments (JDK-8253819) >> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), >> required on macOS/AArch64 platform. It's implemented with >> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a >> thread, so W^X mode change relates to the java thread state change (for java >> threads). In most cases, JVM executes in write-only mode, except when >> calling a generated stub like SafeFetch, which requires a temporary switch >> to execute-only mode. The same execute-only mode is enabled when a java >> thread executes in java or native states. This approach of managing W^X mode >> turned out to be simple and efficient enough. >> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) > > Anton Kozlov has updated the pull request incrementally with two additional > commits since the last revision: > > - Refactor CDS disabling > - Redo builsys support for aarch64-darwin make/modules/java.desktop/lib/Awt2dLibraries.gmk line 573: > 571: EXTRA_HEADER_DIRS := $(LIBFONTMANAGER_EXTRA_HEADER_DIRS), \ > 572: WARNINGS_AS_ERRORS_xlc := false, \ > 573: DISABLED_WARNINGS_clang := deprecated-declarations, \ I see this added here and in another location. What is deprecated ? You really need to explain these sorts of things. I've built JDK using Xcode 12.3 and not had any need for this. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Sun, 24 Jan 2021 15:32:59 GMT, Anton Kozlov wrote: >> Please review the implementation of JEP 391: macOS/AArch64 Port. >> >> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and >> windows/aarch64. >> >> Major changes are in: >> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks >> JDK-8253817, JDK-8253818) >> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with >> necessary adjustments (JDK-8253819) >> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), >> required on macOS/AArch64 platform. It's implemented with >> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a >> thread, so W^X mode change relates to the java thread state change (for java >> threads). In most cases, JVM executes in write-only mode, except when >> calling a generated stub like SafeFetch, which requires a temporary switch >> to execute-only mode. The same execute-only mode is enabled when a java >> thread executes in java or native states. This approach of managing W^X mode >> turned out to be simple and efficient enough. >> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) > > Anton Kozlov has updated the pull request incrementally with two additional > commits since the last revision: > > - Address feedback for signature generators > - Enable -Wformat-nonliteral back Changes requested by prr (Reviewer). src/java.desktop/share/native/libharfbuzz/hb-common.h line 113: > 111: > 112: #define HB_TAG(c1,c2,c3,c4) > ((hb_tag_t)uint32_t)(c1)&0xFF)<<24)|(((uint32_t)(c2)&0xFF)<<16)|(((uint32_t)(c3)&0xFF)<<8)|((uint32_t)(c4)&0xFF))) > 113: #define HB_UNTAG(tag) (char)(((tag)>>24)&0xFF), > (char)(((tag)>>16)&0xFF), (char)(((tag)>>8)&0xFF), (char)((tag)&0xFF) I need a robust explanation of these changes. They are not mentioned, nor are they in upstream harfbuzz. Likely these should be pushed to upstream first if there is a reason for them. src/java.desktop/share/native/libharfbuzz/hb-coretext.cc line 193: > 191:* crbug.com/549610 */ > 192: // 0x0007 stands for "kCTVersionNumber10_10", see CoreText.h > 193: #if TARGET_OS_OSX && MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_10 I need a robust explanation of these changes. They are not mentioned, nor are they in upstream harfbuzz. Likely these should be pushed to upstream first if there is a reason for them. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: [jdk16] RFR: 8258827: ProblemList Naming/DefaultRegistryPort.java and Naming/legalRegistryNames/LegalRegistryNames.java on Windows
On Tue, 22 Dec 2020 16:56:33 GMT, Daniel D. Daugherty wrote: > ProblemList two java/rmi/Naming tests on Windows in order to reduce the > noise in the JDK16 CI. This is a trivial fix. overdue - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.java.net/jdk16/pull/58
Re: RFR: 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo
On Sun, 20 Dec 2020 20:22:48 GMT, Andrey Turbanov wrote: >> jrtfs is compiled twice, the second is to --release 8 so it can be packaged >> into jrt-fs.jar for use by IDEs/tools running on older JDK releases. So need >> to be careful with the changes here as it will likely causing build breakage. >> >> We try to keep the changes to ASM to a minimum, might be better to leave >> this change out of the patch. >> >> One or two of the sources changes should probably uses Files.copy, e.g. >> ZipPath, sjavac/CopyFile.java. > >> One or two of the sources changes should probably uses Files.copy, e.g. >> ZipPath, sjavac/CopyFile.java. > > Good idea! Replaced in few places. But not in ZipPath: it's actually > implementation of underlying call from Files.copy: > `jdk.nio.zipfs.ZipFileSystemProvider#copy`. So, `Files.copy` call will be > recursive. So these changes are all over the place which means no one person knows how to test all of it. Have you run the sound, swing tests, and the printing tests on unix and windows ? For printing for sure you'll need to do some manual testing. - PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8066622 8066637: remove deprecated using java.io.File.toURL() in internal classes
On Sat, 7 Nov 2020 07:55:03 GMT, Sebastian Ritter wrote: > In result of Javadoc to do not use java.io.File.toURL() > Change use java.io.File.toURL().toURI() instead. You reference a desktop bug that discusses many, many deprecations and skara has identified https://bugs.openjdk.java.net/browse/JDK-8066622 as the issue being fixed. Yet you propose to fix precisely one of these. But when this is integrated that bug will be closed out as resolved. I think you need a new bug about JUST the changes you are making. So I don't think you should use that bug id any where in this PR. And I'd like to hear whether you actually *tested* splashscreen with your change ? I see no sign that you did. - Changes requested by prr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1108
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Sun, 6 Sep 2020 13:57:19 GMT, Dmitriy Dumanskiy wrote: > **isEmpty** is faster + has less byte code + easier to read. Benchmarks could > be found > > [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416). 1) This is un-necessary churn. 2) I can't even be sure I am finding the ones in my area because there's so much here 3) The ones I can find have no need of whatever performance improvement this might bring. I think this whole PR should be withdrawn, and there may an attempt at measuring the benefits of this for the various cases before submitting in appropriate smaller chunks. But I'll tell you now I don't see a need for the test updates you are making. - Changes requested by prr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: JDK-8200178 Remove mapfiles for JDK native libraries
http://cr.openjdk.java.net/~ihse/JDK-8200178-remove-mapfiles/webrev.01/src/java.desktop/share/native/libmlib_image/mlib_image_proto.h.udiff.html
The variable definitions here are now misaligned.
..and added 2d-dev since many of these native changes are in 2d.
-phil.
On 03/23/2018 10:33 AM, Phil Race wrote:
There are a lot of changes in the desktop libraries.
Doing mach5 tier1/2/3 testing is not nearly sufficient to cover those
since
only tier3 has any UI tests and it barely uses anything that's touched
here.
So since testing seems to be wise, then I think you should do a
jtreg desktop group run on Linux & Windows.
You can probably skip Mac since it is unaffected and I think Linux
will cover Solaris here.
You should also do some headless testing.
It could take some time to review this properly and decide what
changes are OK,
what changes are something we should clean up later, and what changes
are something
that ought to be addressed now ..
I think I'd be mainly concerned that something fails due to a missing
symbol, or
that for newly exported symbols if we ended up with duplicate symbols
as a result.
The results of a test run will add confidence here.
BTW I don't think you are right that
java.desktop:/libawt_headless: The following symbols are now also
exported due to JNIEXPORT (they were previously
..
X11SurfaceData_GetOps
It looks to me like it was previously exported.
-phil.
On 03/23/2018 06:56 AM, Magnus Ihse Bursie wrote:
With modern compilers, we can use compiler directives (such as
_attribute__((visibility("default"))), or __declspec(dllexport)) to
control symbol visibility, directly in the source code. This has
historically not been present on all compilers, so we had to resort
to using mapfiles (also known as linker scripts).
This is no longer the case. Now all compilers we use support symbol
visibility directives, in one form or another. We should start using
this. Since this has been the only way to control symbol visibility
on Windows, for most of the shared code, we already have proper
JNIEXPORT decorations in place.
If we fix the remaining platform-specific files to have proper
JNIEXPORT tagging, then we can finally get rid of mapfiles.
This fix removed mapfiles for all JDK libraries. It does not touch
hotspot libraries nor JDK executables; they will have to wait for a
future fix -- this was complex enough. This change will not have any
impact on macosx, since we do not use mapfiles there, but instead
export all symbols. (This is not a good idea, but I'll address that
separately.) This change will also have a minimal impact on Windows.
The only reason Windows is impacted at all, is that some changes
needed by Solaris and Linux were simpler to fix for all platforms.
I have strived for this change to have no impact on the actual
generated code. Unfortunately, this was not possible to fully
achieve. I do not believe that these changes will have any actual
impact on the product, though. I will present the differences more in
detail further down. Those who are not interested can probably skip
that.
The patch has passed tier1 testing and is currently running tier2 and
tier3. Since the running code is more or less (see caveat below)
unmodified, I don't expect any testing issues.
Bug: https://bugs.openjdk.java.net/browse/JDK-8200178
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8200178-remove-mapfiles/webrev.01
Details on changes:
Most of the source code changes are (unsurprisingly) in java.base and
java.desktop. Remaining changes are in jdk.crypto.ucrypto,
jdk.hotspot.agent, jdk.jdi and jdk.jdwp.agent.
Source code changes does almost to 100% consists in decorating an
exported function with JNIEXPORT. I have also followed the
long-standing convention of adding JNICALL. This is a no-op on
non-Windows platforms, so for most of the changes this is purely
cosmetic (and possibly adding in robustness, should the function ever
be used on Windows in the future). I have also followed the stylistic
convention of putting "JNIEXPORT JNICALL" on a separate
line. For some functions, however, this might cause a change in
calling convention on Windows. Since this can not apply to exported
functions on Windows (otherwise they would already have had
JNIEXPORT), I do not think this matters anything.
A few libraries did not have a mapfile, on Linux and/or Solaris. This
actually meant that all symbols were exported. It is highly unclear
if this was known and intended by the original make rule writer. I
have emulated this by adding the flag $(EXPORT_ALL_SYMBOLS) to these
libraries. Hopefully, we can remove this flag and fix proper exported
symbols in the future.
I have run the complete build using COMPARE_BUILD, and made a
thourough analysis of the differences for Linux and Solaris. All
native libraries have symbol differences, but most of them are
trivial and/or harmless. As a result, most libraries
Re: RFR: JDK-8200178 Remove mapfiles for JDK native libraries
There are a lot of changes in the desktop libraries.
Doing mach5 tier1/2/3 testing is not nearly sufficient to cover those since
only tier3 has any UI tests and it barely uses anything that's touched here.
So since testing seems to be wise, then I think you should do a
jtreg desktop group run on Linux & Windows.
You can probably skip Mac since it is unaffected and I think Linux will
cover Solaris here.
You should also do some headless testing.
It could take some time to review this properly and decide what changes
are OK,
what changes are something we should clean up later, and what changes
are something
that ought to be addressed now ..
I think I'd be mainly concerned that something fails due to a missing
symbol, or
that for newly exported symbols if we ended up with duplicate symbols as
a result.
The results of a test run will add confidence here.
BTW I don't think you are right that
java.desktop:/libawt_headless: The following symbols are now also
exported due to JNIEXPORT (they were previously
..
X11SurfaceData_GetOps
It looks to me like it was previously exported.
-phil.
On 03/23/2018 06:56 AM, Magnus Ihse Bursie wrote:
With modern compilers, we can use compiler directives (such as
_attribute__((visibility("default"))), or __declspec(dllexport)) to
control symbol visibility, directly in the source code. This has
historically not been present on all compilers, so we had to resort to
using mapfiles (also known as linker scripts).
This is no longer the case. Now all compilers we use support symbol
visibility directives, in one form or another. We should start using
this. Since this has been the only way to control symbol visibility on
Windows, for most of the shared code, we already have proper JNIEXPORT
decorations in place.
If we fix the remaining platform-specific files to have proper
JNIEXPORT tagging, then we can finally get rid of mapfiles.
This fix removed mapfiles for all JDK libraries. It does not touch
hotspot libraries nor JDK executables; they will have to wait for a
future fix -- this was complex enough. This change will not have any
impact on macosx, since we do not use mapfiles there, but instead
export all symbols. (This is not a good idea, but I'll address that
separately.) This change will also have a minimal impact on Windows.
The only reason Windows is impacted at all, is that some changes
needed by Solaris and Linux were simpler to fix for all platforms.
I have strived for this change to have no impact on the actual
generated code. Unfortunately, this was not possible to fully achieve.
I do not believe that these changes will have any actual impact on the
product, though. I will present the differences more in detail further
down. Those who are not interested can probably skip that.
The patch has passed tier1 testing and is currently running tier2 and
tier3. Since the running code is more or less (see caveat below)
unmodified, I don't expect any testing issues.
Bug: https://bugs.openjdk.java.net/browse/JDK-8200178
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8200178-remove-mapfiles/webrev.01
Details on changes:
Most of the source code changes are (unsurprisingly) in java.base and
java.desktop. Remaining changes are in jdk.crypto.ucrypto,
jdk.hotspot.agent, jdk.jdi and jdk.jdwp.agent.
Source code changes does almost to 100% consists in decorating an
exported function with JNIEXPORT. I have also followed the
long-standing convention of adding JNICALL. This is a no-op on
non-Windows platforms, so for most of the changes this is purely
cosmetic (and possibly adding in robustness, should the function ever
be used on Windows in the future). I have also followed the stylistic
convention of putting "JNIEXPORT JNICALL" on a separate
line. For some functions, however, this might cause a change in
calling convention on Windows. Since this can not apply to exported
functions on Windows (otherwise they would already have had
JNIEXPORT), I do not think this matters anything.
A few libraries did not have a mapfile, on Linux and/or Solaris. This
actually meant that all symbols were exported. It is highly unclear if
this was known and intended by the original make rule writer. I have
emulated this by adding the flag $(EXPORT_ALL_SYMBOLS) to these
libraries. Hopefully, we can remove this flag and fix proper exported
symbols in the future.
I have run the complete build using COMPARE_BUILD, and made a
thourough analysis of the differences for Linux and Solaris. All
native libraries have symbol differences, but most of them are trivial
and/or harmless. As a result, most libraries have disasm differences
as well, but these too seem trivial and harmless. The differences in
symbols that are common to all libraries include:
* Internal symbols such as __bss_start, _edata, _end and _fini are
now global. (They are imported as such from the compiler
libraries/archives, and we have no linker script to override this
Fix for 8030115 [parfait] warnings .. broke the 9-dev build.
We have just filed a P1 bug and are testing a fix. -phil.
Re: Fix for 8030115 [parfait] warnings .. broke the 9-dev build.
https://bugs.openjdk.java.net/browse/JDK-8053902 -phil. On 7/29/2014 11:25 AM, Phil Race wrote: We have just filed a P1 bug and are testing a fix. -phil.
Re: AWT Dev RFR(L) - 2nd round: 8024854: Basic changes and files to build the class library on AIX
Looking only at what you needed to change this time round, all seems fine now. -phil. On 11/26/13 8:23 AM, Volker Simonis wrote: Hi, thanks to everybody for the prompt and helpful reviews. Here comes the final webrev which incorporates all the corrections and suggestions from the second review round: http://cr.openjdk.java.net/~simonis/webrevs/8024854.v3/ http://cr.openjdk.java.net/%7Esimonis/webrevs/8024854.v3/ I've successfully build (and run some smoke tests) with these changes on Linux (x86_32, x86_64, ppc64), Solaris/sparcv9, Windows/x86_64, MacOSX and AIX (5.3, 7.1). So if nobody vetoes these changes are ready for push into our staging repository. @Vladimir: can I push them or do you want to run them trough JPRT? Thank you and best regards, Volker PS: compared to the last webrev (http://cr.openjdk.java.net/~simonis/webrevs/8024854.v2/ http://cr.openjdk.java.net/%7Esimonis/webrevs/8024854.v2/), I've slightly changed the following files: makefiles/lib/Awt2dLibraries.gmk makefiles/lib/NetworkingLibraries.gmk makefiles/Setup.gmk src/aix/classes/sun/awt/fontconfigs/aix.fontconfig.properties src/aix/classes/sun/nio/ch/AixPollPort.java src/aix/classes/sun/nio/fs/AixFileSystem.java src/aix/native/java/net/aix_close.c src/aix/porting/porting_aix.c src/share/native/java/net/net_util.c src/share/native/java/net/net_util.h src/share/native/sun/awt/medialib/mlib_sys.c src/solaris/bin/java_md_solinux.c src/solaris/classes/sun/nio/ch/Port.java src/solaris/native/java/net/net_util_md.c src/solaris/native/sun/java2d/x11/XRBackendNative.c src/solaris/native/sun/management/OperatingSystemImpl.c src/solaris/native/sun/nio/fs/UnixNativeDispatcher.c src/windows/native/java/net/net_util_md.c Most of the changes are cosmetic, except the ones in: src/share/native/java/net/net_util.c src/share/native/java/net/net_util.h src/solaris/native/java/net/net_util_md.c src/windows/native/java/net/net_util_md.c where I renamed 'initLocalAddrTable()' to 'platformInit()'. For AIX, this method will now call 'aix_close_init()' as suggested by Alan. The changes to src/solaris/native/com/sun/ management/UnixOperatingSystem_md.c are now in src/solaris/native/sun/management/OperatingSystemImpl.c because that file was moved by an upstream change. On Wed, Nov 20, 2013 at 7:26 PM, Volker Simonis volker.simo...@gmail.com mailto:volker.simo...@gmail.com wrote: Hi, this is the second review round for 8024854: Basic changes and files to build the class library on AIX https://bugs.openjdk.java.net/browse/JDK-8024854. The previous reviews can be found at the end of this mail in the references section. I've tried to address all the comments and suggestions from the first round and to further streamline the patch (it perfectly builds on Linux/x86_64, Linux/PPC664, AIX, Solaris/SPARC and Windows/x86_64). The biggest change compared to the first review round is the new aix/ subdirectory which I've now created under jdk/src and which contains AIX-only code. The webrev is against our http://hg.openjdk.java.net/ppc-aix-port/stage repository which has been recently synchronised with the jdk8 master: http://cr.openjdk.java.net/~simonis/webrevs/8024854.v2/ http://cr.openjdk.java.net/%7Esimonis/webrevs/8024854.v2/ Below (and in the webrev) you can find some comments which explain the changes to each file. In order to be able to push this big change, I need the approval of reviewers from the lib, net, svc, 2d, awt and sec groups. So please don't hesitate to jump at it - there are enough changes for everybody:) Thank you and best regards, Volker *References:* The following reviews have been posted so far (thanks Iris for collecting them :) - Alan Bateman (lib): Initial comments (16 Sep [2]) - Chris Hegarty (lib/net): Initial comments (20 Sep [3]) - Michael McMahon (net): Initial comments (20 Sept [4]) - Steffan Larsen (svc): APPROVED (20 Sept [5]) - Phil Race (2d): Initial comments (18 Sept [6]); Additional comments (15 Oct [7]) - Sean Mullan (sec): Initial comments (26 Sept [8]) [2]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001045.html [3]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001078.html [4]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001079.html [5]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001077.html [6]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001069.html [7]: http://mail.openjdk.java.net/pipermail/2d-dev/2013-October/003826.html [8]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001081.html *Detailed change description:* The new jdk/src/aix subdirectory contains the following new and AIX-specific files for now
Re: [OpenJDK 2D-Dev] RFR(L) - 2nd round: 8024854: Basic changes and files to build the class library on AIX
Hi,
I see you've already received a ton of good feedback on this v2.
I have just a few things to add.
I don't know what symlinks might exist on AIX but it seems odd to
me that you have :-
138 static char *fullAixFontPath[] = {
139 /usr/lpp/X11/lib/X11/fonts/Type1,
..
but the paths in the new file aix.fontconfig.properties look like this :-
/usr/lib/X11/fonts/Type1/cour.pfa
Also for the build to pick up a file called
*src/aix/classes/sun/awt/fontconfigs/aix.fontconfig.properties
*it seems like you should have added a section to handle this path to
jdk/makefiles/gendata/GenDataFontConfig.gmk
That seems to be where the new build handles such files.
Are you seeing the .bfc and .src files created ?
At runtime it'll get picked up so so long as System.getProperty(os.name)
returns aix (all lower-case) so I think that's OK. Its the build time part
I'd expect to see but don't.
-phil.
On 11/20/2013 10:26 AM, Volker Simonis wrote:
Hi,
this is the second review round for 8024854: Basic changes and files
to build the class library on AIX
https://bugs.openjdk.java.net/browse/JDK-8024854. The previous
reviews can be found at the end of this mail in the references section.
I've tried to address all the comments and suggestions from the first
round and to further streamline the patch (it perfectly builds on
Linux/x86_64, Linux/PPC664, AIX, Solaris/SPARC and Windows/x86_64).
The biggest change compared to the first review round is the new
aix/ subdirectory which I've now created under jdk/src and which
contains AIX-only code.
The webrev is against our
http://hg.openjdk.java.net/ppc-aix-port/stage repository which has
been recently synchronised with the jdk8 master:
http://cr.openjdk.java.net/~simonis/webrevs/8024854.v2/
http://cr.openjdk.java.net/%7Esimonis/webrevs/8024854.v2/
Below (and in the webrev) you can find some comments which explain the
changes to each file. In order to be able to push this big change, I
need the approval of reviewers from the lib, net, svc, 2d, awt and sec
groups. So please don't hesitate to jump at it - there are enough
changes for everybody:)
Thank you and best regards,
Volker
*References:*
The following reviews have been posted so far (thanks Iris for
collecting them :)
- Alan Bateman (lib): Initial comments (16 Sep [2])
- Chris Hegarty (lib/net): Initial comments (20 Sep [3])
- Michael McMahon (net): Initial comments (20 Sept [4])
- Steffan Larsen (svc): APPROVED (20 Sept [5])
- Phil Race (2d): Initial comments (18 Sept [6]); Additional comments
(15 Oct [7])
- Sean Mullan (sec): Initial comments (26 Sept [8])
[2]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001045.html
[3]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001078.html
[4]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001079.html
[5]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001077.html
[6]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001069.html
[7]:
http://mail.openjdk.java.net/pipermail/2d-dev/2013-October/003826.html
[8]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001081.html
*Detailed change description:*
The new jdk/src/aix subdirectory contains the following new and
AIX-specific files for now:
src/aix/classes/sun/awt/fontconfigs/aix.fontconfig.properties
src/aix/classes/sun/nio/ch/AixAsynchronousChannelProvider.java
src/aix/classes/sun/nio/ch/AixPollPort.java
src/aix/classes/sun/nio/fs/AixFileStore.java
src/aix/classes/sun/nio/fs/AixFileSystem.java
src/aix/classes/sun/nio/fs/AixFileSystemProvider.java
src/aix/classes/sun/nio/fs/AixNativeDispatcher.java
src/aix/classes/sun/tools/attach/AixAttachProvider.java
src/aix/classes/sun/tools/attach/AixVirtualMachine.java
src/aix/native/java/net/aix_close.c
src/aix/native/sun/nio/ch/AixPollPort.c
src/aix/native/sun/nio/fs/AixNativeDispatcher.c
src/aix/native/sun/tools/attach/AixVirtualMachine.c
src/aix/porting/porting_aix.c
src/aix/porting/porting_aix.h
src/aix/porting/porting_aix.c
src/aix/porting/porting_aix.h
* Added these two files for AIX relevant utility code.
* Currently these files only contain an implementation of |dladdr|
which is not available on AIX.
* In the first review round the existing |dladdr| users in the code
either called the version from the HotSpot on AIX
(|src/solaris/native/sun/awt/awt_LoadLibrary.c|) or had a private
copy of the whole implementation
(|src/solaris/demo/jvmti/hprof/hprof_md.c|). This is now not
necessary any more.
The new file layout required some small changes to the makefiles to
make them aware of the new directory locations:
makefiles/CompileDemos.gmk
* Add an extra argument to |SetupJVMTIDemo| which can be used to
pass additional source locations.
* Currently this is only used on AIX for the AIX porting utilities
Re: [OpenJDK 2D-Dev] RFR(L): 8024854: Basic changes and files to build the class library on AIX
Hello Volker,
I just remembered got back to these changes - my email inbox isn't a
very good reminder system.
I am OK with everything I see in the client areas except as below :-
/* Very basic start for AIX - feel free to complete ..
169 */
We should remove this comment. If that means first getting
any necessary completion from IBM folks, please ask them.
Given the lack of fontconfig on AIX as standard its probably
more important for AIX than any other OS so I suggest the latter.
* insted it has to be installed from the AIX Toolbox for Linux Applications
insted - instead
X11SurfaceData.c:
#ifndef MAX
and
#ifndef MIN
I found that re-definition warnings are seen at least on
64 bit linux. Maybe you should rename these too generically
named macros to XSD_MAX and XSD_MIN.
extern int dladdr(void *addr, Dl_info *info); // use the HotSpot
implementation from libjvm.so
Did you ever get an opinion on this from the libraries or hotspot teams ?
-phil.
On 9/19/2013 7:28 AM, Volker Simonis wrote:
Hi Phil,
I'm open to any good ideas.
The current solution is pragmatic, simple and it has no impact on
existing platforms.
The other possibility I see is to put the 'dladdr' implantation into
its own library (something equivalent to libjli) and link that
library to libawt. But I currently don't see any existing library I
could put the implementation into.
Notice that the change I propose already contains an extra
implementation of 'dladdr' for AIX in
src/solaris/demo/jvmti/hprof/hprof_md.c because libhprof is not
linked against libjvm and we therefore can not use the 'extern'-trick
there.
Of course it would be nice if there would be a small library which
contains 'dladdr' and which could be linked against both, libawt and
libhprof. But I think that would require bigger shared changes (with
possible empty implementations on the current platforms).
What do other think?
Regards,
Volker
On Thu, Sep 19, 2013 at 3:53 PM, Phil Race philip.r...@oracle.com wrote:
Hi,
w.r.t the one issue below : is the awt load library code the only place you
need/are doing this ? If someone else (hotspot, core-libs) already assented
to this
then I guess I could too but I'd like to hear for sure that hotspot and
core-libs
teams both agree to this approach and whether there is an alternative.
-phil.
On 9/19/13 4:29 AM, Volker Simonis wrote:
Hi Phil,
thank you for looking at the changes. Please find my answers inline:
/* AIX does not provide the 'dladdr' function. But fortunately, we've
42 * already implemented it in the HotSpot, because we need it there as
43 * well (see hotspot/src/os/aix/vm/porting_aix.{hpp,cpp}).
Whilst this is in ifdef AIX this reliance on an exported hotspot
function sounds hacky. What actual requirement is there that the
AIX class libraries be so tightly-coupled with that VM?
There is no contract there.
You're right, there is no contract. It's just pragmatic solution and I think
it should always work because the libjvm will always be loaded at the point
in AWT where it is used. Another solution would be to re-implement the
functionality in the class library and I don't like code duplication either.
-phil.
Re: [OpenJDK 2D-Dev] RFR(L): 8024854: Basic changes and files to build the class library on AIX
Hi,
w.r.t the one issue below : is the awt load library code the only place you
need/are doing this ? If someone else (hotspot, core-libs) already
assented to this
then I guess I could too but I'd like to hear for sure that hotspot and
core-libs
teams both agree to this approach and whether there is an alternative.
-phil.
On 9/19/13 4:29 AM, Volker Simonis wrote:
Hi Phil,
thank you for looking at the changes. Please find my answers inline:
/* AIX does not provide the 'dladdr' function. But fortunately, we've
42 * already implemented it in the HotSpot, because we need it
there as
43 * well (see hotspot/src/os/aix/vm/porting_aix.{hpp,cpp}).
Whilst this is in ifdef AIX this reliance on an exported hotspot
function sounds hacky. What actual requirement is there that the
AIX class libraries be so tightly-coupled with that VM?
There is no contract there.
You're right, there is no contract. It's just pragmatic solution and I
think it should always work because the libjvm will always be loaded
at the point in AWT where it is used. Another solution would be to
re-implement the functionality in the class library and I don't like
code duplication either.
-phil.
Re: [OpenJDK 2D-Dev] RFR(L): 8024854: Basic changes and files to build the class library on AIX
Volker,
I've skimmed the client related changes - mostly based on the descriptions.
I'll look in more detail as soon as I can after JavaOne. We also have some
engineers out on vacation who might want to look at these too.
A couple of things that stood out :
On AIX, fontconfig is not a standard package supported by IBM.
I am surprised AIX does not support fontconfig.
That is something IBM should reconsider as its hard to imagine
a modern Unix based system working without it ..
Very basic start for AIX - feel free to complete ..
169 */
170 static char *fullAixFontPath[] = {
...
I'd really like to see it completed but these days
that's largely a fall back for no fontconfig.
/* AIX does not provide the 'dladdr' function. But fortunately, we've
42 * already implemented it in the HotSpot, because we need it there as
43 * well (see hotspot/src/os/aix/vm/porting_aix.{hpp,cpp}).
Whilst this is in ifdef AIX this reliance on an exported hotspot
function sounds hacky. What actual requirement is there that the
AIX class libraries be so tightly-coupled with that VM?
There is no contract there.
-phil.
On 9/16/2013 12:30 PM, Volker Simonis wrote:
Resending this to more lists as requested by Alan Bateman with the
kind request to anybody to review the parts for which he feels
responsible:)
For those not up to date, this change is part of the ongoing
PowerPC/AIX Porting Project:
http://openjdk.java.net/projects/ppc-aix-port
https://wiki.openjdk.java.net/display/PPCAIXPort
http://openjdk.java.net/jeps/175
Please send reviews to all currently included recipients.
Thank you and best regards,
Volker
-- Forwarded message --
From: *Volker Simonis*
Date: Monday, September 16, 2013
Subject: RFR(L): 8024854: Basic changes and files to build the class
library on AIX
To: ppc-aix-port-...@openjdk.java.net
mailto:ppc-aix-port-...@openjdk.java.net
ppc-aix-port-...@openjdk.java.net
mailto:ppc-aix-port-...@openjdk.java.net, Java Core Libs
core-libs-...@openjdk.java.net mailto:core-libs-...@openjdk.java.net
Hi,
could you please review the following webrev which contains the basic
changes and files needed in the 'jdk' repository in order to build the
OpenJDK on AIX:
http://cr.openjdk.java.net/~simonis/webrevs/8024854
http://cr.openjdk.java.net/%7Esimonis/webrevs/8024854
This change together with 8024265: Enable new build on AIX (jdk part)
http://cr.openjdk.java.net/%7Esimonis/webrevs/8024265_jdk/ allows
it to configure and completely build the staging repository on AIX 5.3
and 7.1 with the following command:
configure --with-boot-jdk=jdk-image --with-jvm-variants=core
--with-jvm-interpreter=cpp --with-cups-include=/opt/freeware/include
--x-includes=/opt/freeware/include
Below you can find the changes and additions I've done, sorted by
file. Most of them are just additions which are only active during the
AIX build anyway or simple changes where AIX has been added to
conditions which already check for Linux and/or Solaris. The files
with the biggest changes which you're probably want to look on more
thoroughly are 'src/solaris/native/java/net/NetworkInterface.c' and
'src/solaris/native/sun/nio/ch/Net.c' altough they shouldn't change
anything on the current OpenJDK platforms as well.
Notice that there are still some files and some functionality missing
from the current change (notably NIO) but it still yields a running
JDK which can execute HelloWorld on the command line and as AWT
application. I've intentionally tried to keep this initial change as
simple as possible (with respect tot shared changes) in order to get
it reviewed as fast as possible. The missing parts can then be added
later on, grouped by logical topics, to simplify the review process.
Thank you and best regards,
Volker
src/share/bin/jli_util.h
* Define |JLI_Lseek| on AIX.
src/share/lib/security/java.security-aix
* Provide default |java.security-aix| for AIX.
src/share/native/sun/awt/medialib/mlib_sys.c
* |malloc| always returns 8-byte aligned pointers on AIX as well.
src/share/native/sun/awt/medialib/mlib_types.h
* Add AIX to the list of known platforms.
src/share/native/sun/font/layout/KernTable.cpp
* Rename the macro |DEBUG| to |DEBUG_KERN_TABLE| because |DEBUG| is
too common and may be defined in other headers as well as on the
command line and |xlc| bails out on macro redefinitions with a
different value.
src/share/native/sun/security/ec/impl/ecc_impl.h
* Define required types and macros on AIX.
src/solaris/back/exec_md.c
* AIX behaves like Linux in this case so check for it in the Linux
branch.
src/solaris/bin/java_md_solinux.c,
src/solaris/bin/java_md_solinux.h
* On AIX |LD_LIBRARY_PATH| is called |LIBPATH|
* Always use |LD_LIBRARY_PATH| macro instead of using the string
|LD_LIBRARY_PATH| directly to cope with different library path
Re: Review Request: 7164376 Replace use of sun.security.action.LoadLibraryAction
All looks good to me. Compiler won't spot misspelled library names so I
did try to check all those are still the same.
-phil.
On 4/26/2012 2:20 PM, Mandy Chung wrote:
Thanks, Sean. I have fixed the 3 files per your comment.
Mandy
On 4/26/2012 1:59 PM, Sean Mullan wrote:
Looks fine, just a couple of nits.
src/macosx/classes/com/apple/concurrent/LibDispatchNative.java,
- the closing static brace is not indented the same as the open brace.
src/solaris/classes/sun/management/FileSystemImpl.java
src/windows/classes/sun/management/FileSystemImpl.java
- line-break coding style is different from all others; probably
better to be consistent
--Sean
On 04/26/2012 02:49 PM, Mandy Chung wrote:
7164376 Replace use of sun.security.action.LoadLibraryAction with
direct call of System.loadLibrary
Webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7164376/webrev.00/
This change is required for jdk modularization. High level summary:
it replaces the use of LoadLibraryAction:
FROM: java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));
TO: AccessController.doPrivileged( new
java.security.PrivilegedActionVoid() { public Void run() {
System.loadLibrary(net); return null; } });
It touches files in awt, security and serviceability area (cc'ed).
For this type of simple change, I think 1-2 reviewers can review all
files (simpler to review jdk.patch) and no need for all teams to do
the reviews.
System.loadLibrary and Runtime.loadLibrary loads a system library of
the given library name that requires
RuntimePermission(loadLibrary.+lib) permission. Many places in the
JDK code loading a system native library is using the
sun.security.action.LoadLibraryAction convenient class that will load
the system library as a privileged action:
java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));
The search path of native libraries are coupled with an associated
class loader. For example, the application class loader uses the path
specified in the java.library.path system property for native
library lookup. The loadLibrary implementation uses the caller's
class loader for finding the native library being requested. For
system libraries, the class loader is null and the system library
lookup is handled as a special case. When the
sun.security.action.LoadLibraryAction class is used that is the
caller of System.loadLibrary, the caller's class loader in this case
is null loader and thus it always finds the native library from the
system library path.
In a modular world, JDK modules may be loaded by multiple different
module class loader. The following code would not work if it is
expected to load a native library from a module which is not the
module where the sun.security.action.LoadLibraryAction lives.
For example, the management module is trying to load
libmanagement.so. Calling the following will fail to find
libmanagement.so because the caller of System.loadLibrary is the
LoadLibraryAction which is in the base module and search the library
from the base module only. To prepare for jdk modularization, the use
of LoadLibraryAction should be replaced with a direct call of
System.loadLibrary.
This patch also removes sun.security.action.LoadLibraryAction class
to avoid regression.
Thanks Mandy
