On Tue, 10 Nov 2020 20:34:12 GMT, Robbin Ehn <r...@openjdk.org> wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> coleenp CR - refactor common ThreadBlockInVM code block into >> ObjectSynchronizer::chk_for_block_req(). > > Hi, thanks for fixing. > > I had some comments nothing major so approving. @robehn - Thanks for the review!! And thanks for approving. > src/hotspot/share/runtime/synchronizer.cpp line 1419: > >> 1417: ", count=" SIZE_FORMAT ", max=" SIZE_FORMAT, op_name, >> 1418: in_use_list_ceiling(), _in_use_list.count(), >> _in_use_list.max()); >> 1419: timer_p->start(); > > ThreadBlockInVM have a miss-feature: it safepoint polls on front-edge and > back-edge. > It should only have that poll on backedge, once that is fixed, this will do > the wrong thing. > Also you may safepoint on both front-edge and back-edge now, so the timer > would show the wrong thing then. > > So to get the timer correct you should use: > SafepointMechanism::process_if_requested(thread); The baseline code (ObjectSynchronizer::deflate_common_idle_monitors()) uses ThreadBlockInVM currently. I don't want to change that as part of this work. If we want to generally change uses of ThreadBlockInVM to something else, then we should do that with a dedicated bug. > src/hotspot/share/runtime/synchronizer.cpp line 1532: > >> 1530: // A JavaThread must check for a safepoint/handshake and honor >> it. >> 1531: chk_for_block_req(self->as_Java_thread(), "deletion", >> "deleted_count", >> 1532: deleted_count, ls, &timer); > > If you release oopStorage when deflating you can do this entire loop while > blocked instead, which will be faster. > > (From what I remember you can actually release during a safepoint, but that > is not future-prof as I understood it then. So I skipped going into that > rabbit hole this time also.) @fisk said we should not release the oopStorage during a safepoint because that's not safe or will not be safe. I can't remember which. > src/hotspot/share/runtime/objectMonitor.hpp line 148: > >> 146: DEFINE_PAD_MINUS_SIZE(0, OM_CACHE_LINE_SIZE, sizeof(volatile >> markWord) + >> 147: sizeof(WeakHandle)); >> 148: // Used by async deflation as a marker in the _owner field: > > I have test with and without padding, I saw no difference. We've removed enough padding with this work already. If we want to do more padding removal, then we need to use a different RFE. ------------- PR: https://git.openjdk.java.net/jdk/pull/642
