Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v3]
On Mon, 11 Dec 2023 14:06:43 GMT, Stefan Karlsson wrote: >> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename >> createJavaProcessBuilder' changed the name of the ProcessTools helper >> functions used to create `ProcessBuilder`s used to spawn new java test >> processes. >> >> We now have `createTestJavaProcessBuilder` and >> `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, >> while the latter doesn't. >> >> With these functions it is common to see the following pattern in tests: >> >> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); >> OutputAnalyzer output = executeProcess(pb); >> >> >> We have a couple of thin wrapper in `ProcessTools` that does exactly this, >> so that the code can be written as a one-liner: >> >> OutputAnalyzer output = ProcessTools.executeTestJvm(); >> >> >> I propose that we name this functions using the same naming scheme we used >> for `createTestJavaProcessBuilder` and >> `createLimitedTestJavaProcessBuilder`. That is, we change `executeTestJvm` >> to `executeTestJava` and add a new `executeLimitedTestJava` function. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Test cleanup Looks good to me. - Marked as reviewed by lkorinth (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17049#pullrequestreview-1775250269
Integrated: 8318964: Fix build failures caused by 8315097
On Fri, 27 Oct 2023 09:48:00 GMT, Leo Korinth wrote: > Update method name after huge renaming conflict This pull request has now been integrated. Changeset: b9dcd4b7 Author: Leo Korinth URL: https://git.openjdk.org/jdk/commit/b9dcd4b74138dd77faa46525f101b985248fffc5 Stats: 5 lines in 5 files changed: 0 ins; 0 del; 5 mod 8318964: Fix build failures caused by 8315097 Reviewed-by: aboldtch, rcastanedalo - PR: https://git.openjdk.org/jdk/pull/16395
Re: Integrated: 8318964: Fix build failures caused by 8315097
On Fri, 27 Oct 2023 09:48:00 GMT, Leo Korinth wrote: > Update method name after huge renaming conflict Thanks!!! - PR Comment: https://git.openjdk.org/jdk/pull/16395#issuecomment-1782627439
Integrated: 8318964: Fix build failures caused by 8315097
Update method name after huge renaming conflict - Commit messages: - 8318964: Fix build failures caused by 8315097 Changes: https://git.openjdk.org/jdk/pull/16395/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16395=00 Issue: https://bugs.openjdk.org/browse/JDK-8318964 Stats: 5 lines in 5 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16395.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16395/head:pull/16395 PR: https://git.openjdk.org/jdk/pull/16395
Re: RFR: 8315097: Rename createJavaProcessBuilder [v7]
On Wed, 25 Oct 2023 08:44:29 GMT, Leo Korinth wrote: >> This pull request renames `createJavaProcessBuilder` to >> `createLimitedTestJavaProcessBuilder` and renames `createTestJvm` to >> `createTestJavaProcessBuilder`. Both are implemented through a private >> `createJavaProcessBuilder`. It also updates the java doc. >> >> This is so that it should be harder to by mistake use >> `createLimitedTestJavaProcessBuilder` that is problematic because it will >> not forward JVM flags to the tested JVM. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > fix copyright year and indentation Thanks, see: https://bugs.openjdk.org/browse/JDK-8318962 - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1782552641
Integrated: 8315097: Rename createJavaProcessBuilder
On Mon, 28 Aug 2023 15:54:08 GMT, Leo Korinth wrote: > This pull request renames `createJavaProcessBuilder` to > `createLimitedTestJavaProcessBuilder` and renames `createTestJvm` to > `createTestJavaProcessBuilder`. Both are implemented through a private > `createJavaProcessBuilder`. It also updates the java doc. > > This is so that it should be harder to by mistake use > `createLimitedTestJavaProcessBuilder` that is problematic because it will not > forward JVM flags to the tested JVM. This pull request has now been integrated. Changeset: d52a995f Author:Leo Korinth URL: https://git.openjdk.org/jdk/commit/d52a995f35de26c2cc4074297a75141e4a363e1b Stats: 1574 lines in 560 files changed: 44 ins; 10 del; 1520 mod 8315097: Rename createJavaProcessBuilder Reviewed-by: lmesnik, dholmes, rriggs, stefank - PR: https://git.openjdk.org/jdk/pull/15452
Re: RFR: 8315097: Rename createJavaProcessBuilder [v7]
On Wed, 25 Oct 2023 08:44:29 GMT, Leo Korinth wrote:
>> This pull request renames `createJavaProcessBuilder` to
>> `createLimitedTestJavaProcessBuilder` and renames `createTestJvm` to
>> `createTestJavaProcessBuilder`. Both are implemented through a private
>> `createJavaProcessBuilder`. It also updates the java doc.
>>
>> This is so that it should be harder to by mistake use
>> `createLimitedTestJavaProcessBuilder` that is problematic because it will
>> not forward JVM flags to the tested JVM.
>
> Leo Korinth has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix copyright year and indentation
Would it be okay if we handle the new method documentation in a separate pull
request?
After applying your changes, I also noted that the existing description `The
command line will be like: {test.jdk}/bin/java {test.vm.opts} {test.java.opts}
cmds` is not only incorrect (or at least incomplete), but now also clashes with
the added description. I then removed the sentence, but after I did that I also
found out that similar wording exist in `executeTestJvm` and I fear that if I
continue to pull strings, I will create more and more changes that you will
have opinions on.
Is it all right if we push what we have now, and that I create a new pull
requests with these improvements in documentation that are actually not related
to the changes in this pull request?
-
PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1781359450
Re: RFR: 8315097: Rename createJavaProcessBuilder [v7]
> This pull request renames `createJavaProcessBuilder` to > `createLimitedTestJavaProcessBuilder` and renames `createTestJvm` to > `createTestJavaProcessBuilder`. Both are implemented through a private > `createJavaProcessBuilder`. It also updates the java doc. > > This is so that it should be harder to by mistake use > `createLimitedTestJavaProcessBuilder` that is problematic because it will not > forward JVM flags to the tested JVM. Leo Korinth has updated the pull request incrementally with one additional commit since the last revision: fix copyright year and indentation - Changes: - all: https://git.openjdk.org/jdk/pull/15452/files - new: https://git.openjdk.org/jdk/pull/15452/files/2f57a32d..4cc3865a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15452=06 - incr: https://webrevs.openjdk.org/?repo=jdk=15452=05-06 Stats: 23 lines in 1 file changed: 0 ins; 0 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/15452.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15452/head:pull/15452 PR: https://git.openjdk.org/jdk/pull/15452
Re: RFR: 8315097: Rename createJavaProcessBuilder [v6]
On Tue, 24 Oct 2023 07:49:30 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with six additional > commits since the last revision: > > - static import > - copyright > - whitespace > - whitespace > - sed > - fix test/lib/jdk/test/lib/process/ProcessTools.java Hi, if you want to see what I have modified manually, you can do my sed commands and compare to this pull request: git switch -c _reproduce 15acf4b8d7cffcd0d74bf1b9c43cde9acaf31ea9 find -name "*.java" | xargs -n 1 sed -i -e "s/createJavaProcessBuilder(/createLimitedTestJavaProcessBuilder(/g" find -name "*.java" | xargs -n 1 sed -i -e "s/createTestJvm(/createTestJavaProcessBuilder(/g" find -name "*.java" | xargs -n 1 sed -i -e "s/import static jdk.test.lib.process.ProcessTools.createJavaProcessBuilder/import static jdk.test.lib.process.ProcessTools.createLimitedTestJavaProcessBuilder/g" find -name "*.java" | xargs -n 1 sed -i -e "s/import static jdk.test.lib.process.ProcessTools.createTestJvm/import static jdk.test.lib.process.ProcessTools.createTestJavaProcessBuilder/g" git add -u; git commit -m sed git diff-tree --no-commit-id --name-only -r 15acf4b8d7cffcd0d74bf1b9c43cde9acaf31ea9..HEAD | xargs sed -i -e "s%^( * Copyright (c) )[^[:alpha:]]*(Oracle.*)%\1, 2023, \2%" git ls-files -m | xargs sed -i -e "s%(Copyright (c) 2023), 2023, (Oracle.*)%\1, \2%" git add -u; git commit -m copyright git diff HEAD 2f57a32df8d17da51a04177563327ca2a75e8061 It will give you an easier way to review. - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1776817287
Re: RFR: 8315097: Rename createJavaProcessBuilder [v6]
> Rename createJavaProcessBuilder so that it is not used by mistake instead of > createTestJvm. > > I have used the following sed script: `find -name "*.java" | xargs -n 1 sed > -i -e > "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` > > Then I have manually modified ProcessTools.java. In that file I have moved > one version of createJavaProcessBuilder so that it is close to the other > version. Then I have added a javadoc comment in bold telling: > >/** > * Create ProcessBuilder using the java launcher from the jdk to > * be tested. > * > * Please observe that you likely should use > * createTestJvm() instead of this method because createTestJvm() > * will add JVM options from "test.vm.opts" and "test.java.opts" > * and this method will not do that. > * > * @param command Arguments to pass to the java command. > * @return The ProcessBuilder instance representing the java command. > */ > > > I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of > the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a > better name I could do a rename of the method. I kind of like that it is long > and clumsy, that makes it harder to use... > > I have run tier 1 testing, and I have started more exhaustive testing. Leo Korinth has updated the pull request incrementally with six additional commits since the last revision: - static import - copyright - whitespace - whitespace - sed - fix test/lib/jdk/test/lib/process/ProcessTools.java - Changes: - all: https://git.openjdk.org/jdk/pull/15452/files - new: https://git.openjdk.org/jdk/pull/15452/files/f80dda8d..2f57a32d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15452=05 - incr: https://webrevs.openjdk.org/?repo=jdk=15452=04-05 Stats: 1580 lines in 560 files changed: 44 ins; 34 del; 1502 mod Patch: https://git.openjdk.org/jdk/pull/15452.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15452/head:pull/15452 PR: https://git.openjdk.org/jdk/pull/15452
Re: RFR: 8315097: Rename createJavaProcessBuilder [v5]
On Thu, 19 Oct 2023 15:16:13 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains ten commits: > > - Batch update using sed > >find -name "*.java" | xargs -n 1 sed -i -e > "s/createJavaProcessBuilder(/createLimitedJavaTestProcessBuilder(/g" >find -name "*.java" | xargs -n 1 sed -i -e > "s/createTestJvm(/createJavaTestProcessBuilder(/g" >find -name "*.java" | xargs -n 1 sed -i -e "s/import static > jdk.test.lib.process.ProcessTools.createJavaProcessBuilder/import static > jdk.test.lib.process.ProcessTools.createLimitedJavaTestProcessBuilder/g" > - Merge branch '_master_jdk' into _8315097 > - explain usage > - Revert "8315097: Rename createJavaProcessBuilder" > >This reverts commit 4b2d171133c40c5c48114602bfd0d4da75531317. > - Revert "copyright" > >This reverts commit f3418c80cc0d4cbb722ee5e368f1a001e898b43e. > - Revert "fix static import" > >This reverts commit 27da71508aec9a4bec1c0ad07031887286580171. > - fix static import > - copyright > - 8315097: Rename createJavaProcessBuilder Just ignore what I just pushed, I will have a new version out sorry... - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1772309579 PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1772309992
Re: RFR: 8315097: Rename createJavaProcessBuilder [v4]
On Tue, 17 Oct 2023 12:29:46 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with three additional > commits since the last revision: > > - Revert "8315097: Rename createJavaProcessBuilder" > >This reverts commit 4b2d171133c40c5c48114602bfd0d4da75531317. > - Revert "copyright" > >This reverts commit f3418c80cc0d4cbb722ee5e368f1a001e898b43e. > - Revert "fix static import" > >This reverts commit 27da71508aec9a4bec1c0ad07031887286580171. If this looks roughly acceptable, I will manually add indentation spaces. I am now running tests. The changes can be verified by running the following commands: git switch -c _reproduce 15acf4b8d7cffcd0d74bf1b9c43cde9acaf31ea9 find -name "*.java" | xargs -n 1 sed -i -e "s/createJavaProcessBuilder(/createLimitedJavaTestProcessBuilder(/g" find -name "*.java" | xargs -n 1 sed -i -e "s/createTestJvm(/createJavaTestProcessBuilder(/g" find -name "*.java" | xargs -n 1 sed -i -e "s/import static jdk.test.lib.process.ProcessTools.createJavaProcessBuilder/import static jdk.test.lib.process.ProcessTools.createLimitedJavaTestProcessBuilder/g" git diff HEAD f80dda8d7109c2ef6bc1f685d0b611704dec645e Only the documentation changes should be visible. When I have manually indented everything it should be easy to that verify that change as a whitespace-only change. But that is for tomorrow (at best). - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1771194189
Re: RFR: 8315097: Rename createJavaProcessBuilder [v5]
> Rename createJavaProcessBuilder so that it is not used by mistake instead of > createTestJvm. > > I have used the following sed script: `find -name "*.java" | xargs -n 1 sed > -i -e > "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` > > Then I have manually modified ProcessTools.java. In that file I have moved > one version of createJavaProcessBuilder so that it is close to the other > version. Then I have added a javadoc comment in bold telling: > >/** > * Create ProcessBuilder using the java launcher from the jdk to > * be tested. > * > * Please observe that you likely should use > * createTestJvm() instead of this method because createTestJvm() > * will add JVM options from "test.vm.opts" and "test.java.opts" > * and this method will not do that. > * > * @param command Arguments to pass to the java command. > * @return The ProcessBuilder instance representing the java command. > */ > > > I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of > the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a > better name I could do a rename of the method. I kind of like that it is long > and clumsy, that makes it harder to use... > > I have run tier 1 testing, and I have started more exhaustive testing. Leo Korinth has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - Batch update using sed find -name "*.java" | xargs -n 1 sed -i -e "s/createJavaProcessBuilder(/createLimitedJavaTestProcessBuilder(/g" find -name "*.java" | xargs -n 1 sed -i -e "s/createTestJvm(/createJavaTestProcessBuilder(/g" find -name "*.java" | xargs -n 1 sed -i -e "s/import static jdk.test.lib.process.ProcessTools.createJavaProcessBuilder/import static jdk.test.lib.process.ProcessTools.createLimitedJavaTestProcessBuilder/g" - Merge branch '_master_jdk' into _8315097 - explain usage - Revert "8315097: Rename createJavaProcessBuilder" This reverts commit 4b2d171133c40c5c48114602bfd0d4da75531317. - Revert "copyright" This reverts commit f3418c80cc0d4cbb722ee5e368f1a001e898b43e. - Revert "fix static import" This reverts commit 27da71508aec9a4bec1c0ad07031887286580171. - fix static import - copyright - 8315097: Rename createJavaProcessBuilder - Changes: https://git.openjdk.org/jdk/pull/15452/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15452=04 Stats: 894 lines in 560 files changed: 34 ins; 10 del; 850 mod Patch: https://git.openjdk.org/jdk/pull/15452.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15452/head:pull/15452 PR: https://git.openjdk.org/jdk/pull/15452
Re: RFR: 8315097: Rename createJavaProcessBuilder [v4]
> Rename createJavaProcessBuilder so that it is not used by mistake instead of > createTestJvm. > > I have used the following sed script: `find -name "*.java" | xargs -n 1 sed > -i -e > "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` > > Then I have manually modified ProcessTools.java. In that file I have moved > one version of createJavaProcessBuilder so that it is close to the other > version. Then I have added a javadoc comment in bold telling: > >/** > * Create ProcessBuilder using the java launcher from the jdk to > * be tested. > * > * Please observe that you likely should use > * createTestJvm() instead of this method because createTestJvm() > * will add JVM options from "test.vm.opts" and "test.java.opts" > * and this method will not do that. > * > * @param command Arguments to pass to the java command. > * @return The ProcessBuilder instance representing the java command. > */ > > > I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of > the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a > better name I could do a rename of the method. I kind of like that it is long > and clumsy, that makes it harder to use... > > I have run tier 1 testing, and I have started more exhaustive testing. Leo Korinth has updated the pull request incrementally with three additional commits since the last revision: - Revert "8315097: Rename createJavaProcessBuilder" This reverts commit 4b2d171133c40c5c48114602bfd0d4da75531317. - Revert "copyright" This reverts commit f3418c80cc0d4cbb722ee5e368f1a001e898b43e. - Revert "fix static import" This reverts commit 27da71508aec9a4bec1c0ad07031887286580171. - Changes: - all: https://git.openjdk.org/jdk/pull/15452/files - new: https://git.openjdk.org/jdk/pull/15452/files/27da7150..44af07b9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15452=03 - incr: https://webrevs.openjdk.org/?repo=jdk=15452=02-03 Stats: 1102 lines in 462 files changed: 11 ins; 22 del; 1069 mod Patch: https://git.openjdk.org/jdk/pull/15452.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15452/head:pull/15452 PR: https://git.openjdk.org/jdk/pull/15452
Re: RFR: 8315097: Rename createJavaProcessBuilder [v3]
On Wed, 30 Aug 2023 09:23:55 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > fix static import I think you are missing the point. If you take a look at [the parent bug of the sub task](https://bugs.openjdk.org/browse/JDK-8314823) you can see that the problem described is *not* that people are using `createTestJvm` in error. The problem is that they are (or possibly are) using `createJavaProcessBuilder` in error. Thus renaming `createTestJvm` might help a little at most for this specific problem. Renaming `createJavaProcessBuilder` most probably helps *more*. I guess the alternative of forcing the user to make a choice using an enum value will help even more. - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1708705105
Re: RFR: 8315097: Rename createJavaProcessBuilder [v3]
On Wed, 30 Aug 2023 09:23:55 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > fix static import I have created an alternative that uses enums to force the user to make a decision: https://github.com/openjdk/jdk/compare/master...lkorinth:jdk:+process_tools . Another alternative is to do the same but instead using an enum (I think it is not as good). A third alternative is to use the current pull request with a better name. What do you prefer? Do you have a better alternative? Do someone still think the current code is good? I think what we have today is inferior to all these improvements, and I would like to make it harder to develop bad test cases. - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1705068700
Re: RFR: 8315097: Rename createJavaProcessBuilder [v3]
On Wed, 30 Aug 2023 09:23:55 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > fix static import I have some ideas that I will work on that might lead to a new proposal. - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1699542111
Re: RFR: 8315097: Rename createJavaProcessBuilder [v3]
> Rename createJavaProcessBuilder so that it is not used by mistake instead of > createTestJvm. > > I have used the following sed script: `find -name "*.java" | xargs -n 1 sed > -i -e > "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` > > Then I have manually modified ProcessTools.java. In that file I have moved > one version of createJavaProcessBuilder so that it is close to the other > version. Then I have added a javadoc comment in bold telling: > >/** > * Create ProcessBuilder using the java launcher from the jdk to > * be tested. > * > * Please observe that you likely should use > * createTestJvm() instead of this method because createTestJvm() > * will add JVM options from "test.vm.opts" and "test.java.opts" > * and this method will not do that. > * > * @param command Arguments to pass to the java command. > * @return The ProcessBuilder instance representing the java command. > */ > > > I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of > the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a > better name I could do a rename of the method. I kind of like that it is long > and clumsy, that makes it harder to use... > > I have run tier 1 testing, and I have started more exhaustive testing. Leo Korinth has updated the pull request incrementally with one additional commit since the last revision: fix static import - Changes: - all: https://git.openjdk.org/jdk/pull/15452/files - new: https://git.openjdk.org/jdk/pull/15452/files/f3418c80..27da7150 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15452=02 - incr: https://webrevs.openjdk.org/?repo=jdk=15452=01-02 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15452.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15452/head:pull/15452 PR: https://git.openjdk.org/jdk/pull/15452
Re: RFR: 8315097: Rename createJavaProcessBuilder [v2]
On Tue, 29 Aug 2023 14:06:01 GMT, Roger Riggs wrote: >> Leo Korinth has updated the pull request incrementally with one additional >> commit since the last revision: >> >> copyright > > I don't think this is the best change across so many files. > It gives a very ugly name to a common test function and affects a very large > number of tests. @RogerRiggs If it is only the name you want changed, maybe you can offer a better name? - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1697634872
Re: RFR: 8315097: Rename createJavaProcessBuilder [v2]
> Rename createJavaProcessBuilder so that it is not used by mistake instead of > createTestJvm. > > I have used the following sed script: `find -name "*.java" | xargs -n 1 sed > -i -e > "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` > > Then I have manually modified ProcessTools.java. In that file I have moved > one version of createJavaProcessBuilder so that it is close to the other > version. Then I have added a javadoc comment in bold telling: > >/** > * Create ProcessBuilder using the java launcher from the jdk to > * be tested. > * > * Please observe that you likely should use > * createTestJvm() instead of this method because createTestJvm() > * will add JVM options from "test.vm.opts" and "test.java.opts" > * and this method will not do that. > * > * @param command Arguments to pass to the java command. > * @return The ProcessBuilder instance representing the java command. > */ > > > I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of > the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a > better name I could do a rename of the method. I kind of like that it is long > and clumsy, that makes it harder to use... > > I have run tier 1 testing, and I have started more exhaustive testing. Leo Korinth has updated the pull request incrementally with one additional commit since the last revision: copyright - Changes: - all: https://git.openjdk.org/jdk/pull/15452/files - new: https://git.openjdk.org/jdk/pull/15452/files/4b2d1711..f3418c80 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15452=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15452=00-01 Stats: 344 lines in 344 files changed: 0 ins; 0 del; 344 mod Patch: https://git.openjdk.org/jdk/pull/15452.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15452/head:pull/15452 PR: https://git.openjdk.org/jdk/pull/15452
RFR: 8315097: Rename createJavaProcessBuilder
Rename createJavaProcessBuilder so that it is not used by mistake instead of createTestJvm. I have used the following sed script: `find -name "*.java" | xargs -n 1 sed -i -e "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` Then I have manually modified ProcessTools.java. In that file I have moved one version of createJavaProcessBuilder so that it is close to the other version. Then I have added a javadoc comment in bold telling: /** * Create ProcessBuilder using the java launcher from the jdk to * be tested. * * Please observe that you likely should use * createTestJvm() instead of this method because createTestJvm() * will add JVM options from "test.vm.opts" and "test.java.opts" * and this method will not do that. * * @param command Arguments to pass to the java command. * @return The ProcessBuilder instance representing the java command. */ I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a better name I could do a rename of the method. I kind of like that it is long and clumsy, that makes it harder to use... I have run tier 1 testing, and I have started more exhaustive testing. - Commit messages: - 8315097: Rename createJavaProcessBuilder Changes: https://git.openjdk.org/jdk/pull/15452/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15452=00 Issue: https://bugs.openjdk.org/browse/JDK-8315097 Stats: 756 lines in 462 files changed: 22 ins; 10 del; 724 mod Patch: https://git.openjdk.org/jdk/pull/15452.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15452/head:pull/15452 PR: https://git.openjdk.org/jdk/pull/15452
Re: RFR: JDK-8293114: JVM should trim the native heap [v4]
On Thu, 6 Jul 2023 15:25:03 GMT, Thomas Stuefe wrote:
>> This is a continuation of https://github.com/openjdk/jdk/pull/10085. I
>> closed https://github.com/openjdk/jdk/pull/10085 because it had accumulated
>> too much comment history and got confusing. For a history of this issue, see
>> previous discussions [1] and the comment section of 10085.
>>
>> ---
>>
>> This RFE adds the option to trim the Glibc heap periodically. This can
>> recover a significant memory footprint if the VM process suffers from
>> high-but-rare malloc spikes. It does not matter who causes the spikes: the
>> JDK or customer code running in the JVM process.
>>
>> ### Background:
>>
>> The Glibc is reluctant to return memory to the OS. Temporary malloc spikes
>> often carry over as permanent RSS increase. Note that C-heap retention is
>> difficult to observe. Since it is freed memory, it won't appear in NMT; it
>> is just a part of RSS.
>>
>> This is, effectively, caching - a performance tradeoff by the glibc. It
>> makes a lot of sense with applications that cause high traffic on the
>> C-heap. The JVM, however, clusters allocations and often rolls its own
>> memory management based on virtual memory for many of its use cases.
>>
>> To manually trim the C-heap, Glibc exposes `malloc_trim(3)`. With JDK 18
>> [2], we added a new jcmd command to *manually* trim the C-heap on Linux
>> (`jcmd System.trim_native_heap`). We then observed customers running this
>> command periodically to slim down process sizes of container-bound jvms.
>> That is cumbersome, and the JVM can do this a lot better - among other
>> things because it knows best when *not* to trim.
>>
>> GLIBC internals
>>
>> The following information I took from the glibc source code and
>> experimenting.
>>
>> # Why do we need to trim manually? Does the Glibc not trim on free?
>>
>> Upon `free()`, glibc may return memory to the OS if:
>> - the returned block was mmap'ed
>> - the returned block was not added to tcache or to fastbins
>> - the returned block, possibly merged with its two immediate neighbors, had
>> they been free, is larger than FASTBIN_CONSOLIDATION_THRESHOLD (64K) - in
>> that case:
>> a) for the main arena, glibc attempts to lower the brk()
>> b) for mmap-ed heaps, glibc attempts to completely unmap or shrink the
>> heap.
>> In both cases, (a) and (b), only the top portion of the heap is reclaimed.
>> "Holes" in the middle of other in-use chunks are not reclaimed.
>>
>> So: glibc *may* automatically reclaim memory. In normal configurations, with
>> typical C-heap allocation granularity, it is unlikely.
>>
>> To increase the ...
>
> Thomas Stuefe has updated the pull request incrementally with one additional
> commit since the last revision:
>
> last cleanups and shade feedback
src/hotspot/share/runtime/trimNative.cpp line 78:
> 76: static constexpr int safepoint_poll_ms = 250;
> 77:
> 78: static int64_t now() { return os::javaTimeMillis(); }
I think it would be better to not use CLOCK_REALTIME in case of clock changes
by NTP etc.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14781#discussion_r1254699171
Re: RFR: JDK-8293114: JVM should trim the native heap [v4]
On Thu, 6 Jul 2023 15:25:03 GMT, Thomas Stuefe wrote:
>> This is a continuation of https://github.com/openjdk/jdk/pull/10085. I
>> closed https://github.com/openjdk/jdk/pull/10085 because it had accumulated
>> too much comment history and got confusing. For a history of this issue, see
>> previous discussions [1] and the comment section of 10085.
>>
>> ---
>>
>> This RFE adds the option to trim the Glibc heap periodically. This can
>> recover a significant memory footprint if the VM process suffers from
>> high-but-rare malloc spikes. It does not matter who causes the spikes: the
>> JDK or customer code running in the JVM process.
>>
>> ### Background:
>>
>> The Glibc is reluctant to return memory to the OS. Temporary malloc spikes
>> often carry over as permanent RSS increase. Note that C-heap retention is
>> difficult to observe. Since it is freed memory, it won't appear in NMT; it
>> is just a part of RSS.
>>
>> This is, effectively, caching - a performance tradeoff by the glibc. It
>> makes a lot of sense with applications that cause high traffic on the
>> C-heap. The JVM, however, clusters allocations and often rolls its own
>> memory management based on virtual memory for many of its use cases.
>>
>> To manually trim the C-heap, Glibc exposes `malloc_trim(3)`. With JDK 18
>> [2], we added a new jcmd command to *manually* trim the C-heap on Linux
>> (`jcmd System.trim_native_heap`). We then observed customers running this
>> command periodically to slim down process sizes of container-bound jvms.
>> That is cumbersome, and the JVM can do this a lot better - among other
>> things because it knows best when *not* to trim.
>>
>> GLIBC internals
>>
>> The following information I took from the glibc source code and
>> experimenting.
>>
>> # Why do we need to trim manually? Does the Glibc not trim on free?
>>
>> Upon `free()`, glibc may return memory to the OS if:
>> - the returned block was mmap'ed
>> - the returned block was not added to tcache or to fastbins
>> - the returned block, possibly merged with its two immediate neighbors, had
>> they been free, is larger than FASTBIN_CONSOLIDATION_THRESHOLD (64K) - in
>> that case:
>> a) for the main arena, glibc attempts to lower the brk()
>> b) for mmap-ed heaps, glibc attempts to completely unmap or shrink the
>> heap.
>> In both cases, (a) and (b), only the top portion of the heap is reclaimed.
>> "Holes" in the middle of other in-use chunks are not reclaimed.
>>
>> So: glibc *may* automatically reclaim memory. In normal configurations, with
>> typical C-heap allocation granularity, it is unlikely.
>>
>> To increase the ...
>
> Thomas Stuefe has updated the pull request incrementally with one additional
> commit since the last revision:
>
> last cleanups and shade feedback
src/hotspot/share/runtime/trimNative.cpp line 42:
> 40: class NativeTrimmerThread : public NamedThread {
> 41:
> 42: Monitor* const _lock;
Maybe inline this instead of having it a pointer? Even if the thread and lock
is not destroyed until VM shutdown, I always feel the need to match new in
constructors with delete in destructors.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14781#discussion_r1254691667
Re: RFR: JDK-8293114: JVM should trim the native heap [v4]
On Thu, 6 Jul 2023 15:25:03 GMT, Thomas Stuefe wrote: >> This is a continuation of https://github.com/openjdk/jdk/pull/10085. I >> closed https://github.com/openjdk/jdk/pull/10085 because it had accumulated >> too much comment history and got confusing. For a history of this issue, see >> previous discussions [1] and the comment section of 10085. >> >> --- >> >> This RFE adds the option to trim the Glibc heap periodically. This can >> recover a significant memory footprint if the VM process suffers from >> high-but-rare malloc spikes. It does not matter who causes the spikes: the >> JDK or customer code running in the JVM process. >> >> ### Background: >> >> The Glibc is reluctant to return memory to the OS. Temporary malloc spikes >> often carry over as permanent RSS increase. Note that C-heap retention is >> difficult to observe. Since it is freed memory, it won't appear in NMT; it >> is just a part of RSS. >> >> This is, effectively, caching - a performance tradeoff by the glibc. It >> makes a lot of sense with applications that cause high traffic on the >> C-heap. The JVM, however, clusters allocations and often rolls its own >> memory management based on virtual memory for many of its use cases. >> >> To manually trim the C-heap, Glibc exposes `malloc_trim(3)`. With JDK 18 >> [2], we added a new jcmd command to *manually* trim the C-heap on Linux >> (`jcmd System.trim_native_heap`). We then observed customers running this >> command periodically to slim down process sizes of container-bound jvms. >> That is cumbersome, and the JVM can do this a lot better - among other >> things because it knows best when *not* to trim. >> >> GLIBC internals >> >> The following information I took from the glibc source code and >> experimenting. >> >> # Why do we need to trim manually? Does the Glibc not trim on free? >> >> Upon `free()`, glibc may return memory to the OS if: >> - the returned block was mmap'ed >> - the returned block was not added to tcache or to fastbins >> - the returned block, possibly merged with its two immediate neighbors, had >> they been free, is larger than FASTBIN_CONSOLIDATION_THRESHOLD (64K) - in >> that case: >> a) for the main arena, glibc attempts to lower the brk() >> b) for mmap-ed heaps, glibc attempts to completely unmap or shrink the >> heap. >> In both cases, (a) and (b), only the top portion of the heap is reclaimed. >> "Holes" in the middle of other in-use chunks are not reclaimed. >> >> So: glibc *may* automatically reclaim memory. In normal configurations, with >> typical C-heap allocation granularity, it is unlikely. >> >> To increase the ... > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > last cleanups and shade feedback The description says `-XX:GCTrimNativeHeapInterval= (defaults to 60)`, but the code says milliseconds. - PR Comment: https://git.openjdk.org/jdk/pull/14781#issuecomment-1623895924
Re: RFR: JDK-8293114: JVM should trim the native heap [v2]
On Thu, 6 Jul 2023 13:01:15 GMT, Thomas Stuefe wrote: >> This is a continuation of https://github.com/openjdk/jdk/pull/10085. I >> closed https://github.com/openjdk/jdk/pull/10085 because it had accumulated >> too much comment history and got confusing. For a history of this issue, see >> previous discussions [1] and the comment section of 10085. >> >> --- >> >> This RFE adds the option to trim the Glibc heap periodically. This can >> recover a significant memory footprint if the VM process suffers from >> high-but-rare malloc spikes. It does not matter who causes the spikes: the >> JDK or customer code running in the JVM process. >> >> ### Background: >> >> The Glibc is reluctant to return memory to the OS. Temporary malloc spikes >> often carry over as permanent RSS increase. Note that C-heap retention is >> difficult to observe. Since it is freed memory, it won't appear in NMT; it >> is just a part of RSS. >> >> This is, effectively, caching - a performance tradeoff by the glibc. It >> makes a lot of sense with applications that cause high traffic on the >> C-heap. The JVM, however, clusters allocations and often rolls its own >> memory management based on virtual memory for many of its use cases. >> >> To manually trim the C-heap, Glibc exposes `malloc_trim(3)`. With JDK 18 >> [2], we added a new jcmd command to *manually* trim the C-heap on Linux >> (`jcmd System.trim_native_heap`). We then observed customers running this >> command periodically to slim down process sizes of container-bound jvms. >> That is cumbersome, and the JVM can do this a lot better - among other >> things because it knows best when *not* to trim. >> >> GLIBC internals >> >> The following information I took from the glibc source code and >> experimenting. >> >> # Why do we need to trim manually? Does the Glibc not trim on free? >> >> Upon `free()`, glibc may return memory to the OS if: >> - the returned block was mmap'ed >> - the returned block was not added to tcache or to fastbins >> - the returned block, possibly merged with its two immediate neighbors, had >> they been free, is larger than FASTBIN_CONSOLIDATION_THRESHOLD (64K) - in >> that case: >> a) for the main arena, glibc attempts to lower the brk() >> b) for mmap-ed heaps, glibc attempts to completely unmap or shrink the >> heap. >> In both cases, (a) and (b), only the top portion of the heap is reclaimed. >> "Holes" in the middle of other in-use chunks are not reclaimed. >> >> So: glibc *may* automatically reclaim memory. In normal configurations, with >> typical C-heap allocation granularity, it is unlikely. >> >> To increase the ... > > Thomas Stuefe has updated the pull request incrementally with three > additional commits since the last revision: > > - seconds->ms > - rename pause, unpause -> suspend, resume > - fix ChunkPool::needs_cleaning src/hotspot/share/runtime/trimNative.cpp line 22: > 20: * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA > 21: * or visit www.oracle.com if you need additional information or have any > 22: * questioSns. This copyright notice differs in "questioSns" (extra 'S') and lack of "DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. ", might create problems for Oracle scripts - PR Review Comment: https://git.openjdk.org/jdk/pull/14781#discussion_r1254611231
Withdrawn: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/14698
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change This was not liked, I will close it. I will possibly do it under another PR for the GC code. Thanks for reviewing. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613526703
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 12:40:34 GMT, Coleen Phillimore wrote: > You could fix your emacs functions. It is a *very nice* feature of global-whitespace-cleanup-mode - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613252347
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 12:11:40 GMT, David Holmes wrote: > Neither the PR diffs nor the webrev make it easy to see exactly what is being > changed here. It appeared to me that the last empty line of each file was > being deleted, leaving no newline at the end. My changes look like this in the diff output } - Removal of the last newline would look like this: -} +} \ No newline at end of file (both with `git diff` and `git diff --unified`) I have not tested if this is also true for the generated webrevs, but I think that is precisely how they are created. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613152641
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 12:01:03 GMT, Coleen Phillimore wrote: > Why do we care about this? I care because of global-whitespace-cleanup-mode (in emacs). It helps me remove trailing whitespaces and blanklines when saving but it will not fix a file that was "dirty" when it was opened. Trailing blank lines triggers it not to clean whitespaces for me. And it does not look good. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613095390
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 07:41:11 GMT, David Holmes wrote: > This seems to run contrary to the requirement that files end in a newline, > which git will complain about if the newline is missing. > > It also seems far too large and disruptive. Do you still think it is too disruptive after Erik's explanation? I could split it in more reviews, but I thought that maybe it would just make the review harder. I was hoping the two `git diff` commands I gave in my first comment in combination with the clean script would make the change seem reviewable. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1612660457
RFR: 8311043: Remove trailing blank lines in source files
Remove trailing "blank" lines in source files. I like to use global-whitespace-cleanup-mode, but I can not use it if the files are "dirty" to begin with. This fix will make more files "clean". I also considered adding a check for this in jcheck for Skara, however it seems jcheck code handling hunks does not track end-of-files. Thus I will only clean the files. The fix removes trailing lines matching ^[[:space:]]*$ in - *.java - *.cpp - *.hpp - *.c - *.h I have applied the following bash script to each file: file="$1" while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do truncate -s -1 "$file" done `git diff --ignore-space-change --ignore-blank-lines master` displays no changes `git diff --ignore-blank-lines master` displays one change - Commit messages: - h - c - hpp - cpp - 8311043: Remove trailing blank lines in source files Changes: https://git.openjdk.org/jdk/pull/14698/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14698=00 Issue: https://bugs.openjdk.org/browse/JDK-8311043 Stats: 4529 lines in 4382 files changed: 0 ins; 4529 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14698.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14698/head:pull/14698 PR: https://git.openjdk.org/jdk/pull/14698
Integrated: 8309468: Remove jvmti Allocate locker test case
On Mon, 12 Jun 2023 16:39:12 GMT, Leo Korinth wrote: > There is a bunch of tests that are used to test critical section/gc locker. > One of the test is named jvmti locker. In that test, JNI code is doing a loop > of ` jvmti->Allocate()` followed `sleep()` followed by a > `jvmti->Deallocate()`. There is no JVM lock implementation to be tested on > jvmti Allocate/Deallocate, they are implemented using malloc/free. Let us > save test time, code complexity and confusion by removing this test. > > This removal is very similar to https://bugs.openjdk.org/browse/JDK-8309048 > > (Oracle) hs-tier5 testing passed on x86-64. This pull request has now been integrated. Changeset: c884862a Author:Leo Korinth URL: https://git.openjdk.org/jdk/commit/c884862ad2189654596df27a76ab685dcd7399f6 Stats: 260 lines in 9 files changed: 0 ins; 257 del; 3 mod 8309468: Remove jvmti Allocate locker test case Reviewed-by: dholmes, lmesnik, sspitsyn - PR: https://git.openjdk.org/jdk/pull/14421
Re: RFR: 8309468: Remove jvmti Allocate locker test case
On Mon, 12 Jun 2023 16:39:12 GMT, Leo Korinth wrote: > There is a bunch of tests that are used to test critical section/gc locker. > One of the test is named jvmti locker. In that test, JNI code is doing a loop > of ` jvmti->Allocate()` followed `sleep()` followed by a > `jvmti->Deallocate()`. There is no JVM lock implementation to be tested on > jvmti Allocate/Deallocate, they are implemented using malloc/free. Let us > save test time, code complexity and confusion by removing this test. > > This removal is very similar to https://bugs.openjdk.org/browse/JDK-8309048 > > (Oracle) hs-tier5 testing passed on x86-64. Thanks David, Leonid and Serguei! - PR Comment: https://git.openjdk.org/jdk/pull/14421#issuecomment-1589211147
Re: RFR: 8296926: Sort include lines of files in the include/ directory [v4]
On Wed, 16 Nov 2022 11:02:19 GMT, Stefan Karlsson wrote: >> One of the more prevalent issues is that files in src/hotspot/share/include >> are not properly sorted. There has been some discussion that that was done >> on purpose, but it just adds another exception to the include rules that >> don't have any practical purposes, IMHO. It also goes against our written >> style guide around include files. One argument why it was OK have the files >> in include/ pushed up to the top of the sorted block, was that the file was >> included without specifying a directory. That's an argument that contradicts >> how we treat platform-dependent files, which (unfortunately) often also are >> specified without a prefixed directory. To remove this special case, I've >> removed the extraneous make file entry to have src/hotspot/share/include in >> the set of directories to search for headers when compiling HotSpot. Now all >> the header files in src/hotspot/share/include gets included by specifying >> the path from src/hotspot/share, just like the other platform-independent >> headers in HotSpo t. >> >> This RFE splits out the 'include/' changes from #11108 / JDK-8296886, so >> that those changes can be discussed separately. > > Stefan Karlsson has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Remove include/ from test/hotspot files > - Merge remote-tracking branch 'upstream/master' into > 8296926_proper_include_lines_for_include_dir_files > - Revert make file changes > - Remove include/ from includes > - 8296926: Use proper include lines for files in include/ Although I prefer the original solution with path, I think the current version is also a good improvement. Approved. Thanks for fixing this! - Marked as reviewed by lkorinth (Reviewer). PR: https://git.openjdk.org/jdk/pull/11133
