On Tue, 10 Nov 2020 17:39:25 GMT, Daniel D. Daugherty <dcu...@openjdk.org> wrote:
>> Changes from @fisk and @dcubed-ojdk to: >> >> - simplify ObjectMonitor list management >> - get rid of Type-Stable Memory (TSM) >> >> This change has been tested with Mach5 Tier[1-3],4,5,6,7,8; no new >> regressions. >> Aurora Perf runs have also been done (DaCapo-h2, Quick Startup/Footprint, >> SPECjbb2015-Tuned-G1, SPECjbb2015-Tuned-ParGC, Volano) >> - a few minor regressions (<= -0.24%) >> - Volano is 6.8% better >> >> Eric C. has also done promotion perf runs on these bits and says "the >> results look fine". > > 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 so approving. src/hotspot/share/runtime/monitorDeflationThread.cpp line 92: > 90: // We wait for GuaranteedSafepointInterval so that > 91: // is_async_deflation_needed() is checked at the same interval. > 92: ml.wait(GuaranteedSafepointInterval); I don't like that we add a new global monitor just for the whitebox API. Without WB poking this could just a plain sleep. If we must have this new global monitor there seems be no reason for this to be no safepoint ? So ThreadBlockInVM would not be needed if we did safepoint checks instead? I would either skip WB notification and run the test with a very low GuaranteedSafepointInterval and just set flag and wait a second. Or if this is really important use a local semaphore and skip the boolean, each increase on sema would result in a deflation pass. 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); 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. 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. ------------- Marked as reviewed by rehn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/642
