Re: RFR: 8283237: CallSite should be a sealed class [v4]
On Tue, 22 Mar 2022 23:18:21 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: > > Revert "Remove homemade callsite from test" > > This reverts commit 1bcd779f677420326bf365c3580ceab5a6b5e3fa. Marked as reviewed by mchung (Reviewer). - 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 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 [v4]
> 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: Revert "Remove homemade callsite from test" This reverts commit 1bcd779f677420326bf365c3580ceab5a6b5e3fa. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7840/files - new: https://git.openjdk.java.net/jdk/pull/7840/files/1bcd779f..c0a10f6e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7840=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7840=02-03 Stats: 119 lines in 3 files changed: 113 ins; 2 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
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=7840=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7840=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
Re: RFR: 8283237: CallSite should be a sealed class [v2]
On Fri, 18 Mar 2022 21:05:01 GMT, altrisi wrote:
> Note that there's a class in tests (or something similar) that extends
> CallSite directly:
> https://github.com/openjdk/jdk/blob/d8893fad23d1ee6841336b96c34599643edb81ce/test/hotspot/jtreg/vmTestbase/vm/mlvm/patches/java.base/java/lang/invoke/NewInvokeSpecialCallSite.java
Appears it's exclusively used by `GenManyIndyCorrectBootstrap`. Looking at that
code, it's just used to create random indy instructions to stuff the constant
pool:
if (Env.getRNG().nextBoolean()) {
bsm = new Handle(Opcodes.H_NEWINVOKESPECIAL,
NEW_INVOKE_SPECIAL_CLASS_NAME,
INIT_METHOD_NAME,
NEW_INVOKE_SPECIAL_BOOTSTRAP_METHOD_SIGNATURE);
} else {
bsm = new Handle(Opcodes.H_INVOKESTATIC,
this.fullClassName,
BOOTSTRAP_METHOD_NAME,
BOOTSTRAP_METHOD_SIGNATURE);
}
Appears we can just remove the randomization and always use a standard
reference to the generated class' bootstrap method, and extending `CallSite`
directly for such tiny functionality is overkill. If my approach is not
desired, please speak out and suggest changes. Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v2]
On Thu, 17 Mar 2022 08:07:23 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: > > Swap mutable and volatile in the permits list Note that there's a class in tests (or something similar) that extends CallSite directly: https://github.com/openjdk/jdk/blob/d8893fad23d1ee6841336b96c34599643edb81ce/test/hotspot/jtreg/vmTestbase/vm/mlvm/patches/java.base/java/lang/invoke/NewInvokeSpecialCallSite.java - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v2]
On Thu, 17 Mar 2022 08:07:23 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: > > Swap mutable and volatile in the permits list @jddarcy Could you help me file a CSR for this change? - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v2]
> 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: Swap mutable and volatile in the permits list - Changes: - all: https://git.openjdk.java.net/jdk/pull/7840/files - new: https://git.openjdk.java.net/jdk/pull/7840/files/ff8996e6..eef61c31 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7840=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7840=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
Re: RFR: 8283237: CallSite should be a sealed class
On Thu, 17 Mar 2022 07:32:40 GMT, liach wrote:
>> src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
>>
>>> 86: */
>>> 87: public
>>> 88: abstract sealed class CallSite permits ConstantCallSite,
>>> VolatileCallSite, MutableCallSite {
>>
>> Nitpicking with my JSR 292 hat,
>> given that the permits clause is reflected in the javadoc,
>> the order should be ConstantCS, MutableCS and VolatileCS,
>> it's both in the lexical order and in the "memory access" of setTarget()
>> order , from stronger access to weaker access.
>
> I agree that Constant, Mutable, Volatile order is better, ranked by the
> respective cost for `setTarget()` and (possibly) invocation, and earlier ones
> in the list are more preferable if conditions allow.
>
> However, in the current API documentation, the order is Constant, Mutable,
> and Volatile. Should I update that or leave it?
>
> /*
> *
> * If a mutable target is not required, an {@code invokedynamic}
> instruction
> * may be permanently bound by means of a {@linkplain ConstantCallSite
> constant call site}.
> * If a mutable target is required which has volatile variable semantics,
> * because updates to the target must be immediately and reliably witnessed
> by other threads,
> * a {@linkplain VolatileCallSite volatile call site} may be used.
> * Otherwise, if a mutable target is required,
> * a {@linkplain MutableCallSite mutable call site} may be used.
> *
> */
For me, this is unrelated, for this paragraph it's easier to explain the
semantics of MutableCallSite with an otherwise, i.e. it's mutable but you do
not want the cost of a volatile acces.
-
PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class
On Thu, 17 Mar 2022 06:47:13 GMT, Rémi Forax 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.
>
> src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
>
>> 86: */
>> 87: public
>> 88: abstract sealed class CallSite permits ConstantCallSite,
>> VolatileCallSite, MutableCallSite {
>
> Nitpicking with my JSR 292 hat,
> given that the permits clause is reflected in the javadoc,
> the order should be ConstantCS, MutableCS and VolatileCS,
> it's both in the lexical order and in the "memory access" of setTarget()
> order , from stronger access to weaker access.
I agree that Constant, Mutable, Volatile order is better, ranked by the
respective cost for `setTarget()` and (possibly) invocation, and earlier ones
in the list are more preferable if conditions allow.
However, in the current API documentation, the order is Constant, Mutable, and
Volatile. Should I update that or leave it?
/*
*
* If a mutable target is not required, an {@code invokedynamic} instruction
* may be permanently bound by means of a {@linkplain ConstantCallSite constant
call site}.
* If a mutable target is required which has volatile variable semantics,
* because updates to the target must be immediately and reliably witnessed by
other threads,
* a {@linkplain VolatileCallSite volatile call site} may be used.
* Otherwise, if a mutable target is required,
* a {@linkplain MutableCallSite mutable call site} may be used.
*
*/
-
PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class
On Wed, 16 Mar 2022 13:09:30 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.
src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
> 86: */
> 87: public
> 88: abstract sealed class CallSite permits ConstantCallSite,
> VolatileCallSite, MutableCallSite {
Nitpicking with my JSR 292 hat,
given that the permits clause is reflected in the javadoc,
the order should be ConstantCS, MutableCS and VolatileCS,
it's both in the lexical order and in the "memory access" of setTarget() order
, from stronger access to weaker access.
-
PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class
On Wed, 16 Mar 2022 13:09:30 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. Marked as reviewed by jkuhn (Author). - PR: https://git.openjdk.java.net/jdk/pull/7840
