Skip to content

8346567: Make Class.getModifiers() non-native #22652

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

8346567: Make Class.getModifiers() non-native #22652

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ec546ae
8345678: Make Class.getModifiers() non-native.
coleenp Dec 9, 2024
86bc216
Fix JFR bug.
coleenp Jan 31, 2025
8854fcc
Removed @Stable.
coleenp Feb 3, 2025
ff69341
Fix copyright and param name
coleenp Feb 4, 2025
f92620e
Update src/hotspot/share/opto/memnode.cpp
coleenp Feb 6, 2025
8502636
Add Klass::modifier_flags to look in the mirror, restore ciKlass::mod…
coleenp Feb 6, 2025
146e255
Make compute_modifiers return u2.
coleenp Feb 6, 2025
2ea6664
Hide Class.modifiers field.
coleenp Feb 6, 2025
304a17e
Remove ??? in the code.
coleenp Feb 6, 2025
37a8cf8
Fix jvmci test.
coleenp Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/hotspot/share/ci/ciKlass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,15 @@ ciInstance* ciKlass::java_mirror() {
)
}

// ------------------------------------------------------------------
// ciKlass::modifier_flags
jint ciKlass::modifier_flags() {
assert(is_loaded(), "not loaded");
GUARDED_VM_ENTRY(
return get_Klass()->modifier_flags();
)
}

// ------------------------------------------------------------------
// ciKlass::access_flags
jint ciKlass::access_flags() {
Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/ci/ciKlass.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ class ciKlass : public ciType {
// Get the instance of java.lang.Class corresponding to this klass.
ciInstance* java_mirror();

// Fetch modifier flags.
jint modifier_flags();

// Fetch Klass::access_flags.
jint access_flags();

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/compiler/compileLog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ int CompileLog::identify(ciBaseObject* obj) {
if (!klass->is_loaded()) {
print(" unloaded='1'");
} else {
print(" flags='%d'", klass->access_flags());
print(" flags='%d'", klass->modifier_flags());
}
end_elem();
} else if (mobj->is_method()) {
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ static void do_write_klass(JfrCheckpointWriter* writer, CldPtr cld, KlassPtr kla
writer->write(cld != nullptr ? cld_id(cld, leakp) : 0);
writer->write(mark_symbol(klass, leakp));
writer->write(package_id(klass, leakp));
writer->write(klass->compute_modifier_flags());
writer->write(klass->modifier_flags());
writer->write<bool>(klass->is_hidden());
if (leakp) {
assert(IS_LEAKP(klass), "invariant");
Expand Down
6 changes: 6 additions & 0 deletions src/hotspot/share/oops/klass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,12 @@ jint Klass::array_layout_helper(BasicType etype) {
return lh;
}

int Klass::modifier_flags() const {
int mods = java_lang_Class::modifiers(java_mirror());
assert(mods == compute_modifier_flags(), "should be same");
return mods;
}

bool Klass::can_be_primary_super_slow() const {
if (super() == nullptr)
return true;
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/oops/klass.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,7 @@ class Klass : public Metadata {

public:
virtual int compute_modifier_flags() const = 0;
int modifier_flags() const;

// JVMTI support
virtual jint jvmti_class_status() const;
Expand Down
7 changes: 7 additions & 0 deletions test/micro/org/openjdk/bench/java/lang/reflect/Clazz.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@ public int getModifiers() {
public int getAppArrayModifiers() {
return clazzArray.getClass().getModifiers();
}
Copy link
Member

@dean-long dean-long Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this is the benchmark that shows an extra load. How about adding a benchmark that makes the Clazz[] final or @stable, and see if that makes the extra load go away?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name Cnt Base Error Test Error Unit Change
getAppArrayModifiers 30 0.923 ± 0.004 1.260 ± 0.001 ns/op 0.73x (p = 0.000*)
getAppArrayModifiersFinal 30 0.922 ± 0.000 1.260 ± 0.001 ns/op 0.73x (p = 0.000*)

No it doesn't really help. There's still an extra load.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, if the extra load turns out to be a problem in the future, we could look into why the compilers are generating the load when the Class is known/constant. If the old intrinsic was able to pull the constant out of the Klass, then surely we can do the same and pull the value from the Class field.

Copy link
Member

@liach liach Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does static final help here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Yes it does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cases when a class mirror is a compile-time constant are already well-optimized. Non constant cases are the ones where missing optimization opportunities arise.

In this particular case, C2 doesn't benefit from the observation that Clazz[] is a leaf type at runtime (no subclasses). Hence, a value loaded from a field typed as Clazz[] has exactly the same type and clazzArray.getClass() can be constant folded to Clazz[].class. Rather than a common case, it feels more like a corner case. So, worth addressing as a follow-up enhancement.

Another scenario is a meet of 2 primitive array types (ends up as bottom[] in C2 type system), but I believe it hasn't been optimized before.


static final Clazz[] clazzArrayFinal = new Clazz[1];
@Benchmark
public int getAppArrayModifiersFinal() {
return clazzArrayFinal.getClass().getModifiers();
}

/**
* Get modifiers for an primitive array class through reflection
*
Expand Down