Re: RFR: 8306851: Move Method access flags [v3]
On Fri, 28 Apr 2023 18:39:21 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/oops/constMethod.cpp line 438:
>>
>>> 436: }
>>> 437: st->cr();
>>> 438: st->print(" - flags: "); _flags.print_on(st);
>>>st->cr();
>>
>> Method prints its flags as an int and in decoded form, but ConstMethod only
>> prints the decoded form. Any particular reason for this difference?
>
> No reason for this difference. The only reason I added the int form for
> MethodFlags was because AccessFlags were printed also with the int form.
I fixed the constMethod printing with the last commit.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180757104
Re: RFR: 8306851: Move Method access flags [v3]
On Fri, 28 Apr 2023 18:29:50 GMT, Frederic Parain wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove bool argument from ConstMethodFlags.set function.
>
> src/hotspot/share/oops/constMethod.cpp line 438:
>
>> 436: }
>> 437: st->cr();
>> 438: st->print(" - flags: "); _flags.print_on(st);
>> st->cr();
>
> Method prints its flags as an int and in decoded form, but ConstMethod only
> prints the decoded form. Any particular reason for this difference?
No reason for this difference. The only reason I added the int form for
MethodFlags was because AccessFlags were printed also with the int form.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180712196
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.
Thank you for all those cleanings!
Looks good to me.
src/hotspot/share/oops/constMethod.cpp line 438:
> 436: }
> 437: st->cr();
> 438: st->print(" - flags: "); _flags.print_on(st);
> st->cr();
Method prints its flags as an int and in decoded form, but ConstMethod only
prints the decoded form. Any particular reason for this difference?
-
Marked as reviewed by fparain (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1406421552
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180705106
Re: RFR: 8306851: Move Method access flags [v3]
On Fri, 28 Apr 2023 14:18:11 GMT, Matias Saavedra Silva
wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove bool argument from ConstMethodFlags.set function.
>
> 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.
The reason I left that was to anchor the comment. There is nowhere good to put
that in the X macro. Also, didn't want to fix the callers. It's a good point
about making monitor_matching() private, but also not really doable with the X
macro. So that's why I left it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180548707
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: 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. Marked as reviewed by dnsimon (Committer). Thankfully all these changes only impact values read by JVMCI Java code and none in [Graal Java code](https://github.com/oracle/graal/blob/114067fc41d97e6c07f6de9bd745196d6f967ae4/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java#L47). Looks good to me. - PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1405368377 PR Comment: https://git.openjdk.org/jdk/pull/13654#issuecomment-1527109990
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.
Thanks for the updates. I understand about the fanout from making `is` naming
fully consistent.
src/hotspot/share/oops/method.hpp line 875:
> 873: boolis_not_c1_osr_compilable() const{ return
> is_not_c1_compilable(); }
> 874: void set_is_not_c1_osr_compilable() {
> set_is_not_c1_compilable(); }
> 875: void clear_is_not_c1_osr_compilable() {
> clear_is_not_c1_compilable(); }
Nit: don't need extra spaces after `{`
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1405267857
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1179956725
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. @dougxc can you look at the JVMCI changes? - PR Comment: https://git.openjdk.org/jdk/pull/13654#issuecomment-1525977245
Re: RFR: 8306851: Move Method access flags [v3]
> 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. - Changes: - all: https://git.openjdk.org/jdk/pull/13654/files - new: https://git.openjdk.org/jdk/pull/13654/files/fc5bcaa6..6687cc0e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13654&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13654&range=01-02 Stats: 9 lines in 2 files changed: 0 ins; 5 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13654.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13654/head:pull/13654 PR: https://git.openjdk.org/jdk/pull/13654
