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 Thu, 18 Apr 2024 16:22:31 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. > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Dean and Gilles comments Still good! - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18819#pullrequestreview-2015844122
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
Re: RFR: 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. To borrow @shipilev's comment from a different PR, Good Riddance! - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18819#pullrequestreview-2008602360
Re: RFR: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 22:58:21 GMT, Dean Long wrote: > @dougxc should check JVMCI changes. Thanks for the heads up. I've asked @matias9927 to double check these changes against libgraal which should address any concerns about how this change impacts Graal. - PR Comment: https://git.openjdk.org/jdk/pull/18819#issuecomment-2063308834
Re: RFR: 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.
src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java
line 720:
> 718: @Override
> 719: public JavaMethod lookupMethod(int rawIndex, int opcode,
> ResolvedJavaMethod caller) {
> 720: int which = rawIndex;
We could get rid of that intermediate variable now and just use `rawIndex`
below.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1570192972
Re: RFR: 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. @dougxc should check JVMCI changes. - Marked as reviewed by dlong (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18819#pullrequestreview-2007498087
Re: RFR: 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. Did you consider minimizing changes by leaving decode_invokedynamic_index/encode_invokedynamic_index calls in place, but having the implementations not change the value? - PR Comment: https://git.openjdk.org/jdk/pull/18819#issuecomment-2062609288
Re: RFR: 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.
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`?
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?
src/hotspot/share/interpreter/bootstrapInfo.cpp line 77:
> 75: return true;
> 76: } else if (indy_entry->resolution_failed()) {
> 77: int encoded_index =
> ResolutionErrorTable::encode_indy_index(_indy_index);
That's an improvement, from two levels of encoding to only one!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1569625123
PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1569626519
PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1569627219
Re: RFR: 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. SA changes look good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18819#pullrequestreview-2007176260
