Integrated: 8329418: Replace pointers to tables with offsets in relocation bitmap
On Mon, 6 May 2024 17:05:47 GMT, Matias Saavedra Silva wrote: > The beginning of the RW region contains pointers to c++ vtables which are > always located at a fixed offset from the shared base address at runtime. > This offset can be calculated at dumptime and stored with the read-only > tables at the top of the RO region. As a further improvement, all the > pointers to RO tables are replaced with offsets as well. > > These changes will reduce the number of pointers in the RW and RO regions and > will allow for the relocation bitmap size optimizations to be more effective. > Verified with tier 1-5 tests. This pull request has now been integrated. Changeset: a706ca4f Author:Matias Saavedra Silva URL: https://git.openjdk.org/jdk/commit/a706ca4fdb4db4ba36c6ad04a37c37a348f8af0b Stats: 137 lines in 11 files changed: 64 ins; 24 del; 49 mod 8329418: Replace pointers to tables with offsets in relocation bitmap Reviewed-by: cjplummer, iklam - PR: https://git.openjdk.org/jdk/pull/19107
Re: RFR: 8329418: Replace pointers to tables with offsets in relocation bitmap [v2]
On Tue, 7 May 2024 21:39:04 GMT, Chris Plummer wrote: >> Matias Saavedra Silva has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Chris comments and cleanup > > SA changes look good. Thanks for taking care of this. Thanks for the reviews @plummercj and @iklam! - PR Comment: https://git.openjdk.org/jdk/pull/19107#issuecomment-2103692236
Re: RFR: 8329418: Replace pointers to tables with offsets in relocation bitmap [v4]
> The beginning of the RW region contains pointers to c++ vtables which are > always located at a fixed offset from the shared base address at runtime. > This offset can be calculated at dumptime and stored with the read-only > tables at the top of the RO region. As a further improvement, all the > pointers to RO tables are replaced with offsets as well. > > These changes will reduce the number of pointers in the RW and RO regions and > will allow for the relocation bitmap size optimizations to be more effective. > Verified with tier 1-5 tests. Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: u_char* to void** - Changes: - all: https://git.openjdk.org/jdk/pull/19107/files - new: https://git.openjdk.org/jdk/pull/19107/files/11f39483..7fca6588 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19107=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19107=02-03 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19107.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19107/head:pull/19107 PR: https://git.openjdk.org/jdk/pull/19107
Re: RFR: 8329418: Replace pointers to tables with offsets in relocation bitmap [v3]
> The beginning of the RW region contains pointers to c++ vtables which are > always located at a fixed offset from the shared base address at runtime. > This offset can be calculated at dumptime and stored with the read-only > tables at the top of the RO region. As a further improvement, all the > pointers to RO tables are replaced with offsets as well. > > These changes will reduce the number of pointers in the RW and RO regions and > will allow for the relocation bitmap size optimizations to be more effective. > Verified with tier 1-5 tests. Matias Saavedra Silva 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 12 additional commits since the last revision: - Merge branch 'master' into pointer_to_offset_8329418 - Ioi comments - Chris comments and cleanup - Merge branch 'master' into pointer_to_offset_8329418 - Cleanup - Corrected SA - Editing SA - Fixed dynamic dumping - Now works with -Xshare:on - Adjusted serialization - ... and 2 more: https://git.openjdk.org/jdk/compare/aaa2e59e...11f39483 - Changes: - all: https://git.openjdk.org/jdk/pull/19107/files - new: https://git.openjdk.org/jdk/pull/19107/files/d40afef9..11f39483 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19107=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19107=01-02 Stats: 17065 lines in 289 files changed: 8650 ins; 5891 del; 2524 mod Patch: https://git.openjdk.org/jdk/pull/19107.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19107/head:pull/19107 PR: https://git.openjdk.org/jdk/pull/19107
Re: RFR: 8329418: Replace pointers to tables with offsets in relocation bitmap [v2]
> The beginning of the RW region contains pointers to c++ vtables which are > always located at a fixed offset from the shared base address at runtime. > This offset can be calculated at dumptime and stored with the read-only > tables at the top of the RO region. As a further improvement, all the > pointers to RO tables are replaced with offsets as well. > > These changes will reduce the number of pointers in the RW and RO regions and > will allow for the relocation bitmap size optimizations to be more effective. > Verified with tier 1-5 tests. Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Chris comments and cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/19107/files - new: https://git.openjdk.org/jdk/pull/19107/files/c925025e..d40afef9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19107=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19107=00-01 Stats: 10 lines in 3 files changed: 0 ins; 6 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19107.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19107/head:pull/19107 PR: https://git.openjdk.org/jdk/pull/19107
Re: RFR: 8329418: Replace pointers to tables with offsets in relocation bitmap
On Mon, 6 May 2024 22:32:12 GMT, Chris Plummer wrote:
>> The beginning of the RW region contains pointers to c++ vtables which are
>> always located at a fixed offset from the shared base address at runtime.
>> This offset can be calculated at dumptime and stored with the read-only
>> tables at the top of the RO region. As a further improvement, all the
>> pointers to RO tables are replaced with offsets as well.
>>
>> These changes will reduce the number of pointers in the RW and RO regions
>> and will allow for the relocation bitmap size optimizations to be more
>> effective. Verified with tier 1-5 tests.
>
> test/hotspot/jtreg/serviceability/sa/TestSysProps.java line 68:
>
>> 66: }
>> 67: if (numProps != expectedCount) {
>> 68: throw new RuntimeException("Wrong number of " + cmdName + "
>> properties: " + numProps + " Expected: " + expectedCount);
>
> I think it would be good to add parenthesis around the extra output you added.
This was an accidental leftover from debugging, I didn't intend for this to be
part of the change. I should revert this since it's beyond the scope of this
change.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19107#discussion_r1592648930
RFR: 8329418: Replace pointers to tables with offsets in relocation bitmap
The beginning of the RW region contains pointers to c++ vtables which are always located at a fixed offset from the shared base address at runtime. This offset can be calculated at dumptime and stored with the read-only tables at the top of the RO region. As a further improvement, all the pointers to RO tables are replaced with offsets as well. These changes will reduce the number of pointers in the RW and RO regions and will allow for the relocation bitmap size optimizations to be more effective. Verified with tier 1-5 tests. - Commit messages: - Merge branch 'master' into pointer_to_offset_8329418 - Cleanup - Corrected SA - Editing SA - Fixed dynamic dumping - Now works with -Xshare:on - Adjusted serialization - Serializing offsets - 8329418: Replace pointers to tables with offsets in relocation bitmap Changes: https://git.openjdk.org/jdk/pull/19107/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19107=00 Issue: https://bugs.openjdk.org/browse/JDK-8329418 Stats: 121 lines in 8 files changed: 69 ins; 23 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/19107.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19107/head:pull/19107 PR: https://git.openjdk.org/jdk/pull/19107
Integrated: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 15:26:52 GMT, Matias Saavedra Silva wrote: > Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), > [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and > [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic > operands needed to be rewritten to encoded values to better distinguish indy > entries from other cp cache entries. The above changes now distinguish > between entries with `to_cp_index()` using the bytecode, which is now > propagated by the callers. > > The encoding flips the bits of the index so the encoded index is always > negative, leading to access errors if there is no matching decode call. These > calls are removed with some methods adjusted to distinguish between indices > with the bytecode. Verified with tier 1-5 tests. The changes show no issues > when tested against libgraal. This pull request has now been integrated. Changeset: 383fe6ea Author:Matias Saavedra Silva URL: https://git.openjdk.org/jdk/commit/383fe6eaab423a1218c9915362f691472e3773e7 Stats: 225 lines in 37 files changed: 15 ins; 137 del; 73 mod 8330388: Remove invokedynamic cache index encoding Reviewed-by: cjplummer, dlong, coleenp - PR: https://git.openjdk.org/jdk/pull/18819
Re: RFR: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 22:48:16 GMT, Dean Long wrote: >> Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), >> [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and >> [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic >> operands needed to be rewritten to encoded values to better distinguish indy >> entries from other cp cache entries. The above changes now distinguish >> between entries with `to_cp_index()` using the bytecode, which is now >> propagated by the callers. >> >> The encoding flips the bits of the index so the encoded index is always >> negative, leading to access errors if there is no matching decode call. >> These calls are removed with some methods adjusted to distinguish between >> indices with the bytecode. Verified with tier 1-5 tests. The changes show no >> issues when tested against libgraal. > > Did you consider minimizing changes by leaving > decode_invokedynamic_index/encode_invokedynamic_index calls in place, but > having the implementations not change the value? Thanks for the reviews @dean-long @gilles-duboscq @coleenp and @plummercj! - PR Comment: https://git.openjdk.org/jdk/pull/18819#issuecomment-2072603376
Re: RFR: 8330388: Remove invokedynamic cache index encoding [v2]
On Wed, 17 Apr 2024 22:41:08 GMT, Dean Long wrote: >> Matias Saavedra Silva has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Dean and Gilles comments > > src/hotspot/share/ci/ciEnv.cpp line 1513: > >> 1511: // process the BSM >> 1512: int pool_index = indy_info->constant_pool_index(); >> 1513: BootstrapInfo bootstrap_specifier(cp, pool_index, indy_index); > > Why not just change the incoming parameter name to `index`? `indy_index` is used frequently even in functions that are only used in the context of invokedynamic. I think it helps with clarity. - PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1571043673
Re: RFR: 8330388: Remove invokedynamic cache index encoding [v2]
> Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), > [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and > [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic > operands needed to be rewritten to encoded values to better distinguish indy > entries from other cp cache entries. The above changes now distinguish > between entries with `to_cp_index()` using the bytecode, which is now > propagated by the callers. > > The encoding flips the bits of the index so the encoded index is always > negative, leading to access errors if there is no matching decode call. These > calls are removed with some methods adjusted to distinguish between indices > with the bytecode. Verified with tier 1-5 tests. Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Dean and Gilles comments - Changes: - all: https://git.openjdk.org/jdk/pull/18819/files - new: https://git.openjdk.org/jdk/pull/18819/files/87926aee..3ef92512 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18819=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18819=00-01 Stats: 6 lines in 2 files changed: 0 ins; 1 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/18819.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18819/head:pull/18819 PR: https://git.openjdk.org/jdk/pull/18819
Re: RFR: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 22:48:16 GMT, Dean Long wrote: > Did you consider minimizing changes by leaving > decode_invokedynamic_index/encode_invokedynamic_index calls in place, but > having the implementations not change the value? The intention from the start was to remove the encode/decode methods because they have been made unnecessary thanks to the changes mentioned in the description. As the author of the previously mentioned changes that overhauled the cpcache, this change should have been included in one of those PRs, but I must have forgotten! Leaving the calls in even if they did nothing would just make the code confusing and might become a red herring if other issues in the area come up. - PR Comment: https://git.openjdk.org/jdk/pull/18819#issuecomment-2064263944
Re: RFR: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 22:43:38 GMT, Dean Long wrote: >> Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), >> [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and >> [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic >> operands needed to be rewritten to encoded values to better distinguish indy >> entries from other cp cache entries. The above changes now distinguish >> between entries with `to_cp_index()` using the bytecode, which is now >> propagated by the callers. >> >> The encoding flips the bits of the index so the encoded index is always >> negative, leading to access errors if there is no matching decode call. >> These calls are removed with some methods adjusted to distinguish between >> indices with the bytecode. Verified with tier 1-5 tests. > > src/hotspot/share/classfile/resolutionErrors.hpp line 60: > >> 58: >> 59: // This function is used to encode an invokedynamic index to >> differentiate it from a >> 60: // constant pool index. It assumes it is being called with a index >> that is less than 0 > > Is this comment still correct? The last sentence is no longer valid, thanks for catching this! - PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1570969645
RFR: 8330388: Remove invokedynamic cache index encoding
Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic operands needed to be rewritten to encoded values to better distinguish indy entries from other cp cache entries. The above changes now distinguish between entries with `to_cp_index()` using the bytecode, which is now propagated by the callers. The encoding flips the bits of the index so the encoded index is always negative, leading to access errors if there is no matching decode call. These calls are removed with some methods adjusted to distinguish between indices with the bytecode. Verified with tier 1-5 tests. - Commit messages: - 8330388: Remove invokedynamic cache index encoding Changes: https://git.openjdk.org/jdk/pull/18819/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18819=00 Issue: https://bugs.openjdk.org/browse/JDK-8330388 Stats: 220 lines in 37 files changed: 15 ins; 136 del; 69 mod Patch: https://git.openjdk.org/jdk/pull/18819.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18819/head:pull/18819 PR: https://git.openjdk.org/jdk/pull/18819
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
On Sat, 2 Dec 2023 00:38:58 GMT, Ioi Lam wrote: >> This is a simple clean up that moves the code for initializing the CDS >> config states from arguments.cpp to cdsConfig.cpp >> >> I renamed a few functions, but otherwise the code is unchanged. >> >> - `get_default_shared_archive_path()` -> `default_archive_path()` >> - `GetSharedArchivePath()` -> `static_archive_path()` >> - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` >> >> There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is >> compiled only if CDS is enabled. > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > fixed indentation Thanks for addressing my comments. Approved! - Marked as reviewed by matsaave (Committer). PR Review: https://git.openjdk.org/jdk/pull/16868#pullrequestreview-1766049580
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp
On Tue, 28 Nov 2023 23:24:53 GMT, Ioi Lam wrote:
> This is a simple clean up that moves the code for initializing the CDS config
> states from arguments.cpp to cdsConfig.cpp
>
> I renamed a few functions, but otherwise the code is unchanged.
>
> - `get_default_shared_archive_path()` -> `default_archive_path()`
> - `GetSharedArchivePath()` -> `static_archive_path()`
> - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()`
>
> There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is
> compiled only if CDS is enabled.
This looks good! I think this is a good opportunity to refactor some of the
code for better readability so I left some comments below.
src/hotspot/share/cds/cdsConfig.cpp line 101:
> 99: void CDSConfig::extract_shared_archive_paths(const char* archive_path,
> 100: char** base_archive_path,
> 101: char** top_archive_path) {
Could you align these arguments?
src/hotspot/share/cds/cdsConfig.cpp line 125:
> 123: }
> 124:
> 125: void CDSConfig::init_shared_archive_paths() {
Now that I see this there is a lot of indentation thanks to the nested
conditionals. I don't have much to offer but is there a cleaner way to format
this method? Maybe you can extract the code in `if (archives == 1)` into its
own method for better readability.
src/hotspot/share/runtime/arguments.cpp line 1262:
> 1260: }
> 1261:
> 1262: CDSConfig::check_system_property(key, value);
I see this is only called once, do you expect this method to be used again? It
may be unnecessary to extract this code into its own method.
-
PR Review: https://git.openjdk.org/jdk/pull/16868#pullrequestreview-1760626242
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412613074
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412616593
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412610795
Re: RFR: 8311879: ClassWriter generates invalid invokedynamic code [v2]
On Thu, 13 Jul 2023 03:25:38 GMT, Daohan Qu wrote: >> This patch should fix the wrong CP index for `invokedynamic` instruction >> generated by SA's `ClassWriter`. The buggy code in >> `sun.jvm.hotspot.tools.jcore.ByteCodeRewriter` should have been up-to-date >> with the following code snippet in `hotspot`: >> >> https://github.com/openjdk/jdk/blob/753bd563ecca6bb5ff9b5ebc0957bc1854dce78d/src/hotspot/share/interpreter/rewriter.cpp#L291-L294 >> >> The comments above seem to be obsolete since the following change made in >> [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995). So I also remove >> them. >> >> >> +// Should do nothing since we are not patching this bytecode >> int cache_index = ConstantPool::decode_invokedynamic_index( >> Bytes::get_native_u4(p)); >> // We will reverse the bytecode rewriting _after_ adjusting them. >> // Adjust the cache index by offset to the invokedynamic entries in the >> // cpCache plus the delta if the invokedynamic bytecodes were adjusted. >> -int adjustment = cp_cache_delta() + _first_iteration_cp_cache_limit; >> -int cp_index = invokedynamic_cp_cache_entry_pool_index(cache_index - >> adjustment); >> +int cp_index = >> _initialized_indy_entries.at(cache_index).constant_pool_index(); >> assert(_pool->tag_at(cp_index).is_invoke_dynamic(), "wrong index"); >> >> >> This fix is straightforward and thank @asotona for finding this bug! >> >> ### Test Results of release build on Linux x64 >> * `jtreg:test/hotspot/jtreg/serviceability` and `jtreg:test/jdk/sun/tools/`: >> PASS >> * `tier1`: PASS >> * `tier2` and `tier3`: PASS (Failures of >> `sun/security/lib/cacerts/VerifyCACerts.java` and >> `sun/security/pkcs11/KeyStore/CertChainRemoval.java` seem to be unrelated to >> this patch) > > Daohan Qu has updated the pull request incrementally with one additional > commit since the last revision: > > Use Assert instead of throwing exceptions Looks good, thanks for the fix! - Marked as reviewed by matsaave (Committer). PR Review: https://git.openjdk.org/jdk/pull/14852#pullrequestreview-1529040867
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Thu, 29 Jun 2023 17:29:50 GMT, Coleen Phillimore wrote:
>> Please review change for mostly fixing return types in the constant pool and
>> metadata to fix -Wconversion warnings in JVMTI code. The order of
>> preference for changes are: 1. change the types to more distinct types
>> (fields in the constant pool are u2 because that's their size in the
>> classfile), 2. add direct int casts if the value has been checked in asserts
>> above, and 3. checked_cast<> if not verified, and 4. added some
>> pointer_delta_as_ints where needed.
>> Tested with tier1-4.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a comment about u1 cast to new_index for the ldc bytecode.
Looks good, thanks for this change! I listed a few considerations below, but if
you don't think they are necessary, what you have is just fine.
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 2195:
> 2193: case Bytecodes::_ldc:
> 2194: {
> 2195: u1 cp_index = *(bcp + 1);
Constant pool indices are usually u2, why does this need to be a u1?
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 2268:
> 2266: {
> 2267: address p = bcp + 1;
> 2268: int cp_index = Bytes::get_Java_u2(p);
Should this field also be a u2?
src/hotspot/share/prims/methodComparator.cpp line 79:
> 77: case Bytecodes::_instanceof : {
> 78: int cpi_old = s_old->get_index_u2();
> 79: int cpi_new = s_new->get_index_u2();
These constant pool accessors like `klass_at_noresolve` currently take in `int
which` but I think it's worth looking at if this is necessary. Constant pool
indices and constant pool cache indices seem to both be u2 so it might be a
better option to change the arguments to u2 here to avoid the need to cast.
-
Marked as reviewed by matsaave (Committer).
PR Review: https://git.openjdk.org/jdk/pull/14710#pullrequestreview-1505776921
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1246928389
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1246927933
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1246934498
Re: RFR: 8309878: Reduce inclusion of resolvedIndyEntry.hpp
On Mon, 12 Jun 2023 19:52:36 GMT, Ioi Lam wrote: > resolvedIndyEntry.hpp was added in > [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995) and is included in > the popular cpCache.hpp. As a result, resolvedIndyEntry.hpp is included in > 807 out of about 1160 hotspot .o files. > > The contents of resolvedIndyEntry.hpp is infrequently used. Its inclusion > should be moved from cpCache.hpp to cpCache.inline.hpp. This improves hotspot > build time. > > After this PR, resolvedIndyEntry.hpp is included in only 30 hotspot .o files. Looks good, thanks for this! - Marked as reviewed by matsaave (Committer). PR Review: https://git.openjdk.org/jdk/pull/14427#pullrequestreview-1477437064
Re: RFR: 8309673: Refactor ref_at methods in SA ConstantPool [v2]
On Fri, 9 Jun 2023 14:56:21 GMT, Coleen Phillimore wrote: >> Matias Saavedra Silva has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Removed unnecessary assignment > > Looks good. Thank you for the reviews @coleenp, @fparain, and @iklam ! - PR Comment: https://git.openjdk.org/jdk/pull/14385#issuecomment-1585021327
Integrated: 8309673: Refactor ref_at methods in SA ConstantPool
On Thu, 8 Jun 2023 21:42:24 GMT, Matias Saavedra Silva wrote: > The accessor methods in constantpool.cpp were previously cleaned up to allow > for different types of indices to be used, distinguishing them by the > bytecode. This patch adds the same changes to the hotspot serviceability > agent code. Verified with tier 1-5 tests. This pull request has now been integrated. Changeset: 7d6f97d0 Author:Matias Saavedra Silva URL: https://git.openjdk.org/jdk/commit/7d6f97d04d8fac44b9c71ec7e36c27ec61e82445 Stats: 95 lines in 4 files changed: 32 ins; 25 del; 38 mod 8309673: Refactor ref_at methods in SA ConstantPool Reviewed-by: coleenp, fparain, iklam - PR: https://git.openjdk.org/jdk/pull/14385
Re: RFR: 8309673: Refactor ref_at methods in Serviceability ConstantPool [v2]
> The accessor methods in constantpool.cpp were previously cleaned up to allow > for different types of indices to be used, distinguishing them by the > bytecode. This patch adds the same changes to the hotspot serviceability > agent code. Verified with tier 1-5 tests. Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Removed unnecessary assignment - Changes: - all: https://git.openjdk.org/jdk/pull/14385/files - new: https://git.openjdk.org/jdk/pull/14385/files/2ee60bd1..6630cb2c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14385=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14385=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14385.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14385/head:pull/14385 PR: https://git.openjdk.org/jdk/pull/14385
RFR: 8309673: Refactor ref_at methods in Serviceability ConstantPool
The accessor methods in constantpool.cpp were previously cleaned up to allow for different types of indices to be used, distinguishing them by the bytecode. This patch adds the same changes to the hotspot serviceability code. Verified with tier 1-5 tests. - Commit messages: - 8309673: Refactor ref_at methods in Serviceability ConstantPool Changes: https://git.openjdk.org/jdk/pull/14385/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14385=00 Issue: https://bugs.openjdk.org/browse/JDK-8309673 Stats: 95 lines in 4 files changed: 32 ins; 25 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/14385.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14385/head:pull/14385 PR: https://git.openjdk.org/jdk/pull/14385
Re: RFR: 8308655: Narrow types of ConstantPool and ConstMethod returns [v2]
On Wed, 24 May 2023 18:19:33 GMT, Coleen Phillimore wrote: >> This change uses a number of ways to eliminate -Wconversion warnings in the >> metadata files in the oops directory. >> >> 1. narrow return types to u2 if the accessor is for a field or value that's >> u2 (u2 is most common for constMethod fields and constant pool indices) >> 2. Use checked_cast for places where we know the int value is u2 or s2 >> but propagating these types is too much fan out. >> 3. Use plain casts where it's obvious that the int value fits in the >> casted-to type. >> 4. Moved KlassKind to be contained in Klass to add the Unknown enum value to >> use instead of -1. >> 5. Moved the compute_from_signature function into ConstMethod as it sets >> values in ConstMethod and the parameters are changed in the set functions. >> Removed some pass through functions in Method. >> >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fred's comments. Looks good, I just had a few minor comments that might be unimportant. Otherwise, approved! src/hotspot/share/classfile/classLoaderExt.cpp line 70: > 68: Arguments::assert_is_dumping_archive(); > 69: int start_index = ClassLoader::num_boot_classpath_entries(); > 70: _app_class_paths_start_index = checked_cast(start_index); This might be a misunderstanding but are these meant to be s2 instead of jshort? src/hotspot/share/classfile/classLoaderExt.cpp line 122: > 120: int start_index = ClassLoader::num_boot_classpath_entries() + > 121: ClassLoader::num_app_classpath_entries(); > 122: _app_module_paths_start_index = checked_cast(start_index); Same question as above src/hotspot/share/oops/klass.hpp line 79: > 77: TypeArrayKlassKind, > 78: ObjArrayKlassKind, > 79: Unknown Maybe this should be `UnknownKind` to be consistent with the other options. - Marked as reviewed by matsaave (Committer). PR Review: https://git.openjdk.org/jdk/pull/14092#pullrequestreview-1442597537 PR Review Comment: https://git.openjdk.org/jdk/pull/14092#discussion_r1204662039 PR Review Comment: https://git.openjdk.org/jdk/pull/14092#discussion_r1204662242 PR Review Comment: https://git.openjdk.org/jdk/pull/14092#discussion_r1204658756
Re: RFR: 8306851: Move Method access flags [v4]
On Fri, 28 Apr 2023 15:42:58 GMT, Coleen Phillimore wrote: >> This change moves the flags from AccessFlags to either ConstMethodFlags or >> MethodFlags, depending on whether they are set at class file parse time, >> which makes them essentially const, or at runtime, which makes them needing >> atomic access. >> >> This leaves AccessFlags int size because Klass still has JVM flags that are >> more work to move, but this change doesn't increase Method size. I didn't >> remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are >> several of these in other places, and with this change the code is benign. >> >> Tested with tier1-6, and some manual verification of printing. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Updates Looks good, thanks! - Marked as reviewed by matsaave (Committer). PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1406485565
Re: RFR: 8306851: Move Method access flags [v3]
On Thu, 27 Apr 2023 14:21:23 GMT, Coleen Phillimore wrote:
>> This change moves the flags from AccessFlags to either ConstMethodFlags or
>> MethodFlags, depending on whether they are set at class file parse time,
>> which makes them essentially const, or at runtime, which makes them needing
>> atomic access.
>>
>> This leaves AccessFlags int size because Klass still has JVM flags that are
>> more work to move, but this change doesn't increase Method size. I didn't
>> remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are
>> several of these in other places, and with this change the code is benign.
>>
>> Tested with tier1-6, and some manual verification of printing.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove bool argument from ConstMethodFlags.set function.
Nice change! Just some small nits but it otherwise looks good.
src/hotspot/share/oops/method.hpp line 606:
> 604:
> 605: bool compute_has_loops_flag();
> 606: bool set_has_loops() { set_has_loops_flag();
> set_has_loops_flag_init(); return true; }
Since this has multiple statements it should probably be on different lines
src/hotspot/share/oops/method.hpp line 615:
> 613: // has not been computed yet.
> 614: bool guaranteed_monitor_matching() const { return
> monitor_matching(); }
> 615: void set_guaranteed_monitor_matching() {
> set_monitor_matching(); }
Is this method just obsolete now? If so it might be worth replacing the callers
with `set_monitor_matching()` unless `set_monitor_matching()` is still meant to
be private.
-
Changes requested by matsaave (Committer).
PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1406036462
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180462644
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180472698
Re: RFR: 8306482: Remove unused Method AccessFlags
On Thu, 20 Apr 2023 00:35:06 GMT, Coleen Phillimore wrote: > Please review this small change to remove Method AccessFlags that are unused. > These flags were moved to ConstMethod a long time ago. > Tested with tier1-4, SA tests locally Nice cleanup, LGTM! - Marked as reviewed by matsaave (Committer). PR Review: https://git.openjdk.org/jdk/pull/13549#pullrequestreview-1394595045
Re: RFR: 8298048: Combine CDS archive heap into a single block [v5]
On Wed, 12 Apr 2023 17:59:23 GMT, Ioi Lam wrote: >> This PR combines the "open" and "closed" regions of the CDS archive heap >> into a single region. This significantly simplifies the implementation, >> making it more compatible with non-G1 collectors. There's a net removal of >> ~1000 lines in src code and another ~1200 lines of tests. >> >> **Notes for reviewers:** >> - Most of the code changes in CDS are removing the handling of "open" vs >> "closed" objects. >> - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). >> - It might be easier to see the diff with whitespaces off. >> - There are two major changes in the G1 code >> - The archived objects are now stored in the "old" region (see >> g1CollectedHeap.cpp in >> [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) >> - The majority of the other changes are removal of the "archive" region >> type (see heapRegionType.hpp). For ease of review, such code is isolated in >> [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) >> - Testing changes: >> - Now the archived java objects can move, along with the "old" regions >> that contain them. It's no longer possible to test whether a heap object >> came from CDS. As a result, the `WhiteBox.isShared(Object o)` API has been >> removed. >> - Many tests that uses this API are removed. Most of them were written >> during early development of CDS archived objects and are no longer relevant. >> >> **Testing:** >> - Mach5 tiers 1 ~ 7 > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > @ashu-mehra comments; some clean up The changes look good to me! - Marked as reviewed by matsaave (Committer). PR Review: https://git.openjdk.org/jdk/pull/13284#pullrequestreview-1383920908
Re: RFR: 8298048: Combine CDS archive heap into a single block
On Mon, 3 Apr 2023 03:32:27 GMT, Ioi Lam wrote:
> This PR combines the "open" and "closed" regions of the CDS archive heap into
> a single region. This significantly simplifies the implementation, making it
> more compatible with non-G1 collectors. There's a net removal of ~1000 lines
> in src code and another ~1200 lines of tests.
>
> **Notes for reviewers:**
> - Most of the code changes in CDS are removing the handling of "open" vs
> "closed" objects.
> - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer().
> - It might be easier to see the diff with whitespaces off.
> - There are two major changes in the G1 code
> - The archived objects are now stored in the "old" region (see
> g1CollectedHeap.cpp in
> [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770))
> - The majority of the other changes are removal of the "archive" region
> type (see heapRegionType.hpp). For ease of review, such code is isolated in
> [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6)
> - Testing changes:
> - Now the archived java objects can move, along with the "old" regions that
> contain them. It's no longer possible to test whether a heap object came from
> CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed.
> - Many tests that uses this API are removed. Most of them were written
> during early development of CDS archived objects and are no longer relevant.
>
> **Testing:**
> - Mach5 tiers 1 ~ 7
Changes requested by matsaave (Committer).
src/hotspot/share/cds/archiveBuilder.cpp line 1086:
> 1084: p2i(to_requested(start)), size_t(end - start));
> 1085: log_data(start, end, to_requested(start), /*is_heap=*/true);
> 1086: }
These log messages can be placed inside the else case before the break
src/hotspot/share/cds/archiveHeapWriter.cpp line 369:
> 367: template void
> ArchiveHeapWriter::store_requested_oop_in_buffer(T* buffered_addr,
> 368:
>oop request_oop) {
> 369: //assert(is_in_requested_regions(request_oop), "must be");
Some left over commented code. I assume this should be removed or a new assert
should be here to replace it.
src/hotspot/share/cds/archiveHeapWriter.cpp line 529:
> 527: num_non_null_ptrs ++;
> 528:
> 529: if (max_idx < idx) {
Is there a built in min() function we can use here? Maybe std::min()?
src/hotspot/share/cds/filemap.cpp line 1674:
> 1672:
> 1673: char* buffer = NEW_C_HEAP_ARRAY(char, size_in_bytes, mtClassShared);
> 1674: size_t written = write_bitmap(ptrmap, buffer, 0);
Maybe add a comment to clarify there is no offset? Constants in method
parameters can be confusing sometimes.
src/hotspot/share/cds/filemap.cpp line 2035:
> 2033: }
> 2034: if (end < e) {
> 2035: end = e;
Like mentioned before, maybe we have max() and min() methods to use here.
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 520:
> 518: } else {
> 519: return true;
> 520: }
Maybe make this `return reserved.contains(range.start()) &&
reserved.contains(range.last())`
-
PR Review: https://git.openjdk.org/jdk/pull/13284#pullrequestreview-1376508819
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160911406
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160911559
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160913791
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160916309
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160916888
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160924110
Re: RFR: 8298048: Combine CDS archive heap into a single block
On Mon, 3 Apr 2023 03:32:27 GMT, Ioi Lam wrote: > This PR combines the "open" and "closed" regions of the CDS archive heap into > a single region. This significantly simplifies the implementation, making it > more compatible with non-G1 collectors. There's a net removal of ~1000 lines > in src code and another ~1200 lines of tests. > > **Notes for reviewers:** > - Most of the code changes in CDS are removing the handling of "open" vs > "closed" objects. > - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer(). > - It might be easier to see the diff with whitespaces off. > - There are two major changes in the G1 code > - The archived objects are now stored in the "old" region (see > g1CollectedHeap.cpp in > [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770)) > - The majority of the other changes are removal of the "archive" region > type (see heapRegionType.hpp). For ease of review, such code is isolated in > [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6) > - Testing changes: > - Now the archived java objects can move, along with the "old" regions that > contain them. It's no longer possible to test whether a heap object came from > CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed. > - Many tests that uses this API are removed. Most of them were written > during early development of CDS archived objects and are no longer relevant. > > **Testing:** > - Mach5 tiers 1 ~ 7 Great cleanup! Making CDS easier to read and use is always a plus. Just some observations/nits: - PR Comment: https://git.openjdk.org/jdk/pull/13284#issuecomment-1500569237
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v16]
On Tue, 28 Mar 2023 19:50:36 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC port was provided by @reinrich, RISCV was provided by @DingliZhang >> and @zifeihan, and S390x by @offamitkumar. >> >> This change supports the following platforms: x86, aarch64, PPC, RISCV, and >> S390x > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > s390x NULL to nullptr > This obviously breaks arm, since its implementation is missing. I opened > https://bugs.openjdk.org/browse/JDK-8305387 to track this. This is > unfortunate since it holds work on arm in other areas, in my case for #10907. > > > This change supports the following platforms: x86, aarch64, PPC, RISCV, and > > S390x > > I wonder about the explicit exclusion of arm. Every other CPU seems to be > taken care of, even those Oracle does not maintain. Just curious, was there a > special reason for excluding arm? There is no special reason ARM32 was excluded other than the fact no porter has picked it up yet. Fortunately I was able to get in contact with porters for the other platforms, but nobody took on the ARM port until now. Thank you for opening the issue! - PR Comment: https://git.openjdk.org/jdk/pull/12778#issuecomment-1492144686
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v16]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC port was provided by @reinrich, RISCV was provided by @DingliZhang > and @zifeihan, and S390x by @offamitkumar. > > This change supports the following platforms: x86, aarch64, PPC, RISCV, and > S390x Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: s390x NULL to nullptr - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/dad70dc5..72ef0475 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=15 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=14-15 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Integrated: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry
On Mon, 27 Feb 2023 21:37:34 GMT, Matias Saavedra Silva wrote: > The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC port was provided by @reinrich, RISCV was provided by @DingliZhang > and @zifeihan, and S390x by @offamitkumar. > > This change supports the following platforms: x86, aarch64, PPC, RISCV, and > S390x This pull request has now been integrated. Changeset: 3fbbfd17 Author:Matias Saavedra Silva URL: https://git.openjdk.org/jdk/commit/3fbbfd17491906d707f73fe6b0db2989363c303a Stats: 1516 lines in 58 files changed: 1109 ins; 173 del; 234 mod 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry Co-authored-by: Richard Reingruber Co-authored-by: Dingli Zhang Co-authored-by: Gui Cao Co-authored-by: Amit Kumar Reviewed-by: coleenp, dnsimon, fparain, gcao, aph, fyang, amitkumar, lucy - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v15]
On Tue, 28 Mar 2023 15:00:14 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC port was provided by @reinrich, RISCV was provided by @DingliZhang >> and @zifeihan, and S390x by @offamitkumar. >> >> This change supports the following platforms: x86, aarch64, PPC, RISCV, and >> S390x > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > s390 update Thank you to all the reviewers for the detailed feedback, corrections, and improvements! @theRealAph @dougxc @fparain @coleenp @RealFYang @RealLucy @turbanoff @TheRealMDoerr @calvinccheung Also, thank you very much for completing the ports @reinrich, @DingliZhang, @zifeihan, and @offamitkumar! - PR Comment: https://git.openjdk.org/jdk/pull/12778#issuecomment-1487506191
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v15]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: s390 update - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/84ed272a..dad70dc5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=14 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=13-14 Stats: 105 lines in 4 files changed: 83 ins; 1 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v14]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: RISCV patch and aarch64 improvement - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/ff7f3503..84ed272a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=13 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=12-13 Stats: 21 lines in 3 files changed: 3 ins; 7 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v13]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Improved interpreter comments aarch64 - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/546291fc..ff7f3503 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=12 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=11-12 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v12]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Andrew comments aarch64 - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/a4714f54..546291fc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=11 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=10-11 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v11]
On Wed, 22 Mar 2023 17:42:35 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Improved comment for load-acquire aarch64 src/hotspot/cpu/ppc/templateTable_ppc_64.cpp line 2294: > 2292: > 2293: __ load_resolved_indy_entry(cache, index); > 2294: __ ld_ptr(method, in_bytes(ResolvedIndyEntry::method_offset()), > cache); @reinrich this load needs acquire semantics. src/hotspot/cpu/ppc/templateTable_ppc_64.cpp line 2308: > 2306: // Update registers with resolved info > 2307: __ load_resolved_indy_entry(cache, index); > 2308: __ ld_ptr(method, in_bytes(ResolvedIndyEntry::method_offset()), > cache); @reinrich same for this one. - PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1146343430 PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1146343487
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v11]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Improved comment for load-acquire aarch64 - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/cbe4fdcb..a4714f54 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=10 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=09-10 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v10]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Consistent register naming for aarch64 - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/8607f62a..cbe4fdcb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=09 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=08-09 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v9]
On Tue, 21 Mar 2023 10:51:08 GMT, Andrew Haley wrote:
>> Matias Saavedra Silva has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix riscv interpreter mistake and acquire semantics
>
> src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 2399:
>
>> 2397:bool is_invokevirtual,
>> 2398:bool is_invokevfinal,
>> /*unused*/
>> 2399:bool is_invokedynamic
>> /*unused*/) {
>
> Suggestion:
>
>bool /*is_invokedynamic*/) {
This is a temporary bandaid in the same format we see for `is_invokefinal`,
both of which should be cleaned up once all the ports are complete.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1143508140
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v9]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Fix riscv interpreter mistake and acquire semantics - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/6600e6dc..8607f62a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=08 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=07-08 Stats: 18 lines in 4 files changed: 4 ins; 7 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v8]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Fixed aarch64 and added load-acquire for resolution check - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/3dc112b2..6600e6dc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=07 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=06-07 Stats: 7 lines in 2 files changed: 4 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v7]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Acquire semantics - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/9a3a63ae..3dc112b2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=06 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=05-06 Stats: 10 lines in 4 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v6]
On Thu, 16 Mar 2023 16:11:57 GMT, Richard Reingruber wrote: >> Matias Saavedra Silva has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixed aarch64 interpreter mistake > > src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 2335: > >> 2333: >> 2334: __ load_resolved_indy_entry(cache, index); >> 2335: __ ldr(method, Address(cache, >> in_bytes(ResolvedIndyEntry::method_offset(; > > Should this load have acquire semantics? > Like [here in template > interpreter](https://github.com/openjdk/jdk/blob/2f23c80e0de44815d26a7d541701e16c9c1d32bc/src/hotspot/cpu/aarch64/interp_masm_aarch64.cpp#L239) > and [here for the zero > interpreter](https://github.com/openjdk/jdk/blob/2f23c80e0de44815d26a7d541701e16c9c1d32bc/src/hotspot/share/oops/cpCache.inline.hpp#L33)? > > Call stack for zero interpreter is > > ConstantPoolCacheEntry::indices_ord() > ConstantPoolCacheEntry::bytecode_1() > ConstantPoolCacheEntry::is_resolved(enum Bytecodes::Code) > BytecodeInterpreter::run(interpreterState) Yes, acquire semantics should be used here. Thank you for pointing this out! - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v6]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Fixed aarch64 interpreter mistake - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/415e7116..9a3a63ae Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=05 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=04-05 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v5]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Fixed indentation and other comments - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/db892223..415e7116 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=04 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=03-04 Stats: 71 lines in 9 files changed: 1 ins; 3 del; 67 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v3]
On Tue, 14 Mar 2023 15:39:39 GMT, Gui Cao wrote: >> Matias Saavedra Silva 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: >> >> - Typo in comment >> - Merge branch 'master' into resolvedIndyEntry_8301995 >> - Interpreter optimization and comments >> - PPC and RISCV port >> - 8301995: Move invokedynamic resolution information out of the cpCache > > src/hotspot/cpu/aarch64/interp_masm_aarch64.cpp line 1843: > >> 1841: ldr(cache, Address(rcpool, >> in_bytes(ConstantPoolCache::invokedynamic_entries_offset(; >> 1842: // Scale the index to be the entry index * >> sizeof(ResolvedInvokeDynamicInfo) >> 1843: mov(tmp, sizeof(ResolvedIndyEntry)); > > The tmp register is not used here, is it redundant? Right, the tmp register is not needed anymore thanks to the mul to shift optimization. Note that shifting will not be possible on 32-bit systems due to the size of ResolvedIndyEntry not being a power of two. This optimization only works on 64 bit builds. - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v4]
On Tue, 14 Mar 2023 23:29:17 GMT, Calvin Cheung wrote:
>> Matias Saavedra Silva has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> RISCV port update
>
> src/hotspot/share/interpreter/bootstrapInfo.cpp line 234:
>
>> 232: if (_indy_index > -1) {
>> 233: os::snprintf_checked(what, sizeof(what), "indy#%d", _indy_index);
>> 234: }
>
> Since the `else` case doesn’t have braces, maybe omit the braces for this
> case as well?
The if statements below use braces so I think it would be better to add braces
to the else case.
-
PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v4]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: RISCV port update - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/a3e7ca02..db892223 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=03 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=02-03 Stats: 23 lines in 2 files changed: 5 ins; 12 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v2]
On Tue, 14 Mar 2023 09:20:54 GMT, Richard Reingruber wrote: > @matias9927 can I ask you to merge master? There seem to be conflicts (at > least I see a message "This branch has conflicts that must be resolved"). I'd > like to give the change a spin in our CI testing. This requires that it can > be applied on master. I saw that merge error but nothing came up when I tried to merge locally. The branch is updated nonetheless, so you should be able to test it now @reinrich ! - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v3]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva 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: - Typo in comment - Merge branch 'master' into resolvedIndyEntry_8301995 - Interpreter optimization and comments - PPC and RISCV port - 8301995: Move invokedynamic resolution information out of the cpCache - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/c2d87e59..a3e7ca02 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=01-02 Stats: 92608 lines in 1481 files changed: 72908 ins; 8825 del; 10875 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v2]
On Mon, 13 Mar 2023 21:04:22 GMT, Coleen Phillimore wrote:
>> Matias Saavedra Silva has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Interpreter optimization and comments
>
> src/hotspot/cpu/x86/interp_masm_x86.cpp line 2075:
>
>> 2073: movptr(cache, Address(rbp, frame::interpreter_frame_cache_offset *
>> wordSize));
>> 2074: movptr(cache, Address(cache,
>> in_bytes(ConstantPoolCache::invokedynamic_entries_offset(;
>> 2075: if (is_power_of_2(sizeof(ResolvedIndyEntry))) {
>
> This was a good suggestion but I wonder if we should assert ResolvedIndyEntry
> is a power of 2 so we know if we change the size and make it go the slower
> path? Or is 32 bit not a power of two and we need this?
Currently the structure is a power of two on 64 bits but this is not the case
on 32 bit systems.
-
PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v2]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Interpreter optimization and comments - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/829517d6..c2d87e59 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12778=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12778=00-01 Stats: 47 lines in 10 files changed: 11 ins; 13 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry
On Tue, 7 Mar 2023 14:00:19 GMT, Coleen Phillimore wrote:
>> The current structure used to store the resolution information for
>> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its
>> ambigious fields f1 and f2. This structure can hold information for fields,
>> methods, and invokedynamics and each of its fields can hold different types
>> of values depending on the entry.
>>
>> This enhancement proposes a new structure to exclusively contain
>> invokedynamic information in a manner that is easy to interpret and easy to
>> extend. Resolved invokedynamic entries will be stored in an array in the
>> constant pool cache and the operand of the invokedynamic bytecode will be
>> rewritten to be the index into this array.
>>
>> Any areas that previously accessed invokedynamic data from
>> ConstantPoolCacheEntry will be replaced with accesses to this new array and
>> structure. Verified with tier1-9 tests.
>>
>> The PPC was provided by @reinrich and the RISCV port was provided by
>> @DingliZhang and @zifeihan.
>>
>> This change supports the following platforms: x86, aarch64, PPC, and RISCV
>
> src/hotspot/share/oops/cpCache.cpp line 727:
>
>> 725: set_reference_map(nullptr);
>> 726: #if INCLUDE_CDS
>> 727: if (_initial_entries != nullptr) {
>
> @iklam with moving invokedynamic entries out, do you still need to save
> initialized entries ? Does invokehandle need this? (Should have separate RFE
> if more cleanup is possible)
This along with the previous comment about `_invokedynamic_references_map`
would probably be better suited for their own RFE. I think the scope of this PR
should be limited to the indy structure and its implementation, so any changes
related to invokehandle can be traced more easily.
-
PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry
On Tue, 7 Mar 2023 13:39:50 GMT, Coleen Phillimore wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > src/hotspot/share/ci/ciReplay.cpp line 419: > >> 417: be used to avoid multiple blocks of similar code. When CPCE is >> obsoleted >> 418: these can be removed >> 419: */ > > I don't know if you really need this comment. If so, use // style instead. I think it's worth keeping around as a reminder for cleanup down the line since it's easy to overlook. I will change it to // style. - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry
On Tue, 7 Mar 2023 13:35:18 GMT, Coleen Phillimore wrote:
>> The current structure used to store the resolution information for
>> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its
>> ambigious fields f1 and f2. This structure can hold information for fields,
>> methods, and invokedynamics and each of its fields can hold different types
>> of values depending on the entry.
>>
>> This enhancement proposes a new structure to exclusively contain
>> invokedynamic information in a manner that is easy to interpret and easy to
>> extend. Resolved invokedynamic entries will be stored in an array in the
>> constant pool cache and the operand of the invokedynamic bytecode will be
>> rewritten to be the index into this array.
>>
>> Any areas that previously accessed invokedynamic data from
>> ConstantPoolCacheEntry will be replaced with accesses to this new array and
>> structure. Verified with tier1-9 tests.
>>
>> The PPC was provided by @reinrich and the RISCV port was provided by
>> @DingliZhang and @zifeihan.
>>
>> This change supports the following platforms: x86, aarch64, PPC, and RISCV
>
> src/hotspot/cpu/x86/templateTable_x86.cpp line 2801:
>
>> 2799:bool is_invokevirtual,
>> 2800:bool is_invokevfinal,
>> /*unused*/
>> 2801:bool is_invokedynamic
>> /*unused*/) {
>
> I assume you have to keep this parameter for the platform that doesn't still
> have this change (s390)?
That's correct, this method is declared inside the hpp used by all platforms,
so the parameters can't be changed until all the ports are complete.
-
PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry
On Tue, 7 Mar 2023 14:10:33 GMT, Coleen Phillimore wrote:
>> The current structure used to store the resolution information for
>> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its
>> ambigious fields f1 and f2. This structure can hold information for fields,
>> methods, and invokedynamics and each of its fields can hold different types
>> of values depending on the entry.
>>
>> This enhancement proposes a new structure to exclusively contain
>> invokedynamic information in a manner that is easy to interpret and easy to
>> extend. Resolved invokedynamic entries will be stored in an array in the
>> constant pool cache and the operand of the invokedynamic bytecode will be
>> rewritten to be the index into this array.
>>
>> Any areas that previously accessed invokedynamic data from
>> ConstantPoolCacheEntry will be replaced with accesses to this new array and
>> structure. Verified with tier1-9 tests.
>>
>> The PPC was provided by @reinrich and the RISCV port was provided by
>> @DingliZhang and @zifeihan.
>>
>> This change supports the following platforms: x86, aarch64, PPC, and RISCV
>
> src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantPool.java
> line 935:
>
>> 933: /*if (isInvokedynamicIndex(cpi)) {
>> 934: compilerToVM().resolveInvokeDynamicInPool(this,
>> cpi);
>> 935: }*/
>
> Is there something to fix here?
That's a vestige of old code that I don't believe is necessary anymore.
Invokedynamic is resolved further up so that can be removed. I think it makes
sense to leave the invokedynamic case for completeness, but it will be left
blank.
-
PR: https://git.openjdk.org/jdk/pull/12778
RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry
The current structure used to store the resolution information for invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its ambigious fields f1 and f2. This structure can hold information for fields, methods, and invokedynamics and each of its fields can hold different types of values depending on the entry. This enhancement proposes a new structure to exclusively contain invokedynamic information in a manner that is easy to interpret and easy to extend. Resolved invokedynamic entries will be stored in an array in the constant pool cache and the operand of the invokedynamic bytecode will be rewritten to be the index into this array. Any areas that previously accessed invokedynamic data from ConstantPoolCacheEntry will be replaced with accesses to this new array and structure. Verified with tier1-9 tests. The PPC was provided by @reinrich and the RISCV port was provided by @DingliZhang and @zifeihan. This change supports the following platforms: x86, aarch64, PPC, and RISCV - Commit messages: - PPC and RISCV port - 8301995: Move invokedynamic resolution information out of the cpCache Changes: https://git.openjdk.org/jdk/pull/12778/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12778=00 Issue: https://bugs.openjdk.org/browse/JDK-8301995 Stats: 1418 lines in 54 files changed: 1036 ins; 168 del; 214 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
