Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v3]
On Tue, 11 Mar 2025 15:19:24 GMT, Matthias Baesken wrote: > > Was this always redundant, and does removing it make no change to current > > build options? > > If so, great, let's remove the useless makefile lines. > > There was a bit of discussion before in the thread > https://mail.openjdk.org/pipermail/build-dev/2025-March/049419.html ' > LIBMANAGEMENT_OPTIMIZATION special settings on Linux with debug symbols' and > the outcome was that removing makes sense. My conclusion in that thread was that [JDK-7071907](https://bugs.openjdk.org/browse/JDK-7071907) introduced this (for a number of libraries). Before that change, these libraries were built with optimization HIGH, and after, because we (Oracle and I assume most other distributors) always build with debug symbols enabled, the optimization level for these libraries was de facto changed to LOW. This PR would revert that, so it does indeed change how the build behaves. There is at least some historic precedent for changing it this way, even if it was 13 years ago. I think what Kevin is after is having this explanation made clear in the bug and PR description so that it's made clear what the change is and intends to do. - PR Comment: https://git.openjdk.org/jdk/pull/23966#issuecomment-2714883676
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v2]
On Tue, 11 Mar 2025 11:54:29 GMT, Kevin Walls wrote: > Is it worth making any change here? This was needed because I removed DISABLED_WARNINGS_gcc_VMManagementImpl.c while changing the makefile. - PR Review Comment: https://git.openjdk.org/jdk/pull/23966#discussion_r1989296776
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v2]
On Tue, 11 Mar 2025 09:04:59 GMT, Matthias Baesken wrote:
>> On Linux there are some special settings for LIBMANAGEMENT_OPTIMIZATION that
>> are most likely not needed any more and could be removed.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Bring back comment
src/java.management/share/native/libmanagement/VMManagementImpl.c line 63:
> 61: {
> 62: jmmOptionalSupport mos;
> 63: jmm_interface->GetOptionalSupport(env, &mos);
Is it worth making any change here?
We currently ignore the return value from GetOptionalSupport, and no doubt have
done for years.
So is the fix to just not record the return value, or should we check it?
Making a change to not capture the return value looks like a statement that it
should never be checked. Even if GetOptionalSupport "can't" really fail with
the current implementation, that doesn't seem like the right hint to leave.
Other usage in
Java_com_sun_management_internal_DiagnosticCommandImpl_getDiagnosticCommandInfo
also does:
jint ret = jmm_interface_management_ext->GetOptionalSupport(env, &mos);
...and also doesn't check the return value.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23966#discussion_r1989081708
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v2]
On Tue, 11 Mar 2025 13:18:55 GMT, Kevin Walls wrote: > Windows fastdebug and release I just checked and saw -O1, I'm not sure why > that is. The change touches only Linux so Windows stays as it is. > We do the same thing in make/modules/jdk.management/Lib.gmk so both these > management locations should probably be treated the same. I look into this. - PR Review Comment: https://git.openjdk.org/jdk/pull/23966#discussion_r1989318978
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v3]
On Tue, 11 Mar 2025 14:14:16 GMT, Matthias Baesken wrote: >> On Linux there are some special settings for LIBMANAGEMENT_OPTIMIZATION that >> are most likely not needed any more and could be removed. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust jdk.management/Lib.gmk Hi, I wasn't saying this is wrong, but that the change should say how it makes things better. The issues says there are some things most likely not needed. We should be a bit clearer than that, like: The removal of `ifeq ($(call isTargetOs, linux)+$(COMPILE_WITH_DEBUG_SYMBOLS), true+true)` Was this always redundant, and does removing it make no change to current build options? If so, great, let's remove the useless makefile lines. Remove a compiler directive to avoid unused var warnings, but change the code to make it imply a method has no return value when actually it returns a value: I think you could argue this either way, so I asked if it's really worthwhile. - PR Comment: https://git.openjdk.org/jdk/pull/23966#issuecomment-2714681001
Integrated: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 10:39:27 GMT, snake66 wrote: > Replace hardcoded instances of `-lpthread` with `$(LIBPTHREAD)`, so that it's > possible to parameterize this for platforms that use different flags for > enabling posix threads. > > This work is a continuation of the work done by Greg Lewis in [1], but > generalized for the full JDK, and set at the configure stage. > > Sponsored by: The FreeBSD Foundation > Co-authored-by: Greg Lewis > > [1]: > https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 This pull request has now been integrated. Changeset: b957e5ed Author:Harald Eilertsen URL: https://git.openjdk.org/jdk/commit/b957e5ed1a8b77e01aad1bb574e4914131cdbfa6 Stats: 660 lines in 9 files changed: 9 ins; 0 del; 651 mod 8351322: Parameterize link option for pthreads Reviewed-by: erikj, ihse, dholmes - PR: https://git.openjdk.org/jdk/pull/23930
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v3]
On Tue, 11 Mar 2025 15:04:58 GMT, Kevin Walls wrote: > Remove a compiler directive to avoid unused var warnings, but change the code > to make it imply a method has no return value when actually it returns a > value: I think you could argue this either way, so I asked if it's really > worthwhile. I can move this into a separate PR, maybe it is indeed much better to separate in because it isn't related to the OPTIMIZATION settings. Looking at the coding we might get '-1' back https://github.com/openjdk/jdk/blob/af9af7e90f7dab5adc7b89b76eb978d269e863de/src/hotspot/share/services/management.cpp#L491 so checking for the return code is probably a good idea (also at the other code location you mentioned). - PR Comment: https://git.openjdk.org/jdk/pull/23966#issuecomment-2714714952
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v3]
On Tue, 11 Mar 2025 15:04:58 GMT, Kevin Walls wrote: > Was this always redundant, and does removing it make no change to current > build options? > If so, great, let's remove the useless makefile lines. There was a bit of discussion before in the thread https://mail.openjdk.org/pipermail/build-dev/2025-March/049419.html ' LIBMANAGEMENT_OPTIMIZATION special settings on Linux with debug symbols' and the outcome was that removing makes sense. - PR Comment: https://git.openjdk.org/jdk/pull/23966#issuecomment-2714735126
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v2]
On Mon, 10 Mar 2025 18:51:53 GMT, Matthias Baesken wrote: >> make/modules/java.management/Lib.gmk line 33: >> >>> 31: ## Build libmanagement >>> 32: >>> >>> 33: >> >> Why remove the comment header. This pattern is used throughout the >> `make/modules/*/Lib.gmk` files. > > My motivation was that the comment brings not much value because it just > states the obvious. But if you like I can bring back the comment. I brought back the comment, maybe it is better to keep it because of consistency. - PR Review Comment: https://git.openjdk.org/jdk/pull/23966#discussion_r1988749705
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v2]
> On Linux there are some special settings for LIBMANAGEMENT_OPTIMIZATION that > are most likely not needed any more and could be removed. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Bring back comment - Changes: - all: https://git.openjdk.org/jdk/pull/23966/files - new: https://git.openjdk.org/jdk/pull/23966/files/e6b748d4..fbe7f56e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23966&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23966&range=00-01 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/23966.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23966/head:pull/23966 PR: https://git.openjdk.org/jdk/pull/23966
Re: RFR: 8351322: Parameterize link option for pthreads [v2]
On Sat, 8 Mar 2025 13:39:44 GMT, snake66 wrote: >> Replace hardcoded instances of `-lpthread` with `$(LIBPTHREAD)`, so that >> it's possible to parameterize this for platforms that use different flags >> for enabling posix threads. >> >> This work is a continuation of the work done by Greg Lewis in [1], but >> generalized for the full JDK, and set at the configure stage. >> >> Sponsored by: The FreeBSD Foundation >> Co-authored-by: Greg Lewis >> >> [1]: >> https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 > > snake66 has updated the pull request incrementally with three additional > commits since the last revision: > > - Use shell var syntax in libraries.m4 > - Fix typo PTREAD->PTHREAD > - Revert changes to make/Hsdis.gmk This looks good now. Thanks! - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23930#pullrequestreview-2670569844
Re: RFR: 8351322: Parameterize link option for pthreads [v2]
On Sat, 8 Mar 2025 13:39:44 GMT, snake66 wrote: >> Replace hardcoded instances of `-lpthread` with `$(LIBPTHREAD)`, so that >> it's possible to parameterize this for platforms that use different flags >> for enabling posix threads. >> >> This work is a continuation of the work done by Greg Lewis in [1], but >> generalized for the full JDK, and set at the configure stage. >> >> Sponsored by: The FreeBSD Foundation >> Co-authored-by: Greg Lewis >> >> [1]: >> https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 > > snake66 has updated the pull request incrementally with three additional > commits since the last revision: > > - Use shell var syntax in libraries.m4 > - Fix typo PTREAD->PTHREAD > - Revert changes to make/Hsdis.gmk @snake66 Your change (at version 469997239f26f59cd47df19fb9e836b883687487) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/23930#issuecomment-2711042802
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v2]
On Tue, 11 Mar 2025 13:36:42 GMT, Matthias Baesken wrote:
>> src/java.management/share/native/libmanagement/VMManagementImpl.c line 63:
>>
>>> 61: {
>>> 62: jmmOptionalSupport mos;
>>> 63: jmm_interface->GetOptionalSupport(env, &mos);
>>
>> Is it worth making any change here?
>>
>> We currently ignore the return value from GetOptionalSupport, and no doubt
>> have done for years.
>> So is the fix to just not record the return value, or should we check it?
>>
>> Making a change to not capture the return value looks like a statement that
>> it should never be checked. Even if GetOptionalSupport "can't" really fail
>> with the current implementation, that doesn't seem like the right hint to
>> leave.
>>
>> Other usage in
>> Java_com_sun_management_internal_DiagnosticCommandImpl_getDiagnosticCommandInfo
>> also does:
>> jint ret = jmm_interface_management_ext->GetOptionalSupport(env, &mos);
>>
>> ...and also doesn't check the return value.
>
>> Is it worth making any change here?
>
> This was needed because I removed
> DISABLED_WARNINGS_gcc_VMManagementImpl.c
> while changing the makefile.
> Other usage in
> Java_com_sun_management_internal_DiagnosticCommandImpl_getDiagnosticCommandInfo
> also does: jint ret = jmm_interface_management_ext->GetOptionalSupport(env,
> &mos);
>
> ...and also doesn't check the return value.
Seems this one needs the warning disabling because of the unused variable
DISABLED_WARNINGS_gcc_DiagnosticCommandImpl.c := unused-variable
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23966#discussion_r1989338573
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v2]
On Tue, 11 Mar 2025 09:04:59 GMT, Matthias Baesken wrote: >> On Linux there are some special settings for LIBMANAGEMENT_OPTIMIZATION that >> are most likely not needed any more and could be removed. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Bring back comment make/modules/java.management/Lib.gmk line 35: > 33: > 34: LIBMANAGEMENT_OPTIMIZATION := HIGH > 35: ifeq ($(call isTargetOs, linux)+$(COMPILE_WITH_DEBUG_SYMBOLS), true+true) On removal of `ifeq ($(call isTargetOs, linux)+$(COMPILE_WITH_DEBUG_SYMBOLS), true+true)` ..are we saying this is redundant? It reads like Linux builds with LOW, and this change will change that to HIGH ? I tested existing build and see -O2 in Linux fastdebug and release builds. So this ifeq wasn't doing anything? Windows fastdebug and release I just checked and saw -O1, I'm not sure why that is. We do the same thing in make/modules/jdk.management/Lib.gmk so both these management locations should probably be treated the same. (The same comparison is in make/modules/java.base/lib/CoreLibraries.gmk affecting LIBVERIFY_OPTIMIZATION, but no need to expand this change beyond the management area.) - PR Review Comment: https://git.openjdk.org/jdk/pull/23966#discussion_r1989256782
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v3]
> On Linux there are some special settings for LIBMANAGEMENT_OPTIMIZATION that > are most likely not needed any more and could be removed. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Adjust jdk.management/Lib.gmk - Changes: - all: https://git.openjdk.org/jdk/pull/23966/files - new: https://git.openjdk.org/jdk/pull/23966/files/fbe7f56e..2aa878e8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23966&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23966&range=01-02 Stats: 6 lines in 1 file changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/23966.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23966/head:pull/23966 PR: https://git.openjdk.org/jdk/pull/23966
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings [v3]
On Tue, 11 Mar 2025 14:14:16 GMT, Matthias Baesken wrote: >> On Linux there are some special settings for LIBMANAGEMENT_OPTIMIZATION that >> are most likely not needed any more and could be removed. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust jdk.management/Lib.gmk Yes - this might be a really good change, to move the optimization level up, but we need to be clear what is happening and why, in case it throws up some new issues. Yes, I would agree with handling the unused variable changes separately. If somebody finds a problem with the opt level, it can be reverted without other complications. (and separately, maybe it's worth handling the return value of GetOptionalSupport, which in our implementation really can't be -1 unless you call it wrongly, but strange things happen and things change 8-) ) I can do some tiers of CI testing to see if any odd issues pop up. - PR Comment: https://git.openjdk.org/jdk/pull/23966#issuecomment-2715367338
RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings
On Linux there are some special settings for LIBMANAGEMENT_OPTIMIZATION that are most likely not needed any more and could be removed. - Commit messages: - JDK-8351542 Changes: https://git.openjdk.org/jdk/pull/23966/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23966&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8351542 Stats: 17 lines in 2 files changed: 0 ins; 14 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/23966.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23966/head:pull/23966 PR: https://git.openjdk.org/jdk/pull/23966
Re: RFR: 8351322: Parameterize link option for pthreads [v2]
On Fri, 7 Mar 2025 00:18:14 GMT, David Holmes wrote: >>> What is the intended way of using this? Do you run make with >>> LIBPTHREAD=-pthread or do you apply a patch on libraries.m4 for the >>> specific way of linking to pthread? >> >> This is in preparation of the upcoming BSD port, which uses `-pthread` >> instead of `-pthread`. It was me who suggested that this is done separately >> with the existing code, to minimize the patch of the BSD port. > > @magicus why can't we just use `-pthread` everywhere? My recollection is that > `-pthread` both sets compiler directives needed for pthread programming and > links to libpthread, so it seems to be what we should be using. ?? Another follow-up is if it would hurt to include $LIBPTHREAD for *all* Hotspot tests, to avoid the huge list. @dholmes-ora Do you have anything coming to mind directly that would make that infeasible, or is it just a matter of testing to add it and see if any tests fail? - PR Comment: https://git.openjdk.org/jdk/pull/23930#issuecomment-2710217007
RFR: 8351323: Parameterize compiler and linker flags for iconv
Allows for future support for platforms that require different flags for libiconv support. Sponsored-by: The FreeBSD Foundation - Commit messages: - 8351323: Parameterize libiconv compiler and linker flags Changes: https://git.openjdk.org/jdk/pull/23995/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23995&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8351323 Stats: 37 lines in 5 files changed: 31 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/23995.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23995/head:pull/23995 PR: https://git.openjdk.org/jdk/pull/23995
Re: RFR: 8351542: LIBMANAGEMENT_OPTIMIZATION remove special optimization settings
On Mon, 10 Mar 2025 17:32:07 GMT, Erik Joelsson wrote: >> On Linux there are some special settings for LIBMANAGEMENT_OPTIMIZATION that >> are most likely not needed any more and could be removed. > > make/modules/java.management/Lib.gmk line 33: > >> 31: ## Build libmanagement >> 32: >> >> 33: > > Why remove the comment header. This pattern is used throughout the > `make/modules/*/Lib.gmk` files. My motivation was that the comment brings not much value because it just states the obvious. But if you like I can bring back the comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/23966#discussion_r1987847982
Re: RFR: 8351323: Parameterize compiler and linker flags for iconv
On Tue, 11 Mar 2025 19:35:45 GMT, Erik Joelsson wrote: >> Allows for future support for platforms that require different flags for >> libiconv support. >> >> Sponsored-by: The FreeBSD Foundation > > I think this looks ok, but please wait for Magnus to have a look too. @erikj79 Thanks for the review. I'll wait for Magnus as well, and for the tests to pass :) One thing I was wondering about is that with this change I think it should be safe to just use `$(ICONV_CFLAGS)` etc when setting the main variable. I.e. instead of: CFLAGS := $(LIBSPLASHSCREEN_CFLAGS) \ $(GIFLIB_CFLAGS) $(LIBJPEG_CFLAGS) $(PNG_CFLAGS) $(LIBZ_CFLAGS), \ CFLAGS_aix := $(ICONV_CFLAGS), \ CFLAGS_macosx := $(ICONV_CFLAGS), \ We could just do: CFLAGS := $(LIBSPLASHSCREEN_CFLAGS) \ $(GIFLIB_CFLAGS) $(LIBJPEG_CFLAGS) $(PNG_CFLAGS) $(LIBZ_CFLAGS) \ $(ICONV_CFLAGS), \ The reason I kept it separate for now is that it used separate assignments for the LDFLAGS variable per platform. However, it you consider it better to combine it into one platform neutral assignment, I can do that instead. - PR Comment: https://git.openjdk.org/jdk/pull/23995#issuecomment-2715520647
Re: RFR: 8351323: Parameterize compiler and linker flags for iconv
On Tue, 11 Mar 2025 19:22:34 GMT, snake66 wrote: > Allows for future support for platforms that require different flags for > libiconv support. > > Sponsored-by: The FreeBSD Foundation I think this looks ok, but please wait for Magnus to have a look too. - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23995#pullrequestreview-2675894294
Re: RFR: 8319055: JCMD should not buffer the whole output of commands [v5]
On Fri, 7 Mar 2025 01:46:37 GMT, Alex Menkov wrote: >> The fix implements streaming output support for attach protocol. >> See JBS issue for evaluation, summary of the changes in the 1st comment. >> Testing: tier1..4,hs-tier5-svc > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > feedback +1 - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23405#pullrequestreview-2671094029
Re: RFR: 8350939: Revisit Windows PDH buffer size calculation for OperatingSystemMXBean
On Mon, 3 Mar 2025 17:08:21 GMT, Kevin Walls wrote: > We calculate a size (length of the counter in characters), which might be > '\Process(java#0)% Processor Time', 33 chars. Isn't that 32 characters? I understood the rest of the explanation in this PR and the change sounds reasonable to me. It's just this character count that I'm unsure about. Is that a typo? - PR Comment: https://git.openjdk.org/jdk/pull/23861#issuecomment-2703849110
