Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v6]
On Fri, 9 Oct 2020 06:02:04 GMT, David Holmes wrote: >> Aleksei Voitylov has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now >> contains three commits: >> - Merge branch 'master' into JDK-8247589 >> - JDK-8247589: Implementation of Alpine Linux/x64 Port >> - JDK-8247589: Implementation of Alpine Linux/x64 Port > > Marked as reviewed by dholmes (Reviewer). Thanks everyone! - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Thu, 8 Oct 2020 10:58:56 GMT, Aleksei Voitylov wrote: >> @voitylov For future reference please don't force-push commits on open PRs >> as it breaks the commit history. I can no >> longer just look at the two most recent commits and see what they added >> relative to what I had previously reviewed. >> Thanks. > > @dholmes-ora yes, sorry about that. I updated the branch to pull the recent > changes to enable pre-submit tests and > ensure everything is well before integration and though the branch looked > good it confused the pull request. So I had > to force push it back to the original state. @iignatev I resolved the conflict in whitebox.cpp and fixed a minor style nit on the way. Could you take a look? - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Tue, 6 Oct 2020 02:00:06 GMT, David Holmes wrote: >> I added the contributors that could be found in the portola project commits. >> If anyone knows some other contributors I >> missed, I'll be happy to stand corrected. > > @voitylov For future reference please don't force-push commits on open PRs as > it breaks the commit history. I can no > longer just look at the two most recent commits and see what they added > relative to what I had previously reviewed. > Thanks. @dholmes-ora yes, sorry about that. I updated the branch to pull the recent changes to enable pre-submit tests and ensure everything is well before integration and though the branch looked good it confused the pull request. So I had to force push it back to the original state. - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v6]
> continuing the review thread from here > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html > >> The download side of using JNI in these tests is that it complicates the >> setup a bit for those that run jtreg directly and/or just build the JDK >> and not the test libraries. You could reduce this burden a bit by >> limiting the load library/isMusl check to Linux only, meaning isMusl >> would not be called on other platforms. >> >> The alternative you suggest above might indeed be better. I assume you >> don't mean splitting the tests but rather just adding a second @test >> description so that the vm.musl case runs the test with a system >> property that allows the test know the expected load library path behavior. > > I have updated the PR to split the two tests in multiple @test s. > >> The updated comment in java_md.c in this looks good. A minor comment on >> Platform.isBusybox is Files.isSymbolicLink returning true implies that >> the link exists so no need to check for exists too. Also the >> if-then-else style for the new class in ProcessBuilder/Basic.java is >> inconsistent with the rest of the test so it stands out. > > Thank you, these changes are done in the updated PR. > >> Given the repo transition this weekend then I assume you'll create a PR >> for the final review at least. Also I see JEP 386 hasn't been targeted >> yet but I assume Boris, as owner, will propose-to-target and wait for it >> to be targeted before it is integrated. > > Yes. How can this be best accomplished with the new git workflow? > - we can continue the review process till the end and I will request the > integration to happen only after the JEP is > targeted. I guess this step is now done by typing "slash integrate" in a > comment. > - we can pause the review process now until the JEP is targeted. > > In the first case I'm kindly asking the Reviewers who already chimed in on > that to re-confirm the review here. Aleksei Voitylov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Merge branch 'master' into JDK-8247589 - JDK-8247589: Implementation of Alpine Linux/x64 Port - JDK-8247589: Implementation of Alpine Linux/x64 Port - Changes: https://git.openjdk.java.net/jdk/pull/49/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=49=05 Stats: 403 lines in 30 files changed: 348 ins; 17 del; 38 mod Patch: https://git.openjdk.java.net/jdk/pull/49.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/49/head:pull/49 PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v5]
> continuing the review thread from here > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html > >> The download side of using JNI in these tests is that it complicates the >> setup a bit for those that run jtreg directly and/or just build the JDK >> and not the test libraries. You could reduce this burden a bit by >> limiting the load library/isMusl check to Linux only, meaning isMusl >> would not be called on other platforms. >> >> The alternative you suggest above might indeed be better. I assume you >> don't mean splitting the tests but rather just adding a second @test >> description so that the vm.musl case runs the test with a system >> property that allows the test know the expected load library path behavior. > > I have updated the PR to split the two tests in multiple @test s. > >> The updated comment in java_md.c in this looks good. A minor comment on >> Platform.isBusybox is Files.isSymbolicLink returning true implies that >> the link exists so no need to check for exists too. Also the >> if-then-else style for the new class in ProcessBuilder/Basic.java is >> inconsistent with the rest of the test so it stands out. > > Thank you, these changes are done in the updated PR. > >> Given the repo transition this weekend then I assume you'll create a PR >> for the final review at least. Also I see JEP 386 hasn't been targeted >> yet but I assume Boris, as owner, will propose-to-target and wait for it >> to be targeted before it is integrated. > > Yes. How can this be best accomplished with the new git workflow? > - we can continue the review process till the end and I will request the > integration to happen only after the JEP is > targeted. I guess this step is now done by typing "slash integrate" in a > comment. > - we can pause the review process now until the JEP is targeted. > > In the first case I'm kindly asking the Reviewers who already chimed in on > that to re-confirm the review here. Aleksei Voitylov has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. - Changes: - all: https://git.openjdk.java.net/jdk/pull/49/files - new: https://git.openjdk.java.net/jdk/pull/49/files/5feda5ff..b7ffed87 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=49=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=49=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/49.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/49/head:pull/49 PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v4]
> continuing the review thread from here > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html > >> The download side of using JNI in these tests is that it complicates the >> setup a bit for those that run jtreg directly and/or just build the JDK >> and not the test libraries. You could reduce this burden a bit by >> limiting the load library/isMusl check to Linux only, meaning isMusl >> would not be called on other platforms. >> >> The alternative you suggest above might indeed be better. I assume you >> don't mean splitting the tests but rather just adding a second @test >> description so that the vm.musl case runs the test with a system >> property that allows the test know the expected load library path behavior. > > I have updated the PR to split the two tests in multiple @test s. > >> The updated comment in java_md.c in this looks good. A minor comment on >> Platform.isBusybox is Files.isSymbolicLink returning true implies that >> the link exists so no need to check for exists too. Also the >> if-then-else style for the new class in ProcessBuilder/Basic.java is >> inconsistent with the rest of the test so it stands out. > > Thank you, these changes are done in the updated PR. > >> Given the repo transition this weekend then I assume you'll create a PR >> for the final review at least. Also I see JEP 386 hasn't been targeted >> yet but I assume Boris, as owner, will propose-to-target and wait for it >> to be targeted before it is integrated. > > Yes. How can this be best accomplished with the new git workflow? > - we can continue the review process till the end and I will request the > integration to happen only after the JEP is > targeted. I guess this step is now done by typing "slash integrate" in a > comment. > - we can pause the review process now until the JEP is targeted. > > In the first case I'm kindly asking the Reviewers who already chimed in on > that to re-confirm the review here. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: test2 - Changes: - all: https://git.openjdk.java.net/jdk/pull/49/files - new: https://git.openjdk.java.net/jdk/pull/49/files/705b8555..5feda5ff Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=49=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=49=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/49.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/49/head:pull/49 PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v3]
> continuing the review thread from here > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html > >> The download side of using JNI in these tests is that it complicates the >> setup a bit for those that run jtreg directly and/or just build the JDK >> and not the test libraries. You could reduce this burden a bit by >> limiting the load library/isMusl check to Linux only, meaning isMusl >> would not be called on other platforms. >> >> The alternative you suggest above might indeed be better. I assume you >> don't mean splitting the tests but rather just adding a second @test >> description so that the vm.musl case runs the test with a system >> property that allows the test know the expected load library path behavior. > > I have updated the PR to split the two tests in multiple @test s. > >> The updated comment in java_md.c in this looks good. A minor comment on >> Platform.isBusybox is Files.isSymbolicLink returning true implies that >> the link exists so no need to check for exists too. Also the >> if-then-else style for the new class in ProcessBuilder/Basic.java is >> inconsistent with the rest of the test so it stands out. > > Thank you, these changes are done in the updated PR. > >> Given the repo transition this weekend then I assume you'll create a PR >> for the final review at least. Also I see JEP 386 hasn't been targeted >> yet but I assume Boris, as owner, will propose-to-target and wait for it >> to be targeted before it is integrated. > > Yes. How can this be best accomplished with the new git workflow? > - we can continue the review process till the end and I will request the > integration to happen only after the JEP is > targeted. I guess this step is now done by typing "slash integrate" in a > comment. > - we can pause the review process now until the JEP is targeted. > > In the first case I'm kindly asking the Reviewers who already chimed in on > that to re-confirm the review here. Aleksei Voitylov 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: - Merge branch 'JDK-8247589' of https://github.com/voitylov/jdk into JDK-8247589 - JDK-8247589: Implementation of Alpine Linux/x64 Port - JDK-8247589: Implementation of Alpine Linux/x64 Port - JDK-8247589: Implementation of Alpine Linux/x64 Port - JDK-8247589: Implementation of Alpine Linux/x64 Port - Changes: - all: https://git.openjdk.java.net/jdk/pull/49/files - new: https://git.openjdk.java.net/jdk/pull/49/files/d5994cb5..705b8555 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=49=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=49=01-02 Stats: 73505 lines in 3006 files changed: 26172 ins; 37386 del; 9947 mod Patch: https://git.openjdk.java.net/jdk/pull/49.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/49/head:pull/49 PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Mon, 14 Sep 2020 06:30:50 GMT, Aleksei Voitylov wrote: >> Marked as reviewed by dholmes (Reviewer). > > thank you Alan, Erik, and David! When the JEP becomes Targeted, I'll use this > PR to integrate the changes. I added the contributors that could be found in the portola project commits. If anyone knows some other contributors I missed, I'll be happy to stand corrected. - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Mon, 14 Sep 2020 04:18:39 GMT, David Holmes wrote: >> Aleksei Voitylov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> JDK-8247589: Implementation of Alpine Linux/x64 Port > > Marked as reviewed by dholmes (Reviewer). thank you Alan, Erik, and David! When the JEP becomes Targeted, I'll use this PR to integrate the changes. - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Tue, 8 Sep 2020 23:44:58 GMT, David Holmes wrote:
>> Aleksei Voitylov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8247589: Implementation of Alpine Linux/x64 Port
>
> make/autoconf/platform.m4 line 536:
>
>> 534: AC_SUBST(HOTSPOT_$1_CPU_DEFINE)
>> 535:
>> 536: if test "x$OPENJDK_$1_LIBC" = "xmusl"; then
>
> I'm not clear why we only check for musl when setting the HOTSPOT_$1_LIBC
> variable
this check is removed in the updated version. As a consequence, LIBC variable
is added to the release file for all
platforms, which is probably a good thing.
> src/hotspot/os/linux/os_linux.cpp line 624:
>
>> 622: // confstr() from musl libc returns EINVAL for
>> 623: // _CS_GNU_LIBC_VERSION and _CS_GNU_LIBPTHREAD_VERSION
>> 624: os::Linux::set_libc_version("unknown");
>
> This should be "musl - unknown" as we don't know an exact version but we do
> know that it is musl.
Right, this should be more consistent with glibc which here returns name and
version. Updated as suggested.
> src/hotspot/os/linux/os_linux.cpp line 625:
>
>> 623: // _CS_GNU_LIBC_VERSION and _CS_GNU_LIBPTHREAD_VERSION
>> 624: os::Linux::set_libc_version("unknown");
>> 625: os::Linux::set_libpthread_version("unknown");
>
> This should be "musl - unknown" as we don't know an exact version but we do
> know that it is musl.
The pthread version is also updated to "musl - unknown". Reason being, pthread
functionality for musl is built into the
library.
> src/hotspot/share/runtime/abstract_vm_version.cpp line 263:
>
>> 261: #define LIBC_STR "-" XSTR(LIBC)
>> 262: #else
>> 263: #define LIBC_STR ""
>
> Again I'm not clear why we do nothing in the non-musl case? Shouldn't we be
> reporting glibc or musl?
Unlike the case above, I think it's best to keep it as is. I'd expect there to
be a bunch of scripts in the wild which
parse it and may get broken when facing a triplet for existing platforms.
> src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c line 284:
>
>> 282: // To improve portability across platforms and avoid conflicts
>> 283: // between GNU and XSI versions of strerror_r, plain strerror is
>> used.
>> 284: // It's safe because this code is not used in any multithreaded
>> environment.
>
> I still question this assertion. The issue is not that the current code path
> that leads to strerror use may be executed
> concurrently but that any other strerror use could be concurrent with this
> one. I would consider this a "must fix" if
> not for the fact we already use strerror in the code and so this doesn't
> really change the exposure to the problem.
You are right! The updated version #ifdefs the XSI or GNU versions of
strerror_r in this place. Note to self: file a
bug to address the usage of strerror in other places, at least for HotSpot.
> test/hotspot/jtreg/runtime/StackGuardPages/exeinvoke.c line 282:
>
>> 280:
>> 281: pthread_attr_init(_attr);
>> 282: pthread_attr_setstacksize(_attr, stack_size);
>
> Just a comment in response to the explanation as to why this change is
> needed. If the default thread stacksize under
> musl is insufficient to successfully attach such a thread to the VM then this
> will cause problems for applications that
> embed the VM directly (or which otherwise directly attach existing threads).
This fix
https://git.musl-libc.org/cgit/musl/commit/src/aio/aio.c?id=1a6d6f131bd60ec2a858b34100049f0c042089f2
addresses the problem for recent versions of musl. The test passes on a recent
Alpine Linux 3.11.6 (musl 1.1.24) and
fails on Alpine Linux 3.8.2 (musl 1.1.19) without this test fix.
There are still older versions of the library in the wild, hence the test fix.
The mitigation for such users would be a
distro upgrade.
> test/hotspot/jtreg/runtime/TLS/exestack-tls.c line 60:
>
>> 58: }
>> 59:
>> 60: #if defined(__GLIBC)
>
> Why do we use this form here but at line 30 we have:
> #ifdef __GLIBC__
> ?
Fixed to be consistent.
-
PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
> continuing the review thread from here > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html > >> The download side of using JNI in these tests is that it complicates the >> setup a bit for those that run jtreg directly and/or just build the JDK >> and not the test libraries. You could reduce this burden a bit by >> limiting the load library/isMusl check to Linux only, meaning isMusl >> would not be called on other platforms. >> >> The alternative you suggest above might indeed be better. I assume you >> don't mean splitting the tests but rather just adding a second @test >> description so that the vm.musl case runs the test with a system >> property that allows the test know the expected load library path behavior. > > I have updated the PR to split the two tests in multiple @test s. > >> The updated comment in java_md.c in this looks good. A minor comment on >> Platform.isBusybox is Files.isSymbolicLink returning true implies that >> the link exists so no need to check for exists too. Also the >> if-then-else style for the new class in ProcessBuilder/Basic.java is >> inconsistent with the rest of the test so it stands out. > > Thank you, these changes are done in the updated PR. > >> Given the repo transition this weekend then I assume you'll create a PR >> for the final review at least. Also I see JEP 386 hasn't been targeted >> yet but I assume Boris, as owner, will propose-to-target and wait for it >> to be targeted before it is integrated. > > Yes. How can this be best accomplished with the new git workflow? > - we can continue the review process till the end and I will request the > integration to happen only after the JEP is > targeted. I guess this step is now done by typing "slash integrate" in a > comment. > - we can pause the review process now until the JEP is targeted. > > In the first case I'm kindly asking the Reviewers who already chimed in on > that to re-confirm the review here. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: JDK-8247589: Implementation of Alpine Linux/x64 Port - Changes: - all: https://git.openjdk.java.net/jdk/pull/49/files - new: https://git.openjdk.java.net/jdk/pull/49/files/f61f546a..d5994cb5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=49=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=49=00-01 Stats: 19 lines in 4 files changed: 7 ins; 4 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/49.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/49/head:pull/49 PR: https://git.openjdk.java.net/jdk/pull/49
RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port
continuing the review thread from here https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html > The download side of using JNI in these tests is that it complicates the > setup a bit for those that run jtreg directly and/or just build the JDK > and not the test libraries. You could reduce this burden a bit by > limiting the load library/isMusl check to Linux only, meaning isMusl > would not be called on other platforms. > > The alternative you suggest above might indeed be better. I assume you > don't mean splitting the tests but rather just adding a second @test > description so that the vm.musl case runs the test with a system > property that allows the test know the expected load library path behavior. I have updated the PR to split the two tests in multiple @test s. > The updated comment in java_md.c in this looks good. A minor comment on > Platform.isBusybox is Files.isSymbolicLink returning true implies that > the link exists so no need to check for exists too. Also the > if-then-else style for the new class in ProcessBuilder/Basic.java is > inconsistent with the rest of the test so it stands out. Thank you, these changes are done in the updated PR. > Given the repo transition this weekend then I assume you'll create a PR > for the final review at least. Also I see JEP 386 hasn't been targeted > yet but I assume Boris, as owner, will propose-to-target and wait for it > to be targeted before it is integrated. Yes. How can this be best accomplished with the new git workflow? - we can continue the review process till the end and I will request the integration to happen only after the JEP is targeted. I guess this step is now done by typing "slash integrate" in a comment. - we can pause the review process now until the JEP is targeted. In the first case I'm kindly asking the Reviewers who already chimed in on that to re-confirm the review here. - Commit messages: - JDK-8247589: Implementation of Alpine Linux/x64 Port Changes: https://git.openjdk.java.net/jdk/pull/49/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=49=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8247589 Stats: 403 lines in 30 files changed: 346 ins; 17 del; 40 mod Patch: https://git.openjdk.java.net/jdk/pull/49.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/49/head:pull/49 PR: https://git.openjdk.java.net/jdk/pull/49
Re: serviceability agent : problems when using gcc LTO (link time optimization)
Volker, not a full answer, but here is some static size stats: Server x86_64 AArch64 regular 23M 20M lto 17M 14M Minimal x86_64 AArch64 regular 4.9M 3.9M lto 4.7M 3.6M -Aleksei On 15/01/2020 16:40, Volker Simonis wrote: > While we are speaking about all the drawbacks of LTO, it's still not > clear what the benefits are? In the very first mail Matthias mentioned > that there might be performance improvements but that performance is > not the main driving factor behind this initiative. So is it the > reduced code size (Matthias mentioned something around ~10%)? > > It would be nice to see some real numbers on various platform for > both, the performance improvements for native parts like JIT/GC as > well as for the size reduction. > > Aleksei Voitylov <mailto:aleksei.voity...@bell-sw.com>> schrieb am Di., 14. Jan. 2020, > 09:54: > > > On 14/01/2020 19:57, Baesken, Matthias wrote: > > Hello Magnus and Aleksei, thanks for the input . > > > > The times you provided really look like they make a big > difference at least for people often building minimal-vm . > > Guess I have to measure myself a bit (maybe the difference is > not that big on our linux s390x / ppc64(le) ) . > > > >> If the change to enable lto by default is proposed, what would > be the > >> recommended strategy for development? > >> > > Probably we should a) do not enable it by default but just > make sure it can be enabled easily and works for the minimal-vm > That would be welcome. I have high hopes to LTO the VM some time by > default, and the tendency observed is that the compiler time overhead > for GCC becomes smaller. At the same time there is no reason why > vendors > that invested in testing and can absorb the build time hit could > provide > binaries with LTO built VMs by passing an additional option flag. > > or b) take it easy to disable it for local development. > > > > Best regards, Matthias > > > > > > > >> Magnus, Matthias, > >> > >> for me, lto is a little heavyweight for development. x86_64 > build time > >> with gcc 7: > >> > >> Server 1m32.484s > >> Server+Minimal 1m42.166s > >> Server+Minimal (--with-jvm-features="link-time-opt") 5m29.422s > >> > >> If the change to enable lto by default is proposed, what would > be the > >> recommended strategy for development? > >> > >> For ARM32 Minimal, please keep in mind that it's not uncommon > to disable > >> LTO plugin in commodity ARM32 gcc compiler distributions, so > for some it > >> does not matter what settings we have in OpenJDK. I believe > there could > >> be other reasons for that on top of build time (bugs?). > >> >
Re: serviceability agent : problems when using gcc LTO (link time optimization)
On 14/01/2020 19:57, Baesken, Matthias wrote: > Hello Magnus and Aleksei, thanks for the input . > > The times you provided really look like they make a big difference at least > for people often building minimal-vm . > Guess I have to measure myself a bit (maybe the difference is not that big > on our linux s390x / ppc64(le) ) . > >> If the change to enable lto by default is proposed, what would be the >> recommended strategy for development? >> > Probably we should a) do not enable it by default but just make sure it > can be enabled easily and works for the minimal-vm That would be welcome. I have high hopes to LTO the VM some time by default, and the tendency observed is that the compiler time overhead for GCC becomes smaller. At the same time there is no reason why vendors that invested in testing and can absorb the build time hit could provide binaries with LTO built VMs by passing an additional option flag. > or b) take it easy to disable it for local development. > > Best regards, Matthias > > > >> Magnus, Matthias, >> >> for me, lto is a little heavyweight for development. x86_64 build time >> with gcc 7: >> >> Server 1m32.484s >> Server+Minimal 1m42.166s >> Server+Minimal (--with-jvm-features="link-time-opt") 5m29.422s >> >> If the change to enable lto by default is proposed, what would be the >> recommended strategy for development? >> >> For ARM32 Minimal, please keep in mind that it's not uncommon to disable >> LTO plugin in commodity ARM32 gcc compiler distributions, so for some it >> does not matter what settings we have in OpenJDK. I believe there could >> be other reasons for that on top of build time (bugs?). >>
Re: serviceability agent : problems when using gcc LTO (link time optimization)
Magnus, Matthias, for me, lto is a little heavyweight for development. x86_64 build time with gcc 7: Server 1m32.484s Server+Minimal 1m42.166s Server+Minimal (--with-jvm-features="link-time-opt") 5m29.422s If the change to enable lto by default is proposed, what would be the recommended strategy for development? For ARM32 Minimal, please keep in mind that it's not uncommon to disable LTO plugin in commodity ARM32 gcc compiler distributions, so for some it does not matter what settings we have in OpenJDK. I believe there could be other reasons for that on top of build time (bugs?). -Aleksei On 14/01/2020 17:04, Magnus Ihse Bursie wrote: > On 2020-01-14 13:49, Baesken, Matthias wrote: >> >> Hi Magnus, thanks for the info , I already noticed yesterday the >> setting for arm-32 in the minimal build . >> >> Do you think we could set it too for the other Linux platforms in >> the minimal build ( serviceability agent is not supported there as >> well so the observed issue wouldn’t be a problem). >> > > You mean if you could enable it on your builds without any issues? I'd > guess so, but I don't know. Just try it: > --with-jvm-features="link-time-opt". > > If you mean that it should be turned on by default on minimal builds > for all platforms? No, I don't think that's a good idea. The link time > is really a killer. I remember arm-32 going from like a couple of > minutes to half an hour for linking libjvm.so. > > Things might be different with gold, though. I know they have done > work with at least some kind of "lightweight" LTO, that might be worth > at least looking into. > > /Magnus > > >> Best regards, Matthias >> >> On 2020-01-10 11:01, Baesken, Matthias wrote: >> >> Hello, I recently looked into the gcc lto optimization mode >> (see for some >> detailshttps://gcc.gnu.org/onlinedocs/gccint/LTO-Overview.html >> andhttp://hubicka.blogspot.com/2019/05/gcc-9-link-time-and-inter-procedural.html >> ). >> >> This mode can lead to more compact binaries (~10% smaller) , it >> also might bring small performance improvements but that wasn't my >> (main) goal . >> >> The changes for this are rather small , one needs to use a recent >> gcc , add -flto to the compile flags , for example >> >> --- a/make/autoconf/flags-cflags.m4 Wed Jan 01 03:08:45 2020 >> +0100 >> >> +++ b/make/autoconf/flags-cflags.m4 Wed Jan 08 17:39:10 2020 +0100 >> >> @@ -530,8 +530,13 @@ >> >> fi >> >> if test "x$TOOLCHAIN_TYPE" = xgcc; then >> >> - TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new >> -fstack-protector" >> >> - TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector" >> >> + TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new >> -fstack-protector -flto" >> >> + TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector -flto" >> >> and you have to make sure to use gcc-ar and gcc-nm >> instead of ar / nm . >> >> Build and test(s) work, however with one exception. >> >> The serviceability tests like serviceability/sa seems to >> rely heavily on the "normal" structure of libjvm.so (from >> what I understand e.g. in LinuxVtblAccess it is attempted to >> access internal symbols like _ZTV ). >> >> Errors in the sa tests look like : >> >> java.lang.InternalError: Metadata does not appear to be polymorphic >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.types.basic.BasicTypeDataBase.findDynamicTypeForAddress(BasicTypeDataBase.java:279) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.runtime.VirtualBaseConstructor.instantiateWrapperFor(VirtualBaseConstructor.java:102) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.oops.Metadata.instantiateWrapperFor(Metadata.java:74) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.memory.SystemDictionary.getClassLoaderKlass(SystemDictionary.java:96) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.printClassLoaderStatistics(ClassLoaderStats.java:93) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.run(ClassLoaderStats.java:78) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.run(JMap.java:115) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.startInternal(Tool.java:262) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.start(Tool.java:225) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.execute(Tool.java:118) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.main(JMap.java:176) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.runJMAP(SALauncher.java:321) >> >> at >> jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.main(SALauncher.java:406) >> >> Has anyone experimented with LTO optimization ? >> >> >> Hi Matthias, >> >> We used to have LTO enabled on the old, closed-source Oracle arm-32 >> builds.
