Skip to content

8382306: Remove JvmtiTagMap::check_hashmap #30757

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
lmesnik wants to merge 1 commit into from
+1 −27
Closed

8382306: Remove JvmtiTagMap::check_hashmap #30757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 1 addition & 25 deletions src/hotspot/share/prims/jvmtiTagMap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,20 +136,6 @@ bool JvmtiTagMap::is_empty() {
return hashmap()->is_empty();
}

// This checks for posting before operations that use
// this tagmap table.
void JvmtiTagMap::check_hashmap(GrowableArray<jlong>* objects) {
assert(is_locked(), "checking");

if (is_empty()) { return; }

if (_needs_cleaning &&
objects != nullptr &&
env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)) {
remove_dead_entries_locked(objects);
}
}

// This checks for posting and is called from the heap walks.
void JvmtiTagMap::check_hashmaps_for_heapwalk(GrowableArray<jlong>* objects) {
assert(SafepointSynchronize::is_at_safepoint(), "called from safepoints");
Expand All @@ -162,7 +148,7 @@ void JvmtiTagMap::check_hashmaps_for_heapwalk(GrowableArray<jlong>* objects) {
if (tag_map != nullptr) {
// The ZDriver may be walking the hashmaps concurrently so this lock is needed.
MutexLocker ml(tag_map->lock(), Mutex::_no_safepoint_check_flag);
tag_map->check_hashmap(objects);
tag_map->remove_dead_entries_locked(objects);
Comment on lines -165 to +151
Copy link
Copy Markdown
Member

@dholmes-ora dholmes-ora Apr 16, 2026

Choose a reason for hiding this comment

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

Seems the new code no longer skips processing in the is_empty() case - IIUC that will just be an extra clearing of weak refs if anything.

Copy link
Copy Markdown
Member Author

@lmesnik lmesnik Apr 16, 2026

Choose a reason for hiding this comment

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

I don't think it is important here. The check_hashmaps_for_heapwalk is called once during heapwalking operation which is extremely slow already.

Copy link
Copy Markdown
Member Author

@lmesnik lmesnik Apr 17, 2026

Choose a reason for hiding this comment

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

The another difference observed by @sspitsyn while discussing this issue.
The remove_dead_entries_locked() is now called and work even if
env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)
is false.
It just cleans up tagmap table, but doesn't post events. Shouldn't change behaviour observed by jvmti agent.
The performance impact is not significant comparing with heap walking.

Copy link
Copy Markdown
Contributor

@sspitsyn sspitsyn Apr 17, 2026

Choose a reason for hiding this comment

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

One more difference that the check_hashmaps_for_heapwalk() is also called when objects == nullptr.

Copy link
Copy Markdown
Member Author

@lmesnik lmesnik Apr 17, 2026

Choose a reason for hiding this comment

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

check_hashmaps_for_heapwalk()
never called with objects == nullptr
The goal of this function is to post ObjectFree events.
I still wonder why is it needed? It collect tags for dead objects before heap walking and post them right after
corresponding VMOp is completed.
It is really unclear for me if there are any reasons to do this.

}
}
}
Expand Down Expand Up @@ -329,11 +315,6 @@ class TwoOopCallbackWrapper : public CallbackWrapper {
void JvmtiTagMap::set_tag(jobject object, jlong tag) {
MutexLocker ml(lock(), Mutex::_no_safepoint_check_flag);

// SetTag should not post events because the JavaThread has to
// transition to native for the callback and this cannot stop for
// safepoints with the hashmap lock held.
check_hashmap(nullptr); /* don't collect dead objects */

// resolve the object
oop o = JNIHandles::resolve_non_null(object);

Expand All @@ -354,11 +335,6 @@ void JvmtiTagMap::set_tag(jobject object, jlong tag) {
jlong JvmtiTagMap::get_tag(jobject object) {
MutexLocker ml(lock(), Mutex::_no_safepoint_check_flag);

// GetTag should not post events because the JavaThread has to
// transition to native for the callback and this cannot stop for
// safepoints with the hashmap lock held.
check_hashmap(nullptr); /* don't collect dead objects */

// resolve the object
oop o = JNIHandles::resolve_non_null(object);

Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/prims/jvmtiTagMap.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@ class JvmtiTagMap : public CHeapObj<mtServiceability> {
// accessors
inline JvmtiEnv* env() const { return _env; }

void check_hashmap(GrowableArray<jlong>* objects);

void entry_iterate(JvmtiTagMapKeyClosure* closure);

public:
Expand Down