Clean up class visitors
Move from function pointers to virtual function visitors.
Change-Id: I68cb83c1d2ed9b5a89f8e534fe7ca4bbc1c91f45
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index ce54c14..3883246 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -1332,16 +1332,16 @@
}
}
-void ClassLinker::VisitClassesInternal(ClassVisitor* visitor, void* arg) {
+void ClassLinker::VisitClassesInternal(ClassVisitor* visitor) {
for (auto& pair : classes_) {
ClassTable* const class_table = pair.second;
- if (!class_table->Visit(visitor, arg)) {
+ if (!class_table->Visit(visitor)) {
return;
}
}
}
-void ClassLinker::VisitClasses(ClassVisitor* visitor, void* arg) {
+void ClassLinker::VisitClasses(ClassVisitor* visitor) {
if (dex_cache_image_class_lookup_required_) {
MoveImageClassesToClassTable();
}
@@ -1350,79 +1350,85 @@
// Not safe to have thread suspension when we are holding a lock.
if (self != nullptr) {
ScopedAssertNoThreadSuspension nts(self, __FUNCTION__);
- VisitClassesInternal(visitor, arg);
+ VisitClassesInternal(visitor);
} else {
- VisitClassesInternal(visitor, arg);
+ VisitClassesInternal(visitor);
}
}
-static bool GetClassesVisitorSet(mirror::Class* c, void* arg) {
- std::set<mirror::Class*>* classes = reinterpret_cast<std::set<mirror::Class*>*>(arg);
- classes->insert(c);
- return true;
-}
-
-struct GetClassesVisitorArrayArg {
- Handle<mirror::ObjectArray<mirror::Class>>* classes;
- int32_t index;
- bool success;
+class GetClassesInToVector : public ClassVisitor {
+ public:
+ bool Visit(mirror::Class* klass) OVERRIDE {
+ classes_.push_back(klass);
+ return true;
+ }
+ std::vector<mirror::Class*> classes_;
};
-static bool GetClassesVisitorArray(mirror::Class* c, void* varg)
- SHARED_REQUIRES(Locks::mutator_lock_) {
- GetClassesVisitorArrayArg* arg = reinterpret_cast<GetClassesVisitorArrayArg*>(varg);
- if (arg->index < (*arg->classes)->GetLength()) {
- (*arg->classes)->Set(arg->index, c);
- arg->index++;
- return true;
- } else {
- arg->success = false;
+class GetClassInToObjectArray : public ClassVisitor {
+ public:
+ explicit GetClassInToObjectArray(mirror::ObjectArray<mirror::Class>* arr)
+ : arr_(arr), index_(0) {}
+
+ bool Visit(mirror::Class* klass) OVERRIDE SHARED_REQUIRES(Locks::mutator_lock_) {
+ ++index_;
+ if (index_ <= arr_->GetLength()) {
+ arr_->Set(index_ - 1, klass);
+ return true;
+ }
return false;
}
-}
-void ClassLinker::VisitClassesWithoutClassesLock(ClassVisitor* visitor, void* arg) {
+ bool Succeeded() const SHARED_REQUIRES(Locks::mutator_lock_) {
+ return index_ <= arr_->GetLength();
+ }
+
+ private:
+ mirror::ObjectArray<mirror::Class>* const arr_;
+ int32_t index_;
+};
+
+void ClassLinker::VisitClassesWithoutClassesLock(ClassVisitor* visitor) {
// TODO: it may be possible to avoid secondary storage if we iterate over dex caches. The problem
// is avoiding duplicates.
if (!kMovingClasses) {
- std::set<mirror::Class*> classes;
- VisitClasses(GetClassesVisitorSet, &classes);
- for (mirror::Class* klass : classes) {
- if (!visitor(klass, arg)) {
+ GetClassesInToVector accumulator;
+ VisitClasses(&accumulator);
+ for (mirror::Class* klass : accumulator.classes_) {
+ if (!visitor->Visit(klass)) {
return;
}
}
} else {
- Thread* self = Thread::Current();
+ Thread* const self = Thread::Current();
StackHandleScope<1> hs(self);
- MutableHandle<mirror::ObjectArray<mirror::Class>> classes =
- hs.NewHandle<mirror::ObjectArray<mirror::Class>>(nullptr);
- GetClassesVisitorArrayArg local_arg;
- local_arg.classes = &classes;
- local_arg.success = false;
+ auto classes = hs.NewHandle<mirror::ObjectArray<mirror::Class>>(nullptr);
// We size the array assuming classes won't be added to the class table during the visit.
// If this assumption fails we iterate again.
- while (!local_arg.success) {
+ while (true) {
size_t class_table_size;
{
ReaderMutexLock mu(self, *Locks::classlinker_classes_lock_);
- class_table_size = NumZygoteClasses() + NumNonZygoteClasses();
+ // Add 100 in case new classes get loaded when we are filling in the object array.
+ class_table_size = NumZygoteClasses() + NumNonZygoteClasses() + 100;
}
mirror::Class* class_type = mirror::Class::GetJavaLangClass();
mirror::Class* array_of_class = FindArrayClass(self, &class_type);
classes.Assign(
mirror::ObjectArray<mirror::Class>::Alloc(self, array_of_class, class_table_size));
CHECK(classes.Get() != nullptr); // OOME.
- local_arg.index = 0;
- local_arg.success = true;
- VisitClasses(GetClassesVisitorArray, &local_arg);
+ GetClassInToObjectArray accumulator(classes.Get());
+ VisitClasses(&accumulator);
+ if (accumulator.Succeeded()) {
+ break;
+ }
}
for (int32_t i = 0; i < classes->GetLength(); ++i) {
// If the class table shrank during creation of the clases array we expect null elements. If
// the class table grew then the loop repeats. If classes are created after the loop has
// finished then we don't visit.
mirror::Class* klass = classes->Get(i);
- if (klass != nullptr && !visitor(klass, arg)) {
+ if (klass != nullptr && !visitor->Visit(klass)) {
return;
}
}
@@ -5563,13 +5569,22 @@
return dex_file.GetMethodShorty(method_id, length);
}
-bool DumpClassVisitor(mirror::Class* klass, void* arg) SHARED_REQUIRES(Locks::mutator_lock_) {
- klass->DumpClass(LOG(ERROR), reinterpret_cast<ssize_t>(arg));
- return true;
-}
+class DumpClassVisitor : public ClassVisitor {
+ public:
+ explicit DumpClassVisitor(int flags) : flags_(flags) {}
+
+ bool Visit(mirror::Class* klass) OVERRIDE SHARED_REQUIRES(Locks::mutator_lock_) {
+ klass->DumpClass(LOG(ERROR), flags_);
+ return true;
+ }
+
+ private:
+ const int flags_;
+};
void ClassLinker::DumpAllClasses(int flags) {
- VisitClasses(&DumpClassVisitor, reinterpret_cast<void*>(flags));
+ DumpClassVisitor visitor(flags);
+ VisitClasses(&visitor);
}
static OatFile::OatMethod CreateOatMethod(const void* code) {
diff --git a/runtime/class_linker.h b/runtime/class_linker.h
index 1337563..c53ff61 100644
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -288,14 +288,14 @@
const OatFile* GetPrimaryOatFile()
REQUIRES(!dex_lock_);
- void VisitClasses(ClassVisitor* visitor, void* arg)
+ void VisitClasses(ClassVisitor* visitor)
REQUIRES(!Locks::classlinker_classes_lock_) SHARED_REQUIRES(Locks::mutator_lock_);
// Less efficient variant of VisitClasses that copies the class_table_ into secondary storage
// so that it can visit individual classes without holding the doesn't hold the
// Locks::classlinker_classes_lock_. As the Locks::classlinker_classes_lock_ isn't held this code
// can race with insertion and deletion of classes while the visitor is being called.
- void VisitClassesWithoutClassesLock(ClassVisitor* visitor, void* arg)
+ void VisitClassesWithoutClassesLock(ClassVisitor* visitor)
SHARED_REQUIRES(Locks::mutator_lock_) REQUIRES(!dex_lock_);
void VisitClassRoots(RootVisitor* visitor, VisitRootFlags flags)
@@ -486,7 +486,7 @@
typedef SafeMap<GcRoot<mirror::ClassLoader>, ClassTable*, CompareClassLoaderGcRoot>
ClassLoaderClassTable;
- void VisitClassesInternal(ClassVisitor* visitor, void* arg)
+ void VisitClassesInternal(ClassVisitor* visitor)
REQUIRES(Locks::classlinker_classes_lock_) SHARED_REQUIRES(Locks::mutator_lock_);
// Returns the number of zygote and image classes.
diff --git a/runtime/class_table.cc b/runtime/class_table.cc
index f775235..c245d4e 100644
--- a/runtime/class_table.cc
+++ b/runtime/class_table.cc
@@ -71,10 +71,10 @@
}
}
-bool ClassTable::Visit(ClassVisitor* visitor, void* arg) {
+bool ClassTable::Visit(ClassVisitor* visitor) {
for (ClassSet& class_set : classes_) {
for (GcRoot<mirror::Class>& root : class_set) {
- if (!visitor(root.Read(), arg)) {
+ if (!visitor->Visit(root.Read())) {
return false;
}
}
diff --git a/runtime/class_table.h b/runtime/class_table.h
index af25131..252a47d 100644
--- a/runtime/class_table.h
+++ b/runtime/class_table.h
@@ -36,7 +36,12 @@
class ClassLoader;
} // namespace mirror
-typedef bool (ClassVisitor)(mirror::Class* c, void* arg);
+class ClassVisitor {
+ public:
+ virtual ~ClassVisitor() {}
+ // Return true to continue visiting.
+ virtual bool Visit(mirror::Class* klass) = 0;
+};
// Each loader has a ClassTable
class ClassTable {
@@ -66,7 +71,7 @@
REQUIRES(Locks::classlinker_classes_lock_) SHARED_REQUIRES(Locks::mutator_lock_);
// Return false if the callback told us to exit.
- bool Visit(ClassVisitor* visitor, void* arg)
+ bool Visit(ClassVisitor* visitor)
REQUIRES(Locks::classlinker_classes_lock_) SHARED_REQUIRES(Locks::mutator_lock_);
mirror::Class* Lookup(const char* descriptor, size_t hash)
diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 287a50b..f0de65b 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -948,33 +948,27 @@
return JDWP::ERR_NONE;
}
+// Get the complete list of reference classes (i.e. all classes except
+// the primitive types).
+// Returns a newly-allocated buffer full of RefTypeId values.
+class ClassListCreator : public ClassVisitor {
+ public:
+ explicit ClassListCreator(std::vector<JDWP::RefTypeId>* classes) : classes_(classes) {}
+
+ bool Visit(mirror::Class* c) OVERRIDE SHARED_REQUIRES(Locks::mutator_lock_) {
+ if (!c->IsPrimitive()) {
+ classes_->push_back(Dbg::GetObjectRegistry()->AddRefType(c));
+ }
+ return true;
+ }
+
+ private:
+ std::vector<JDWP::RefTypeId>* const classes_;
+};
+
void Dbg::GetClassList(std::vector<JDWP::RefTypeId>* classes) {
- // Get the complete list of reference classes (i.e. all classes except
- // the primitive types).
- // Returns a newly-allocated buffer full of RefTypeId values.
- struct ClassListCreator {
- explicit ClassListCreator(std::vector<JDWP::RefTypeId>* classes_in) : classes(classes_in) {
- }
-
- static bool Visit(mirror::Class* c, void* arg) {
- return reinterpret_cast<ClassListCreator*>(arg)->Visit(c);
- }
-
- // TODO: Enable annotalysis. We know lock is held in constructor, but abstraction confuses
- // annotalysis.
- bool Visit(mirror::Class* c) NO_THREAD_SAFETY_ANALYSIS {
- if (!c->IsPrimitive()) {
- classes->push_back(gRegistry->AddRefType(c));
- }
- return true;
- }
-
- std::vector<JDWP::RefTypeId>* const classes;
- };
-
ClassListCreator clc(classes);
- Runtime::Current()->GetClassLinker()->VisitClassesWithoutClassesLock(ClassListCreator::Visit,
- &clc);
+ Runtime::Current()->GetClassLinker()->VisitClassesWithoutClassesLock(&clc);
}
JDWP::JdwpError Dbg::GetClassInfo(JDWP::RefTypeId class_id, JDWP::JdwpTypeTag* pTypeTag,
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index 9711cf2..e28d578 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -49,12 +49,20 @@
static constexpr StackVisitor::StackWalkKind kInstrumentationStackWalk =
StackVisitor::StackWalkKind::kSkipInlinedFrames;
-static bool InstallStubsClassVisitor(mirror::Class* klass, void* arg)
- REQUIRES(Locks::mutator_lock_) {
- Instrumentation* instrumentation = reinterpret_cast<Instrumentation*>(arg);
- instrumentation->InstallStubsForClass(klass);
- return true; // we visit all classes.
-}
+class InstallStubsClassVisitor : public ClassVisitor {
+ public:
+ explicit InstallStubsClassVisitor(Instrumentation* instrumentation)
+ : instrumentation_(instrumentation) {}
+
+ bool Visit(mirror::Class* klass) OVERRIDE REQUIRES(Locks::mutator_lock_) {
+ instrumentation_->InstallStubsForClass(klass);
+ return true; // we visit all classes.
+ }
+
+ private:
+ Instrumentation* const instrumentation_;
+};
+
Instrumentation::Instrumentation()
: instrumentation_stubs_installed_(false), entry_exit_stubs_installed_(false),
@@ -563,14 +571,16 @@
entry_exit_stubs_installed_ = true;
interpreter_stubs_installed_ = false;
}
- runtime->GetClassLinker()->VisitClasses(InstallStubsClassVisitor, this);
+ InstallStubsClassVisitor visitor(this);
+ runtime->GetClassLinker()->VisitClasses(&visitor);
instrumentation_stubs_installed_ = true;
MutexLock mu(self, *Locks::thread_list_lock_);
runtime->GetThreadList()->ForEach(InstrumentationInstallStack, this);
} else {
interpreter_stubs_installed_ = false;
entry_exit_stubs_installed_ = false;
- runtime->GetClassLinker()->VisitClasses(InstallStubsClassVisitor, this);
+ InstallStubsClassVisitor visitor(this);
+ runtime->GetClassLinker()->VisitClasses(&visitor);
// Restore stack only if there is no method currently deoptimized.
bool empty;
{