Skip to content

8302108: Clean up placeholder supername code #12495

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
coleenp wants to merge 3 commits into from
Closed

8302108: Clean up placeholder supername code #12495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d5fd3c
8302108: Clean up placeholder supername code
coleenp Feb 8, 2023
fab1114
Merge branch 'master' into placeholder-cleanup
coleenp Feb 10, 2023
378170e
Fix test for cleanup for nulling out supername when no longer used.
coleenp Feb 10, 2023
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
27 changes: 15 additions & 12 deletions src/hotspot/share/classfile/placeholders.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,11 @@ bool PlaceholderEntry::remove_seen_thread(JavaThread* thread, PlaceholderTable::
}


// Placeholder methods
void PlaceholderEntry::set_supername(Symbol* supername) {
assert_locked_or_safepoint(SystemDictionary_lock);
assert(_supername == nullptr || _supername->refcount() > 1, "must be referenced also by the loader");
_supername = supername;
}

// Placeholder objects represent classes currently being loaded.
// All threads examining the placeholder table must hold the
Expand Down Expand Up @@ -272,7 +276,6 @@ PlaceholderEntry* PlaceholderTable::find_and_add(Symbol* name,


// placeholder is used to track class loading internal states
// placeholder existence now for loading superclass/superinterface
// superthreadQ tracks class circularity, while loading superclass/superinterface
// loadInstanceThreadQ tracks load_instance_class calls
// definer() tracks the single thread that owns define token
Expand All @@ -283,21 +286,21 @@ PlaceholderEntry* PlaceholderTable::find_and_add(Symbol* name,
// On removal: if definer and all queues empty, remove entry
// Note: you can be in both placeholders and systemDictionary
// Therefore - must always check SD first
// Ignores the case where entry is not found
void PlaceholderTable::find_and_remove(Symbol* name, ClassLoaderData* loader_data,
classloadAction action,
JavaThread* thread) {
assert_locked_or_safepoint(SystemDictionary_lock);
PlaceholderEntry* probe = get_entry(name, loader_data);
if (probe != nullptr) {
log(name, probe, "find_and_remove", action);
probe->remove_seen_thread(thread, action);
// If no other threads using this entry, and this thread is not using this entry for other states
if ((probe->superThreadQ() == nullptr) && (probe->loadInstanceThreadQ() == nullptr)
&& (probe->defineThreadQ() == nullptr) && (probe->definer() == nullptr)) {
probe->clear_supername();
remove_entry(name, loader_data);
}
assert(probe != nullptr, "must find an entry");
log(name, probe, "find_and_remove", action);
probe->remove_seen_thread(thread, action);
if (probe->superThreadQ() == nullptr) {
probe->set_supername(nullptr);
}
// If no other threads using this entry, and this thread is not using this entry for other states
if ((probe->superThreadQ() == nullptr) && (probe->loadInstanceThreadQ() == nullptr)
&& (probe->defineThreadQ() == nullptr) && (probe->definer() == nullptr)) {
remove_entry(name, loader_data);
}
}

Expand Down
35 changes: 12 additions & 23 deletions src/hotspot/share/classfile/placeholders.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#ifndef SHARE_CLASSFILE_PLACEHOLDERS_HPP
#define SHARE_CLASSFILE_PLACEHOLDERS_HPP

#include "oops/symbol.hpp"
#include "oops/symbolHandle.hpp"

class PlaceholderEntry;
class Thread;
Expand Down Expand Up @@ -81,7 +81,7 @@ class SeenThread;
class PlaceholderEntry {
friend class PlaceholderTable;
private:
Symbol* _supername;
SymbolHandle _supername;
JavaThread* _definer; // owner of define token
InstanceKlass* _instanceKlass; // InstanceKlass from successful define
SeenThread* _superThreadQ; // doubly-linked queue of Threads loading a superclass for this class
Expand All @@ -99,39 +99,28 @@ class PlaceholderEntry {
void add_seen_thread(JavaThread* thread, PlaceholderTable::classloadAction action);
bool remove_seen_thread(JavaThread* thread, PlaceholderTable::classloadAction action);

SeenThread* superThreadQ() const { return _superThreadQ; }
Copy link
Member

@iklam iklam Feb 13, 2023

Choose a reason for hiding this comment

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

If you just want to change these function to private, instead of moving them, I would suggest adding private: and public: keywords around them. That way we can limit the delta.

Copy link
Contributor Author

@coleenp coleenp Feb 13, 2023

Choose a reason for hiding this comment

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

I really wanted them moved to the section of the class where the other private methods are. This class isn't so overgrown that we need to keep switching from private to public, and I want the public methods to be together.
Java is nice in that you can have 'private' and 'public' keywords on each function, but I'd still like to keep the public ones together.

void set_superThreadQ(SeenThread* SeenThread) { _superThreadQ = SeenThread; }

SeenThread* loadInstanceThreadQ() const { return _loadInstanceThreadQ; }
void set_loadInstanceThreadQ(SeenThread* SeenThread) { _loadInstanceThreadQ = SeenThread; }

SeenThread* defineThreadQ() const { return _defineThreadQ; }
void set_defineThreadQ(SeenThread* SeenThread) { _defineThreadQ = SeenThread; }
public:
PlaceholderEntry() :
_supername(nullptr), _definer(nullptr), _instanceKlass(nullptr),
_definer(nullptr), _instanceKlass(nullptr),
_superThreadQ(nullptr), _loadInstanceThreadQ(nullptr), _defineThreadQ(nullptr) { }

Symbol* supername() const { return _supername; }
void set_supername(Symbol* supername) {
if (supername != _supername) {
Symbol::maybe_decrement_refcount(_supername);
_supername = supername;
Symbol::maybe_increment_refcount(_supername);
}
}
void clear_supername() {
Symbol::maybe_decrement_refcount(_supername);
_supername = nullptr;
}
void set_supername(Symbol* supername);

JavaThread* definer() const {return _definer; }
void set_definer(JavaThread* definer) { _definer = definer; }

InstanceKlass* instance_klass() const {return _instanceKlass; }
void set_instance_klass(InstanceKlass* ik) { _instanceKlass = ik; }

SeenThread* superThreadQ() const { return _superThreadQ; }
void set_superThreadQ(SeenThread* SeenThread) { _superThreadQ = SeenThread; }

SeenThread* loadInstanceThreadQ() const { return _loadInstanceThreadQ; }
void set_loadInstanceThreadQ(SeenThread* SeenThread) { _loadInstanceThreadQ = SeenThread; }

SeenThread* defineThreadQ() const { return _defineThreadQ; }
void set_defineThreadQ(SeenThread* SeenThread) { _defineThreadQ = SeenThread; }

bool super_load_in_progress() {
return (_superThreadQ != nullptr);
}
Expand Down
3 changes: 1 addition & 2 deletions src/hotspot/share/oops/symbolHandle.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@ class SymbolHandleBase : public StackObj {
// Does not increment the current reference count if temporary.
SymbolHandleBase(Symbol *s) : _temp(s) {
if (!TEMP) {
assert(s != nullptr, "must not be null");
s->increment_refcount();
Symbol::maybe_increment_refcount(_temp);
}
}

Expand Down
4 changes: 1 addition & 3 deletions test/hotspot/gtest/classfile/test_placeholders.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,12 @@ TEST_VM(PlaceholderTable, supername) {
PlaceholderTable::find_and_add(D, loader_data, super_action, interf, T2);
PlaceholderTable::find_and_remove(D, loader_data, super_action, T2);

ASSERT_EQ(interf->refcount(), 3) << "supername isn't replaced until super set";
ASSERT_EQ(interf->refcount(), 1) << "supername is replaced with null";

// Add placeholder to the table for loading A and super, and D also loading super
PlaceholderTable::find_and_add(A, loader_data, super_action, super, THREAD);
PlaceholderTable::find_and_add(D, loader_data, super_action, super, T2);

ASSERT_EQ(interf->refcount(), 1) << "now should be one";

// Another thread comes in and finds A loading Super
PlaceholderEntry* placeholder = PlaceholderTable::get_entry(A, loader_data);
SymbolHandle supername = placeholder->supername();
Expand Down