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
+28
−19
Closed
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
3f653c5
8193559.cr0
dcubed-ojdk 5ee4991
kbarrett CR - delete unused _list member.
dcubed-ojdk a133ca4
Merge branch 'master' into JDK-8193559
dcubed-ojdk 3be5dc7
8193559.kbarrett.part1 - Kim's proposed rewrite using newer C++ featu…
dcubed-ojdk 8c255c4
8271513: support JavaThreadIteratorWithHandle replacement by new Thre…
dcubed-ojdk 6254eca
8271514: support JFR use of new ThreadsList::Iterator
dcubed-ojdk a8865b8
kbarrett CR - simplify 'ThreadsList::Iterator::operator!=(Iterator i)'
dcubed-ojdk b3edb5c
Merge branch 'pull/4671' into JDK-8271513
dcubed-ojdk 056badc
Merge branch 'pull/4948' into JDK-8271514
dcubed-ojdk 733faa7
Merge branch 'master' into JDK-8271513
dcubed-ojdk 3c64ea2
Merge branch 'pull/4948' into JDK-8271514
dcubed-ojdk 8873ddd
Merge branch 'master' into JDK-8271514
dcubed-ojdk 76d44dc
Merge branch 'master' into JDK-8271514
dcubed-ojdk a2e39c3
Merge branch 'master' into JDK-8271514
dcubed-ojdk File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
There was a problem hiding this comment.
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. ??
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.