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

8271514: support JFR use of new ThreadsList::Iterator #4949

Closed
wants to merge 14 commits into from
Closed
Changes from all commits
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
@@ -26,6 +26,7 @@
#include "jfr/support/jfrThreadLocal.hpp"
#include "jfr/utilities/jfrThreadIterator.hpp"
#include "runtime/thread.inline.hpp"
#include "runtime/threadSMR.inline.hpp"

static bool thread_inclusion_predicate(Thread* t) {
assert(t != NULL, "invariant");
@@ -40,14 +41,6 @@ static bool java_thread_inclusion_predicate(JavaThread* jt, bool live_only) {
return thread_inclusion_predicate(jt);
}

static JavaThread* next_java_thread(JavaThreadIteratorWithHandle& iter, bool live_only) {
JavaThread* next = iter.next();
while (next != NULL && !java_thread_inclusion_predicate(next, live_only)) {
next = iter.next();
}
return next;
}

static NonJavaThread* next_non_java_thread(NonJavaThread::Iterator& iter) {
while (!iter.end()) {
NonJavaThread* next = iter.current();
@@ -60,15 +53,29 @@ static NonJavaThread* next_non_java_thread(NonJavaThread::Iterator& iter) {
return NULL;
}

JfrJavaThreadIteratorAdapter::JfrJavaThreadIteratorAdapter(bool live_only /* true */) : _iter(),
_next(next_java_thread(_iter, live_only)),
_live_only(live_only) {}
JfrJavaThreadIteratorAdapter::JfrJavaThreadIteratorAdapter(bool live_only /* true */) :
_tlist(),
_it(_tlist.begin()),
_end(_tlist.end()),
_live_only(live_only)
{
skip_excluded();
}

bool JfrJavaThreadIteratorAdapter::has_next() const {
return _it != _end;
}

void JfrJavaThreadIteratorAdapter::skip_excluded() {
while (has_next() && !java_thread_inclusion_predicate(*_it, _live_only)) {
++_it;
}
}

JavaThread* JfrJavaThreadIteratorAdapter::next() {
assert(has_next(), "invariant");
Type* const temp = _next;
_next = next_java_thread(_iter, _live_only);
assert(temp != _next, "invariant");
Type* const temp = *_it++;
skip_excluded();
return temp;
}

@@ -47,15 +47,17 @@ class JfrThreadIterator : public AP {

class JfrJavaThreadIteratorAdapter {
private:
JavaThreadIteratorWithHandle _iter;
JavaThread* _next;
ThreadsListHandle _tlist;
Copy link
Member

@dholmes-ora dholmes-ora Aug 2, 2021

Choose a reason for hiding this comment

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

Why do we need to store this?

It looks very suspiocious to have a member that is a stackObj, in a class that is not itself a stackObj. ??

Copy link
Contributor

@sspitsyn sspitsyn Aug 4, 2021

Choose a reason for hiding this comment

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

The _tlist is used locally in JfrJavaThreadIteratorAdapter constructor only, so it is possible to get rid of it for the price of complicating the constructor a little bit.

Choose a reason for hiding this comment

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

The _tlist is a handle that ensures the captured ThreadsList remains live while the iterator (or a copy) exists. Dropping it would leave _it and _end potentially dangling.

Copy link
Member

@dholmes-ora dholmes-ora Aug 5, 2021

Choose a reason for hiding this comment

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

Okay I had assumed there was a ThreadsList/Handle in the enclosing scope that was being used to initialize this and keep things live, but that is not the case.
I still think it makes no sense to have a StackObj subtype as a member though. Maybe ThreadsListHandle should no longer be a StackObj ?

Choose a reason for hiding this comment

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

This isn't a change wrto StackObj usage. This adapter class used to have a member that was a JavaThreadIteratorWithHandle (the WithHandle class for the same reason the new code has a ThreadsListHandle, to keep the associated ThreadsList alive), which also derives from StackObj.

Personally, I want to agree with you. I would (and do) use StackObj far more sparingly than it appears in our code base. But I've had this same argument with other folks. The de facto situation, so far as I understand it, is that deriving from StackObj documents normal usage. It has no operational behavior or additional semantics, other than attempting to prevent heap allocation (and it doesn't really succeed there, since a derived class could override that behavior, though I don't currently know of any that do so). That seems to be what some people want from it and think is useful, and I've stopped trying to convince them otherwise.

Copy link

@mgronlun mgronlun Aug 5, 2021

Choose a reason for hiding this comment

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

The JfrThreadIterator is only used as a stack-allocated object, so it and the adapters can be decorated with StackObj to better communicate this intent. The AP (Allocation Policy) policy in JfrThreadIterator can be removed and the JfrThreadIterator can be decorated with StackObj (unconditionally). Depends on how much you want to spend on this (I would guess minimal). But in actual usages, the iterators and adapters are de-facto StackObj already.

Copy link
Member

@dholmes-ora dholmes-ora Aug 5, 2021

Choose a reason for hiding this comment

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

Ah I didn't realise JTIWH was also stackObj.

The thing with StackObj to me is that it indicates a local object that does something interesting on construction and then also on destruction (eg. like MutexLocker). So having one as a member where the destruction is not related to a stack scope seems odd to me. But a stackObj member in a stackObj class seems more reasonable.

Copy link
Contributor

@coleenp coleenp Aug 6, 2021

Choose a reason for hiding this comment

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

StackObj to me communicates intent. I don't have to look for "new SomeClass" to see how it is allocated and deallocated, since the new and delete operator are private and won't compile. Same is true for AllStatic. I'm one that hasn't been convinced otherwise. I'm not totally intransigent though as i approved the removal of ValueObj. So there's that.

ThreadsListHandle::Iterator _it;
ThreadsListHandle::Iterator _end;
bool _live_only;

void skip_excluded();

public:
typedef JavaThread Type;
JfrJavaThreadIteratorAdapter(bool live_only = true);
bool has_next() const {
return _next != NULL;
}
bool has_next() const;
Type* next();
};