Skip to content
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.

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

8193559: ugly DO_JAVA_THREADS macro should be replaced #4671

Closed
wants to merge 5 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -94,7 +94,6 @@
#include "runtime/objectMonitor.hpp"
#include "runtime/orderAccess.hpp"
#include "runtime/osThread.hpp"
#include "runtime/prefetch.inline.hpp"
#include "runtime/safepoint.hpp"
#include "runtime/safepointMechanism.inline.hpp"
#include "runtime/safepointVerifiers.hpp"
@@ -2478,35 +2477,6 @@ size_t JavaThread::_stack_size_at_create = 0;
bool Threads::_vm_complete = false;
#endif

static inline JavaThread *prefetch_and_load_ptr(JavaThread*const * addr, intx prefetch_interval) {
Prefetch::read((void*)addr, prefetch_interval);
return *addr;
}

struct JavaThreadPrefetchedIterator {
JavaThread*const * _end;
JavaThread*const * _current;

JavaThreadPrefetchedIterator(ThreadsList* list) :
_end(list->threads() + list->length()), _current(list->threads()) {}

JavaThread* current() {
return _current != _end
? prefetch_and_load_ptr(_current, PrefetchScanIntervalInBytes)
: NULL;
}

void next() {
_current++;
}
};

#define DO_JAVA_THREADS(LIST, X) \
for (JavaThreadPrefetchedIterator iter(LIST); JavaThread* X = iter.current(); iter.next())

// All JavaThreads
#define ALL_JAVA_THREADS(X) DO_JAVA_THREADS(ThreadsSMRSupport::get_java_thread_list(), X)

// All NonJavaThreads (i.e., every non-JavaThread in the system).
void Threads::non_java_threads_do(ThreadClosure* tc) {
NoSafepointVerifier nsv;
@@ -2515,6 +2485,10 @@ void Threads::non_java_threads_do(ThreadClosure* tc) {
}
}

// All JavaThreads
#define ALL_JAVA_THREADS(X) \
for (JavaThread* X : *ThreadsSMRSupport::get_java_thread_list())

// All JavaThreads
void Threads::java_threads_do(ThreadClosure* tc) {
assert_locked_or_safepoint(Threads_lock);
@@ -3648,7 +3622,7 @@ GrowableArray<JavaThread*>* Threads::get_pending_threads(ThreadsList * t_list,
GrowableArray<JavaThread*>* result = new GrowableArray<JavaThread*>(count);

int i = 0;
DO_JAVA_THREADS(t_list, p) {
for (JavaThread* p : *t_list) {
if (!p->can_call_java()) continue;

// The first stage of async deflation does not affect any field
@@ -3669,7 +3643,7 @@ JavaThread *Threads::owning_thread_from_monitor_owner(ThreadsList * t_list,
// NULL owner means not locked so we can skip the search
if (owner == NULL) return NULL;

DO_JAVA_THREADS(t_list, p) {
for (JavaThread* p : *t_list) {
// first, see if owner is the address of a Java thread
if (owner == (address)p) return p;
}
@@ -3684,7 +3658,7 @@ JavaThread *Threads::owning_thread_from_monitor_owner(ThreadsList * t_list,
// Lock Word in the owning Java thread's stack.
//
JavaThread* the_owner = NULL;
DO_JAVA_THREADS(t_list, q) {
for (JavaThread* q : *t_list) {
if (q->is_lock_owned(owner)) {
the_owner = q;
break;
@@ -35,6 +35,7 @@
#include "services/threadIdTable.hpp"
#include "services/threadService.hpp"
#include "utilities/copy.hpp"
#include "utilities/debug.hpp"
#include "utilities/globalDefinitions.hpp"
#include "utilities/ostream.hpp"
#include "utilities/powerOfTwo.hpp"
@@ -629,6 +630,29 @@ static JavaThread* const* make_threads_list_data(int entries) {
return data;
}

#ifdef ASSERT

ThreadsList::Iterator::Iterator() : _thread_ptr(nullptr), _list(nullptr) {}

uint ThreadsList::Iterator::check_index(ThreadsList* list, uint i) {
assert(i <= list->length(), "invalid index %u", i);
Copy link
Member

@dholmes-ora dholmes-ora Jul 30, 2021

Choose a reason for hiding this comment

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

Shouldn't that just be a '<' check?

Choose a reason for hiding this comment

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

No, less-eq is correct. The index being checked here can be 1-past-the-last-element, for an end-iterator. Including 0 for an empty sequence.

Copy link
Member

@dholmes-ora dholmes-ora Jul 30, 2021

Choose a reason for hiding this comment

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

Okay - thanks for explaining.

return i;
}

void ThreadsList::Iterator::assert_not_singular() const {
assert(_list != nullptr, "singular iterator");
}

void ThreadsList::Iterator::assert_dereferenceable() const {
assert(_thread_ptr < (_list->threads() + _list->length()), "not dereferenceable");
}

void ThreadsList::Iterator::assert_same_list(Iterator i) const {
assert(_list == i._list, "iterators from different lists");
}

#endif // ASSERT

ThreadsList::ThreadsList(int entries) :
_magic(THREADS_LIST_MAGIC),
_length(entries),
@@ -29,6 +29,7 @@
#include "runtime/mutexLocker.hpp"
#include "runtime/thread.hpp"
#include "runtime/timer.hpp"
#include "utilities/debug.hpp"

class JavaThread;
class Monitor;
@@ -192,6 +193,10 @@ class ThreadsList : public CHeapObj<mtThread> {
explicit ThreadsList(int entries);
~ThreadsList();

class Iterator;
inline Iterator begin();
inline Iterator end();

template <class T>
void threads_do(T *cl) const;

@@ -211,6 +216,29 @@ class ThreadsList : public CHeapObj<mtThread> {
#endif
};

class ThreadsList::Iterator {
JavaThread* const* _thread_ptr;
DEBUG_ONLY(ThreadsList* _list;)

static uint check_index(ThreadsList* list, uint i) NOT_DEBUG({ return i; });
void assert_not_singular() const NOT_DEBUG_RETURN;
void assert_dereferenceable() const NOT_DEBUG_RETURN;
void assert_same_list(Iterator i) const NOT_DEBUG_RETURN;

public:
Iterator() NOT_DEBUG(= default); // Singular iterator.
inline Iterator(ThreadsList* list, uint i);

inline bool operator==(Iterator other) const;
inline bool operator!=(Iterator other) const;

inline JavaThread* operator*() const;
inline JavaThread* operator->() const;

inline Iterator& operator++();
inline Iterator operator++(int);
};

// An abstract safe ptr to a ThreadsList comprising either a stable hazard ptr
// for leaves, or a retained reference count for nested uses. The user of this
// API does not need to know which mechanism is providing the safety.
@@ -33,6 +33,59 @@
#include "memory/iterator.hpp"
#include "runtime/prefetch.inline.hpp"
#include "runtime/thread.inline.hpp"
#include "utilities/debug.hpp"
#include "utilities/macros.hpp"

ThreadsList::Iterator::Iterator(ThreadsList* list, uint i) :
_thread_ptr(list->threads() + check_index(list, i))
DEBUG_ONLY(COMMA _list(list))
{}

bool ThreadsList::Iterator::operator==(Iterator i) const {
assert_not_singular();
assert_same_list(i);
return _thread_ptr == i._thread_ptr;
}

bool ThreadsList::Iterator::operator!=(Iterator i) const {
assert_not_singular();
assert_same_list(i);
return _thread_ptr != i._thread_ptr;

Choose a reason for hiding this comment

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

Better might be to have the entire body consist of return operator==(i);.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Jul 30, 2021

Choose a reason for hiding this comment

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

I think what's there is more clear. If we switch to return operator==(i),
then I think that will lead to some head scratching. Especially since it's
making me wonder right now...

Choose a reason for hiding this comment

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

And obviously I meant return !operator==(i). Need to keep that all-important one-character difference...

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Jul 30, 2021

Choose a reason for hiding this comment

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

Thanks for the clarification. Now you see why I was scratching my head and
wondering what C++ voodoo you were doing now... :-)

}

JavaThread* ThreadsList::Iterator::operator*() const {
assert_not_singular();
assert_dereferenceable();
Prefetch::read(const_cast<JavaThread**>(_thread_ptr), PrefetchScanIntervalInBytes);

Choose a reason for hiding this comment

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

This prefetch is included because the old code had it. But I'm dubious about it. This is just linear iteration through an array, which seems like the canonical best case for leaving it to the hardware, rather than doing cachelinesize/ptrsize software prefetches per cache line. I'm hoping the original authors can comment.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Jul 30, 2021

Choose a reason for hiding this comment

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

The original author that added the prefetch is @fisk. He's reviewed an earlier
version of this fix and I'm hoping he's around to review this version.

Copy link
Contributor

@fisk fisk Jul 31, 2021

Choose a reason for hiding this comment

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

I agree that it should be okay without prefetch. I added it when I was running on a 4096 strand SPARC machine running over 8000 threads, and walking the list would approach 1 ms. While intuitively linear scan should be good enough, I did actually measure slight improvements with explicit prefetching, and the slight improvements did seem worth it at the time as the amount of time spent in there was non-trivial.
Now we don't support that kind of hardware any longer, so I don't think this trick would necessarily spark as much joy any longer.
I don't mind if we do or do not perform the prefetching. I'm okay either way.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Jul 31, 2021

Choose a reason for hiding this comment

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

I'm going to leave resolution of the prefetch question to a follow-up bug.
I think that discussion deserves a separate issue.

Choose a reason for hiding this comment

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

OK.

return *_thread_ptr;
}

JavaThread* ThreadsList::Iterator::operator->() const {
return operator*();
}

ThreadsList::Iterator& ThreadsList::Iterator::operator++() {
assert_not_singular();
assert_dereferenceable();
++_thread_ptr;
return *this;
}

ThreadsList::Iterator ThreadsList::Iterator::operator++(int) {
assert_not_singular();
assert_dereferenceable();
Iterator result = *this;
++_thread_ptr;
return result;
}

ThreadsList::Iterator ThreadsList::begin() {
return Iterator(this, 0);
}

ThreadsList::Iterator ThreadsList::end() {
return Iterator(this, length());
}

// Devirtualize known thread closure types.
template <class T>