On Tue, 25 Apr 2023 19:09:23 GMT, Coleen Phillimore <cole...@openjdk.org> 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.
General idea is good but I have some issues with naming inconsistencies. A few
other queries.
Thanks.
src/hotspot/share/classfile/classFileParser.cpp line 2741:
> 2739: parsed_annotations.apply_to(methodHandle(THREAD, m));
> 2740:
> 2741: if (is_hidden() && !m->is_hidden()) { // Mark methods in hidden
> classes as 'hidden'.
This seems odd - how would m already be marked hidden? And why do we care? The
check-and-branch is more expensive than just setting the field.
src/hotspot/share/oops/constMethodFlags.hpp line 34:
> 32:
> 33: // The ConstMethodFlags class contains the parse-time flags associated
> with
> 34: // an Method, and their associated accessors.
s/an/a/
s/their/its/
src/hotspot/share/oops/constMethodFlags.hpp line 53:
> 51: flag(has_type_annotations , 1 << 9) \
> 52: flag(has_default_annotations , 1 << 10) \
> 53: flag(caller_sensitive , 1 << 11) \
Nit: we should consistently use either `x` or `is_x` for `x` in `overpass,
caller_sensitive, hidden, scoped, ...`
src/hotspot/share/oops/method.cpp line 735:
> 733: case Bytecodes::_jsr:
> 734: if (bcs.dest() < bcs.next_bci()) {
> 735: return set_has_loops();
I don't understand the new return logic here. The break gets us out of the
switch but we are still in the while loop, but the return takes us all the way
out. ???
src/hotspot/share/oops/method.hpp line 808:
> 806:
> 807: bool is_hidden() const { return constMethod()->is_hidden(); }
> 808: void set_hidden() { constMethod()->set_is_hidden(); }
The naming is inconsistent here regarding `is` - either both should have it or
neither IMO.
src/hotspot/share/oops/method.hpp line 871:
> 869: void clear_not_c2_compilable() {
> set_not_c2_compilable(false); }
> 870:
> 871: bool is_not_c1_osr_compilable() const { return
> not_c1_compilable(); } // don't waste a flags bit
Again inconsistent naming with `is` but also `osr` in this case. I would expect
being compilable to be quite different to being "osr compilable".
-------------
PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1403149129
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1178611573
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1178613648
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1178614433
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1178618364
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1178619597
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1178620558
