Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Wed, 6 Dec 2023 01:56:46 GMT, David Holmes wrote: > FWIW exitCode out numbers exitValue in source code 3:1 (and > 5:1 in test > code). That might be the case, but both the called function returning the value and the function we are changing uses the name exitValue. It seems irrelevant that other places in the code uses exitCode. - PR Comment: https://git.openjdk.org/jdk/pull/16919#issuecomment-1842489162
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Tue, 5 Dec 2023 10:56:44 GMT, Jaikiran Pai wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> remove micro optimization > > Thank you Stefan and Leonid for the reviews. Sorry I missed this party :) Thanks for finding and fixing @jaikiran . FWIW `exitCode` out numbers `exitValue` in source code 3:1 (and > 5:1 in test code). - PR Comment: https://git.openjdk.org/jdk/pull/16919#issuecomment-1841951455
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Fri, 1 Dec 2023 12:44:17 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change to the test library's >> `OutputAnalyzer` class, which proposes to remove some unnecessary logging >> from the `getExitValue()` call? >> >> As noted in https://bugs.openjdk.org/browse/JDK-8321163, right now this >> method logs: >> >> >> [2023-11-24T11:47:54.557561Z] Waiting for completion for process 24909 >> [2023-11-24T11:47:54.557873Z] Waiting for completion finished for process >> 24909 >> >> >> even when the process has already completed and the exit value already >> known. The change in this PR makes it such that if the exit value is >> available then we no longer log this (nor call `process.waitFor()`). >> >> No new tests have been added given the nature of this change. tier1, tier2 >> and tier3 tests continue to pass with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > remove micro optimization Thank you Stefan and Leonid for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/16919#issuecomment-1840512527
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Fri, 1 Dec 2023 20:01:42 GMT, Leonid Mesnik wrote: > Technically, the volatile is not enough to avoid racy writing into exitValue. > However, it doesn't affect logic. Agreed. - PR Comment: https://git.openjdk.org/jdk/pull/16919#issuecomment-1836981670
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Fri, 1 Dec 2023 12:44:17 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change to the test library's >> `OutputAnalyzer` class, which proposes to remove some unnecessary logging >> from the `getExitValue()` call? >> >> As noted in https://bugs.openjdk.org/browse/JDK-8321163, right now this >> method logs: >> >> >> [2023-11-24T11:47:54.557561Z] Waiting for completion for process 24909 >> [2023-11-24T11:47:54.557873Z] Waiting for completion finished for process >> 24909 >> >> >> even when the process has already completed and the exit value already >> known. The change in this PR makes it such that if the exit value is >> available then we no longer log this (nor call `process.waitFor()`). >> >> No new tests have been added given the nature of this change. tier1, tier2 >> and tier3 tests continue to pass with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > remove micro optimization Technically, the volatile is not enough to avoid racy writing into exitValue. However, it doesn't affect logic. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16919#pullrequestreview-1760491166
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Fri, 1 Dec 2023 12:44:17 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change to the test library's >> `OutputAnalyzer` class, which proposes to remove some unnecessary logging >> from the `getExitValue()` call? >> >> As noted in https://bugs.openjdk.org/browse/JDK-8321163, right now this >> method logs: >> >> >> [2023-11-24T11:47:54.557561Z] Waiting for completion for process 24909 >> [2023-11-24T11:47:54.557873Z] Waiting for completion finished for process >> 24909 >> >> >> even when the process has already completed and the exit value already >> known. The change in this PR makes it such that if the exit value is >> available then we no longer log this (nor call `process.waitFor()`). >> >> No new tests have been added given the nature of this change. tier1, tier2 >> and tier3 tests continue to pass with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > remove micro optimization Looks good! - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16919#pullrequestreview-1759744755
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Fri, 1 Dec 2023 12:09:14 GMT, Stefan Karlsson wrote: >> Hello Stefan, this is just for a tiny optimization to prevent multiple reads >> (in the same thread) on the `volatile` field `processExitCode` in this >> method. > > I don't think such an optimization is needed here. This is not > performance-critical code. That's true. I've now updated the PR to keep it simple and remove that optimization. - PR Review Comment: https://git.openjdk.org/jdk/pull/16919#discussion_r1412058767
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
> Can I please get a review for this change to the test library's > `OutputAnalyzer` class, which proposes to remove some unnecessary logging > from the `getExitValue()` call? > > As noted in https://bugs.openjdk.org/browse/JDK-8321163, right now this > method logs: > > > [2023-11-24T11:47:54.557561Z] Waiting for completion for process 24909 > [2023-11-24T11:47:54.557873Z] Waiting for completion finished for process > 24909 > > > even when the process has already completed and the exit value already known. > The change in this PR makes it such that if the exit value is available then > we no longer log this (nor call `process.waitFor()`). > > No new tests have been added given the nature of this change. tier1, tier2 > and tier3 tests continue to pass with this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: remove micro optimization - Changes: - all: https://git.openjdk.org/jdk/pull/16919/files - new: https://git.openjdk.org/jdk/pull/16919/files/2fc11922..d81f8e12 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16919&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16919&range=01-02 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16919.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16919/head:pull/16919 PR: https://git.openjdk.org/jdk/pull/16919
