Re: RFR: 8247281: migrate ObjectMonitor::_object to OopStorage [v2]
On Fri, 11 Sep 2020 21:42:08 GMT, Daniel D. Daugherty <dcubed@openjdk.org> wrote:
This RFE is to migrate the following field to OopStorage:
class ObjectMonitor { <snip> void* volatile _object; // backward object pointer - strong root
Unlike the previous patches in this series, there are a lot of collateral changes so this is not a trivial review. Sorry for the tedious parts of the review. Since Erik and I are both contributors to this patch, we would like at least 1 GC team reviewer and 1 Runtime team reviewer.
This changeset was tested with Mach5 Tier[1-3],4,5,6,7,8 testing along with JDK-8252980 and JDK-8252981. I also ran it through my inflation stress kit for 48 hours on my Linux-X64 machine.
Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
coleenp CR - changes to resolve Coleen's comments.
I approve of this without seeing any new changes. src/hotspot/share/runtime/thread.cpp line 4593:
4591: // are used in om_flush(). 4592: BarrierSet::barrier_set()->on_thread_detach(p); 4593:
One last question, that doesn't require a comment why it's here, but why was this moved? ------------- Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/135
On Fri, 11 Sep 2020 21:33:56 GMT, Coleen Phillimore <coleenp@openjdk.org> wrote:
Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
coleenp CR - changes to resolve Coleen's comments.
I approve of this without seeing any new changes.
I have to also comment that reviewing code in github is really nice! ------------- PR: https://git.openjdk.java.net/jdk/pull/135
On Fri, 11 Sep 2020 21:35:40 GMT, Coleen Phillimore <coleenp@openjdk.org> wrote:
I approve of this without seeing any new changes.
I have to also comment that reviewing code in github is really nice!
Agreed. When I did my self-review for this fix, I didn't use the webrev link at all. I did generate a local webrev via "git webrev" and sanity checked that webrev to verify that "git webrev" works (and it did). ------------- PR: https://git.openjdk.java.net/jdk/pull/135
On Fri, 11 Sep 2020 21:33:33 GMT, Coleen Phillimore <coleenp@openjdk.org> wrote:
Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
coleenp CR - changes to resolve Coleen's comments.
src/hotspot/share/runtime/thread.cpp line 4593:
4591: // are used in om_flush(). 4592: BarrierSet::barrier_set()->on_thread_detach(p); 4593:
One last question, that doesn't require a comment why it's here, but why was this moved?
We had to move that code to make om_flush() happy. om_flush() accesses the (weak) oops in the monitor list after the thread is off the threads list so the barrier change had to move. It will move back in part3. ------------- PR: https://git.openjdk.java.net/jdk/pull/135
On Fri, 11 Sep 2020 21:35:41 GMT, Daniel D. Daugherty <dcubed@openjdk.org> wrote:
src/hotspot/share/runtime/thread.cpp line 4593:
4591: // are used in om_flush(). 4592: BarrierSet::barrier_set()->on_thread_detach(p); 4593:
One last question, that doesn't require a comment why it's here, but why was this moved?
We had to move that code to make om_flush() happy. om_flush() accesses the (weak) oops in the monitor list after the thread is off the threads list so the barrier change had to move. It will move back in part3.
Just to be clear the thread is still on the threads_list at this point. ------------- PR: https://git.openjdk.java.net/jdk/pull/135
On Mon, 14 Sep 2020 01:43:01 GMT, David Holmes <dholmes@openjdk.org> wrote:
We had to move that code to make om_flush() happy. om_flush() accesses the (weak) oops in the monitor list after the thread is off the threads list so the barrier change had to move. It will move back in part3.
Just to be clear the thread is still on the threads_list at this point.
Sorry, I should have been more clear here. We had to move that code after the om_flush() call because om_flush() accesses the (weak) oops in the monitor list after the original location of the code. The om_flush() call is made in the code that removes the thread from the threads list, but the removal happens after the call to om_flush(). ------------- PR: https://git.openjdk.java.net/jdk/pull/135
participants (3)
-
Coleen Phillimore
-
Daniel D.Daugherty
-
David Holmes