Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test Ah, that's great. Seems all tests that have `@library`... `/vmTestbase/vm/mlvm/patches` are ignored (you missed `test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/stress/classfmt/manyIndyOneCPX`) Safe to revert all the test changes. Now waiting for build again. - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test Which tests do you see? The following tests are all `@ignored`: test/hotspot/jtreg/vmTestbase/vm/mlvm/meth/stress/gc/createLotsOfMHConsts/Test.java test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/stress/classfmt/mt/TestDescription.java test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/stress/classfmt/mh/TestDescription.java test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/stress/classfmt/incorrectBootstrap/TestDescription.java test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/stress/classfmt/correctBootstrap/TestDescription.java - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Tue, 22 Mar 2022 21:55:34 GMT, Mandy Chung wrote: > I created CSR (https://bugs.openjdk.java.net/browse/JDK-8283530). I took the > liberty to adjust the CSR to include only the needed information. Since I'm > the RE for this CSR (you don't have JBS account), you will need another > reviewer for this CSR. I've added myself as a reviewer on the CSR. - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test I see a few other constant pool stress tests compile the `java.base` patches, which includes this broken `CallSite` implementation. Should we remove that implementation, or does it not cause compilation failure as is? - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test I created CSR (https://bugs.openjdk.java.net/browse/JDK-8283530). I took the liberty to adjust the CSR to include only the needed information. Since I'm the RE for this CSR (you don't have JBS account), you will need another reviewer for this CSR. - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test This `GenManyIndyCorrectBootstrap.java` test is currently ignored (JDK-8194951). Since this test is not executed and will need to be reworked anyway, I would suggest to leave this test as is and let JDK-8194951 to handle it properly. We should add a comment in JDK-8194951 to describe this change. P.S. Sorry for the late comment as I was on vacation last week. - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote:
>> Change `CallSite` to a sealed class, as `CallSite` is an abstract class
>> which does not allow direct subclassing by users per its documentation.
>> Since I don't have a JBS account, I posted the content for the CSR in a
>> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and
>> wish someone can submit a CSR for me.
>
> liach has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Remove homemade callsite from test
Hmm, I think the purpose of this test is just to fill up the class constant
pool with random garbage and to ensure it runs, and using either type of indy
doesn't seem to make big differences.
Moreover, these tests [need to be
reworked](https://github.com/openjdk/jdk/blob/d8893fad23d1ee6841336b96c34599643edb81ce/test/hotspot/jtreg/vmTestbase/README.md),
but I don't think I am suited for such a rework for this test.
--
If we do tweak the test, a subclass of `ConstantCallSite` would work too:
public class CustomCallSite extends ConstantCallSite {
public CustomCallSite(MethodHandles.Lookup lookup, String callName,
MethodType mtype, MethodHandle handle) {
super(handle);
}
}
And we write bytecode that makes an indy using this ctor as bootstrap method
and additionally pass a `Handle` with `fullClassName`, `TARGET_METHOD_NAME`,
`TARGET_METHOD_SIGNATURE`.
https://github.com/openjdk/jdk/blob/d8893fad23d1ee6841336b96c34599643edb81ce/test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/share/GenManyIndyCorrectBootstrap.java#L107-L109
Becomes
mw.visitInvokeDynamicInsn(TARGET_METHOD_NAME,
TARGET_METHOD_SIGNATURE,
bsm,
customCallSite ? new Object[] {TARGET_HANDLE} : new Object[0]);
The problem with this approach is:
1. Where should I put this `CustomCallSite` class? The bootstrap is only a
class generator as opposed to a runtime, and this class probably shouldn't be
put into the patches for `java.lang`. (In fact, the patches on asm should be
replaced by patches on a shaded standalone asm library to reduce compile time
for tests, too)
- Should I generate it in the `generateBytecodes` method instead?
2. The approach of using a indy + a MethodHandle constant would use an extra
constant, wonder if it hurts the goal of filling up the constant pool without
exceeding the limit.
-
PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test The purpose of `GenManyIndyCorrectBootstrap`’s `NewInvokeSpecialCallSite` is to check that bootstrap methods work correctly with a `REF_newInvokeSpecial` method handle. Instead, it should probably be implemented by subclassing `MutableCallSite`. - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v3]
> Change `CallSite` to a sealed class, as `CallSite` is an abstract class which > does not allow direct subclassing by users per its documentation. Since I > don't have a JBS account, I posted the content for the CSR in a GitHub Gist > at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and wish someone > can submit a CSR for me. liach has updated the pull request incrementally with one additional commit since the last revision: Remove homemade callsite from test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7840/files - new: https://git.openjdk.java.net/jdk/pull/7840/files/eef61c31..1bcd779f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7840&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7840&range=01-02 Stats: 119 lines in 3 files changed: 2 ins; 113 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7840.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7840/head:pull/7840 PR: https://git.openjdk.java.net/jdk/pull/7840
