On Tue, 10 Nov 2020 23:24:24 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:
>
> resolve more robehn and coleenp comments.
One change requested in relation to use of jint instead of size_t.
One code simplification suggestion.
Thanks,
David
src/hotspot/share/runtime/synchronizer.cpp line 153:
> 151: if (self->is_Java_thread()) {
> 152: // A JavaThread must check for a safepoint/handshake and honor it.
> 153: ObjectSynchronizer::chk_for_block_req(self->as_Java_thread(),
> "unlinking",
I won't disapprove but this is a case where refactoring is IMO worse than code
duplication. Logging parameters should not be a part of this API IMO.
src/hotspot/share/runtime/synchronizer.cpp line 1228:
> 1226: os::naked_short_sleep(999); // sleep for almost 1 second
> 1227: } else {
> 1228: os::naked_short_sleep(999); // sleep for almost 1 second
So this block can now just be:
> if (self->is_Java_thread()) {
> ThreadBlockInVM tbivm(self->as_Java_thread());
> }
> os::naked_short_sleep(999); // sleep for almost 1 second
src/hotspot/share/runtime/synchronizer.cpp line 246:
> 244: //
> 245: // Start the ceiling with the estimate for one thread:
> 246: jint _in_use_list_ceiling = AvgMonitorsPerThreadEstimate;
Why is this a jint when you use size_t for its accessor and all the other sizes
that you compare with the ceiling are also size_t?
I'm not sure size_t is right to use in these cases (do we really expect
different maximums on 32-bit versus 64-bit?) but it should be all or none IMO.
-------------
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/642
