Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v16]
On Fri, 21 Apr 2023 02:33:37 GMT, SUN Guoyun wrote: >> Matias Saavedra Silva has updated the pull request incrementally with one >> additional commit since the last revision: >> >> s390x NULL to nullptr > > src/hotspot/cpu/riscv/templateTable_riscv.cpp line 2233: > >> 2231: >> 2232: __ load_resolved_indy_entry(cache, index); >> 2233: __ membar(MacroAssembler::AnyAny); > > Why is the AnyAny barrier used here？ Hi @sunny868, I'm working on removing these unnecessary barriers. RISC-V port uses more conservative barriers like this for some reasons (e.g.: [1][2][3]), we can just remove them. 1. https://github.com/openjdk/jdk/blob/36ec05d52a79185d8c6669713fd17933128c032a/src/hotspot/cpu/riscv/templateTable_riscv.cpp#L3438-L3443 2. https://github.com/openjdk/jdk/blob/36ec05d52a79185d8c6669713fd17933128c032a/src/hotspot/cpu/riscv/templateTable_riscv.cpp#L3558-L3563 3. https://github.com/openjdk/jdk/blob/36ec05d52a79185d8c6669713fd17933128c032a/src/hotspot/cpu/riscv/templateTable_riscv.cpp#L3614-L3619 - PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1173362912
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 src/hotspot/cpu/riscv/templateTable_riscv.cpp line 2233: > 2231: > 2232: __ load_resolved_indy_entry(cache, index); > 2233: __ membar(MacroAssembler::AnyAny); Why is the AnyAny barrier used here？ - PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1173246912
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v16]
On Mon, 3 Apr 2023 05:57:18 GMT, Thomas Stuefe wrote: > > 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! For future reference, we maintain a list of people working on ports: https://wiki.openjdk.org/display/HotSpot/Ports and we do have a mailing list for porters as well: porters-...@openjdk.org . This makes it easier to find out who to contact. Cheers, Thomas - PR Comment: https://git.openjdk.org/jdk/pull/12778#issuecomment-1493717962
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v16]
On Fri, 31 Mar 2023 15:34:12 GMT, Matias Saavedra Silva wrote: > > 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! I lack the time to do this atm; let's see if one of the porters can help. @bulasevich @snazarkin ? - PR Comment: https://git.openjdk.org/jdk/pull/12778#issuecomment-1493703283
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]
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 It has also broken GraalVM Native Image. I'll open a JBS issue with a reproducer soon but here's hs-err from a slowdebug JDK build showing the problem: [hs_err_pid30379.log](https://github.com/openjdk/jdk/files/11122818/hs_err_pid30379.log) - PR Comment: https://git.openjdk.org/jdk/pull/12778#issuecomment-1492011186
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 https://github.com/openjdk/jdk/pull/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? - PR Comment: https://git.openjdk.org/jdk/pull/12778#issuecomment-1491971108
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&pr=12778&range=15 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12778&range=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
