Re: RFR: 8273459: Update code segment alignment to 64 bytes [v2]
> Change the default code entry alignment to 64 bytes from 32 bytes. This > allows for maintaining proper 64-byte alignment of data within a code > segment, which is required by several AVX-512 instructions. > > I ran into this while implementing Base64 encoding and decoding. Code > segments which were allocated with the address mod 32 == 0 but with the > address mod 64 != 0 would cause the align() macro to misalign. This is > because the align macro aligns to the size of the code segment and not the > offset of the PC. So align(64) would align the PC to a multiple of 64 bytes > from the start of the segment, and not to a pure 64-byte boundary as > requested. Changing the alignment of the segment to 64 bytes fixes the issue. > > I have not seen any measurable difference in either performance or memory > usage with the tests I have run. > > See [this > ](https://mail.openjdk.java.net/pipermail/hotspot-dev/2021-August/054180.html) > article for the discussion thread. Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision: Revert alignment of 64-bytes; Add align64() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5547/files - new: https://git.openjdk.java.net/jdk/pull/5547/files/3c7e8db3..f6eefd34 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5547=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5547=00-01 Stats: 21 lines in 4 files changed: 6 ins; 0 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/5547.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5547/head:pull/5547 PR: https://git.openjdk.java.net/jdk/pull/5547
Re: RFR: 8273459: Update code segment alignment to 64 bytes
On Thu, 16 Sep 2021 13:52:24 GMT, Scott Gibbons wrote: > Change the default code entry alignment to 64 bytes from 32 bytes. This > allows for maintaining proper 64-byte alignment of data within a code > segment, which is required by several AVX-512 instructions. > > I ran into this while implementing Base64 encoding and decoding. Code > segments which were allocated with the address mod 32 == 0 but with the > address mod 64 != 0 would cause the align() macro to misalign. This is > because the align macro aligns to the size of the code segment and not the > offset of the PC. So align(64) would align the PC to a multiple of 64 bytes > from the start of the segment, and not to a pure 64-byte boundary as > requested. Changing the alignment of the segment to 64 bytes fixes the issue. > > I have not seen any measurable difference in either performance or memory > usage with the tests I have run. > > See [this > ](https://mail.openjdk.java.net/pipermail/hotspot-dev/2021-August/054180.html) > article for the discussion thread. Reverted `CodeEntryAlignment` to 32 bytes. Added `align64()` function and updated references to `align(64)`. - PR: https://git.openjdk.java.net/jdk/pull/5547
Re: RFR: 8273459: Update code segment alignment to 64 bytes
On Thu, 16 Sep 2021 13:52:24 GMT, Scott Gibbons wrote: > Change the default code entry alignment to 64 bytes from 32 bytes. This > allows for maintaining proper 64-byte alignment of data within a code > segment, which is required by several AVX-512 instructions. > > I ran into this while implementing Base64 encoding and decoding. Code > segments which were allocated with the address mod 32 == 0 but with the > address mod 64 != 0 would cause the align() macro to misalign. This is > because the align macro aligns to the size of the code segment and not the > offset of the PC. So align(64) would align the PC to a multiple of 64 bytes > from the start of the segment, and not to a pure 64-byte boundary as > requested. Changing the alignment of the segment to 64 bytes fixes the issue. > > I have not seen any measurable difference in either performance or memory > usage with the tests I have run. > > See [this > ](https://mail.openjdk.java.net/pipermail/hotspot-dev/2021-August/054180.html) > article for the discussion thread. I am back from vacation! I want to point out that when we generate code for these stubs we don't move them in CodeCache (in contrast to compiled methods): https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/stubRoutines.cpp#L268 `BufferBlob::create()` allocates specified space (of size `code_size2`, for example) directly in CodeCache in `NonNMethod` section: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/code/codeBlob.cpp#L232 https://github.com/openjdk/jdk/blob/master/src/hotspot/share/code/codeBlob.cpp#L272 Based on that, using `pc()` and new `align64()` should be fine for these few cases. - PR: https://git.openjdk.java.net/jdk/pull/5547
Re: RFR: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields [v2]
> This is an initial PR for expanded lint warnings done under two bugs:
>
> 8202056: Expand serial warning to check for bad overloads of serial-related
> methods and ineffectual fields
> 8160675: Issue lint warning for non-serializable non-transient instance
> fields in serializable type
>
> to get feedback on the general approach and test strategy before further
> polishing the implementation.
>
> The implementation initially started as an annotation processor I wrote
> several years ago. The refined version being incorporated into Attr has been
> refactored, had its checks expanded, and been partially ported to idiomatic
> javac coding style rather than using the javax.lang.model API from annotation
> processing.
>
> Subsequent versions of this PR are expected to move the implementation closer
> to idiomatic javac, in particular to use javac flags rather than
> javax.lang.model.Modifier's. Additional resources keys will be defined for
> the serialization-related fields and methods not having the expected
> modifiers, types, etc. The resource keys for the existing checks related to
> serialVersionUID and reused.
>
> Please also review the corresponding CSRs:
>
> https://bugs.openjdk.java.net/browse/JDK-8274335
> https://bugs.openjdk.java.net/browse/JDK-8274336
>
> Informative serialization-related warning messages must take into account
> whether a class, interface, annotation, record, and enum is being analyzed.
> Enum classes and record classes have special handling in serialization. This
> implementation under review has been augmented with checks for interface
> types recommended by Chris Hegarty in an attachment on 8202056.
>
> The JDK build has the Xlint:serial check enabled. The build did not pass with
> the augmented checks. For most modules, this PR contains the library changes
> necessary for the build to pass. I will start separate PRs in those library
> areas to get the needed SuppressWarning("serial") or other changes in place.
> For one module, I temporarily disabled the Xlint:serial check.
>
> In terms of performance, I have not done benchmarks of the JDK build with and
> without these changes, but informally the build seems to take about as long
> as before.
Joe Darcy 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 30 additional commits since the
last revision:
- Merge branch 'master' into JDK-8202056
- Appease jcheck
- Implement checks chegar recommended for interfaces.
- Update comment.
- Add tests for instance field checks.
- Clean build with instance field checks in place.
- Merge branch 'master' into JDK-8202056
- Put Externalizable checks last.
- Add checks for constructor access in Serializable classes.
- Add no-arg ctor check for Externalizable classes.
- ... and 20 more: https://git.openjdk.java.net/jdk/compare/fa1a96de...053de6bb
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5709/files
- new: https://git.openjdk.java.net/jdk/pull/5709/files/d498ff5f..053de6bb
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=5709=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=5709=00-01
Stats: 469 lines in 32 files changed: 252 ins; 70 del; 147 mod
Patch: https://git.openjdk.java.net/jdk/pull/5709.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5709/head:pull/5709
PR: https://git.openjdk.java.net/jdk/pull/5709
Re: RFR: 8273459: Update code segment alignment to 64 bytes
On Fri, 17 Sep 2021 14:00:44 GMT, Scott Gibbons wrote: >> Change the default code entry alignment to 64 bytes from 32 bytes. This >> allows for maintaining proper 64-byte alignment of data within a code >> segment, which is required by several AVX-512 instructions. >> >> I ran into this while implementing Base64 encoding and decoding. Code >> segments which were allocated with the address mod 32 == 0 but with the >> address mod 64 != 0 would cause the align() macro to misalign. This is >> because the align macro aligns to the size of the code segment and not the >> offset of the PC. So align(64) would align the PC to a multiple of 64 bytes >> from the start of the segment, and not to a pure 64-byte boundary as >> requested. Changing the alignment of the segment to 64 bytes fixes the >> issue. >> >> I have not seen any measurable difference in either performance or memory >> usage with the tests I have run. >> >> See [this >> ](https://mail.openjdk.java.net/pipermail/hotspot-dev/2021-August/054180.html) >> article for the discussion thread. > > I think I have not made the point clearly enough. The `align` function is > used to manipulate the address bits for the byte following the `align()`. > This means that wherever the code is copied, the address of that byte should > have the appropriate address bit configuration in the copy (as well as the > original, of course). Since the current implementation is using the base > address of the allocated segment to determine alignment, the only way to > ensure the proper bit configuration of the address is to ensure the base > address of the newly-allocated segment is aligned identically to the original. > > I believe this is entirely independent of `MaxVectorSize`, so I don't believe > it's appropriate to use this value for address alignment. Using `pc()` fixes > the case in the source segment, but will break 50% of the time when the > segment is copied with a `CodeEntryAlignment` of 32. > > I think the bottom line is that `align()` is broken for any value greater > than `CodeEntryAlignment`. I can foresee a case where it may be beneficial > (from an algorithm perspective) to have large alignment values, like > align(256) to simplify pointer arithmetic (for example). All of these > proposed changes will not ensure this alignment when a segment is copied. > > Perhaps the appropriate thing to do is to put an `assert()` in `align()` to > fail if the requested alignment cannot be ensured? > > IMHO, the "right" thing to do is to mark the bytes requiring address > alignment and handle the cases on copy. This would add significant > complexity, however. @asgibbons To me Vladimir Kozlov's suggestion of adding a align64() method calling pc() as you originally proposed looks the best. It meets our purpose and is limited in scope. - PR: https://git.openjdk.java.net/jdk/pull/5547
Integrated: 8274345: make build-test-lib is broken
On Mon, 27 Sep 2021 03:26:54 GMT, xpbob wrote: > 8274345: make build-test-lib is broken This pull request has now been integrated. Changeset: 0865120e Author:bobpengxie Committer: Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/0865120e95f31f3c84282613860e9198a7d3003c Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod 8274345: make build-test-lib is broken Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/5710
Integrated: 8267636: Bump minimum boot jdk to JDK 17
On Wed, 22 Sep 2021 20:03:55 GMT, Mikael Vidstedt wrote: > With the JDK 17 GA out it's time to bump the minimum boot JDK version for > mainline/JDK 18. > > Testing: tier1-5, GHA builds This pull request has now been integrated. Changeset: 75404ea2 Author:Mikael Vidstedt URL: https://git.openjdk.java.net/jdk/commit/75404ea25ed5ed77fda41afc6662b1fe7ea2fb43 Stats: 23 lines in 3 files changed: 0 ins; 9 del; 14 mod 8267636: Bump minimum boot jdk to JDK 17 Reviewed-by: darcy, erikj, iris - PR: https://git.openjdk.java.net/jdk/pull/5639
Re: RFR: JDK-8274170: Add hooks for custom makefiles to augment jtreg test execution
On Thu, 23 Sep 2021 22:02:07 GMT, Mikhailo Seledtsov wrote: > Please review this small change that adds hook for custom makefiles to > augment parameters for jtreg test execution. Thank you Erik ! - PR: https://git.openjdk.java.net/jdk/pull/5666
Integrated: JDK-8274170: Add hooks for custom makefiles to augment jtreg test execution
On Thu, 23 Sep 2021 22:02:07 GMT, Mikhailo Seledtsov wrote: > Please review this small change that adds hook for custom makefiles to > augment parameters for jtreg test execution. This pull request has now been integrated. Changeset: 14100d55 Author:Mikhailo Seledtsov URL: https://git.openjdk.java.net/jdk/commit/14100d55dc822a7eb4f3e499aa9077e7ad17b2a6 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod 8274170: Add hooks for custom makefiles to augment jtreg test execution Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/5666
Integrated: JDK-8274311: Make build.tools.jigsaw.GenGraphs more configurable
On Fri, 24 Sep 2021 23:07:33 GMT, Mandy Chung wrote: > GenGraphs tool generates the module graph. It currently supports the > configuration via javadoc-graphs.properties. However, > `make/jdk/src/classes/build/tools/jigsaw/javadoc-graphs.properties` only > documents two properties. It should be updated to cover all configurable > properties. > > There are a couple other properties not configurable such as nodesep and node > margin. This extends the configuration to allow to set additional properties. > > This also fixes `requiresMandatedColor` in javadoc-graphs.properties to light > gray to match the default configuration in the implementation, i.e. the color > of the edge to java.base. It seems a bug that was unnoticed until Alex and > Iris spotted it. This pull request has now been integrated. Changeset: daaa47e2 Author:Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/daaa47e2005cfa1d72f94a32e7756255f24c4d1f Stats: 167 lines in 3 files changed: 100 ins; 33 del; 34 mod 8274311: Make build.tools.jigsaw.GenGraphs more configurable Reviewed-by: alanb, iris - PR: https://git.openjdk.java.net/jdk/pull/5690
Re: RFR: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields
On Mon, 27 Sep 2021 01:00:18 GMT, Joe Darcy wrote:
> This is an initial PR for expanded lint warnings done under two bugs:
>
> 8202056: Expand serial warning to check for bad overloads of serial-related
> methods and ineffectual fields
> 8160675: Issue lint warning for non-serializable non-transient instance
> fields in serializable type
>
> to get feedback on the general approach and test strategy before further
> polishing the implementation.
>
> The implementation initially started as an annotation processor I wrote
> several years ago. The refined version being incorporated into Attr has been
> refactored, had its checks expanded, and been partially ported to idiomatic
> javac coding style rather than using the javax.lang.model API from annotation
> processing.
>
> Subsequent versions of this PR are expected to move the implementation closer
> to idiomatic javac, in particular to use javac flags rather than
> javax.lang.model.Modifier's. Additional resources keys will be defined for
> the serialization-related fields and methods not having the expected
> modifiers, types, etc. The resource keys for the existing checks related to
> serialVersionUID and reused.
>
> Please also review the corresponding CSRs:
>
> https://bugs.openjdk.java.net/browse/JDK-8274335
> https://bugs.openjdk.java.net/browse/JDK-8274336
>
> Informative serialization-related warning messages must take into account
> whether a class, interface, annotation, record, and enum is being analyzed.
> Enum classes and record classes have special handling in serialization. This
> implementation under review has been augmented with checks for interface
> types recommended by Chris Hegarty in an attachment on 8202056.
>
> The JDK build has the Xlint:serial check enabled. The build did not pass with
> the augmented checks. For most modules, this PR contains the library changes
> necessary for the build to pass. I will start separate PRs in those library
> areas to get the needed SuppressWarning("serial") or other changes in place.
> For one module, I temporarily disabled the Xlint:serial check.
>
> In terms of performance, I have not done benchmarks of the JDK build with and
> without these changes, but informally the build seems to take about as long
> as before.
Build change looks ok.
-
Marked as reviewed by erikj (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5709
Re: RFR: 8274345: make build-test-lib is broken
On Mon, 27 Sep 2021 03:26:54 GMT, xpbob wrote: > 8274345: make build-test-lib is broken Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5710
RFR: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields
This is an initial PR for expanded lint warnings done under two bugs:
8202056: Expand serial warning to check for bad overloads of serial-related
methods and ineffectual fields
8160675: Issue lint warning for non-serializable non-transient instance fields
in serializable type
to get feedback on the general approach and test strategy before further
polishing the implementation.
The implementation initially started as an annotation processor I wrote several
years ago. The refined version being incorporated into Attr has been
refactored, had its checks expanded, and been partially ported to idiomatic
javac coding style rather than using the javax.lang.model API from annotation
processing.
Subsequent versions of this PR are expected to move the implementation closer
to idiomatic javac, in particular to use javac flags rather than
javax.lang.model.Modifier's. Additional resources keys will be defined for the
serialization-related fields and methods not having the expected modifiers,
types, etc. The resource keys for the existing checks related to
serialVersionUID and reused.
Please also review the corresponding CSRs:
https://bugs.openjdk.java.net/browse/JDK-8274335
https://bugs.openjdk.java.net/browse/JDK-8274336
Informative serialization-related warning messages must take into account
whether a class, interface, annotation, record, and enum is being analyzed.
Enum classes and record classes have special handling in serialization. This
implementation under review has been augmented with checks for interface types
recommended by Chris Hegarty in an attachment on 8202056.
The JDK build has the Xlint:serial check enabled. The build did not pass with
the augmented checks. For most modules, this PR contains the library changes
necessary for the build to pass. I will start separate PRs in those library
areas to get the needed SuppressWarning("serial") or other changes in place.
For one module, I temporarily disabled the Xlint:serial check.
In terms of performance, I have not done benchmarks of the JDK build with and
without these changes, but informally the build seems to take about as long as
before.
-
Commit messages:
- Appease jcheck
- Implement checks chegar recommended for interfaces.
- Update comment.
- Add tests for instance field checks.
- Clean build with instance field checks in place.
- Merge branch 'master' into JDK-8202056
- Put Externalizable checks last.
- Add checks for constructor access in Serializable classes.
- Add no-arg ctor check for Externalizable classes.
- Improve Externalization warnings.
- ... and 19 more: https://git.openjdk.java.net/jdk/compare/5756385c...d498ff5f
Changes: https://git.openjdk.java.net/jdk/pull/5709/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5709=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8202056
Stats: 1519 lines in 79 files changed: 1511 ins; 1 del; 7 mod
Patch: https://git.openjdk.java.net/jdk/pull/5709.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5709/head:pull/5709
PR: https://git.openjdk.java.net/jdk/pull/5709
RFR: 8274345: make build-test-lib is broken
8274345: make build-test-lib is broken - Commit messages: - 8274345: make build-test-lib is broken Changes: https://git.openjdk.java.net/jdk/pull/5710/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5710=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274345 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5710.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5710/head:pull/5710 PR: https://git.openjdk.java.net/jdk/pull/5710
Withdrawn: 8271148: static-libs-image target --with-native-debug-symbols=external doesn't produce debug info
On Thu, 22 Jul 2021 16:43:26 GMT, Severin Gehwolf wrote: > Hi! > > Please review this tiny patch which removes the special casing of > `--with-native-debug-symbols=external` for the static libs build. I don't see > why this is needed. If no debug symbols are wanted > `--with-native-debug-symbols=none` can be used to achieve the same effect. > Therefore, I propose to remove this hunk. > > Testing: Inspecting of the log files and seeing that `-g` switch is there. > Run the reproducer test on the resulting static libraries. > > Thoughts? This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4876
Re: RFR: 8271148: static-libs-image target --with-native-debug-symbols=external doesn't produce debug info
On Thu, 22 Jul 2021 16:43:26 GMT, Severin Gehwolf wrote: > Hi! > > Please review this tiny patch which removes the special casing of > `--with-native-debug-symbols=external` for the static libs build. I don't see > why this is needed. If no debug symbols are wanted > `--with-native-debug-symbols=none` can be used to achieve the same effect. > Therefore, I propose to remove this hunk. > > Testing: Inspecting of the log files and seeing that `-g` switch is there. > Run the reproducer test on the resulting static libraries. > > Thoughts? The use case we'd be needing for this is to have debug info in the static libraries, but not in the dynamic variants. The reason for this is that in order for somebody to get debug symbols for a binary that includes some OpenJDK static libs **and** want relevant debug info for the OpenJDK libs, stripping needs to happen after the final binary has been linked. As such, external debug symbols for static libs aren't as useful. Therefore, implementing stripping for static libs has fairly low priority for me. For the time being I'll withdraw this PR. - PR: https://git.openjdk.java.net/jdk/pull/4876
