On Tue, 10 Nov 2020 02:35:17 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:
>
> dholmes-ora - convert inner while loop to do-while loop in
> unlink_deflated().
Changes requested by coleenp (Reviewer).
src/hotspot/share/oops/markWord.cpp line 63:
> 61: fatal("bad header=" INTPTR_FORMAT, value());
> 62: }
> 63:
This makes it so much clearer where the displaced markWord is.
src/hotspot/share/runtime/globals.hpp line 750:
> 748: product(intx, MonitorUsedDeflationThreshold, 90, EXPERIMENTAL,
> \
> 749: "Percentage of used monitors before triggering deflation (0 is
> " \
> 750: "off). The check is performed on GuaranteedSafepointInterval "
> \
Should there still be experimental options after this change?
src/hotspot/share/runtime/monitorDeflationThread.cpp line 85:
> 83: // visible to external suspension.
> 84:
> 85: ThreadBlockInVM tbivm(jt);
Does this have to be a JavaThread? Could it be a non-java thread since
deflating monitors doesn't have to call any Java code? You'd have to lock down
the Monitor list maybe, but couldn't this be a NamedThread? This isn't a
request to change it right now.
src/hotspot/share/runtime/synchronizer.cpp line 1641:
> 1639:
> 1640: // Do the final audit and print of ObjectMonitor stats; must be done
> 1641: // by the VMThread (at VM exit time).
Can you take (at VM exit time) out of parenthesis? it made me wonder when else
is this called.
src/hotspot/share/runtime/objectMonitor.cpp line 509:
> 507: //
> 508: bool ObjectMonitor::deflate_monitor() {
> 509: if (is_busy()) {
is_busy should be checked != 0 since it doesn't return a bool.
src/hotspot/share/runtime/objectMonitor.cpp line 540:
> 538: if (try_set_owner_from(NULL, DEFLATER_MARKER) != NULL) {
> 539: // The owner field is no longer NULL so we lost the race since the
> 540: // ObjectMonitor is now busy.
So here would contentions be > 0? Can it be asserted? Doesn't need to be, the
comment really helps to understand why the cas failed.
src/hotspot/share/runtime/objectMonitor.cpp line 551:
> 549: if (try_set_owner_from(DEFLATER_MARKER, NULL) != DEFLATER_MARKER) {
> 550: // Deferred decrement for the JT EnterI() that cancelled the
> async deflation.
> 551: add_to_contentions(-1);
contentions is essentially a refcount, isn't it. Can you fix the comment to
include this at line 360 since that's not the only purpose of this count.
// Keep track of contention for JVM/TI and M&M queries.
add_to_contentions(1);
src/hotspot/share/runtime/synchronizer.hpp line 61:
> 59: bool has_next() const { return _current != NULL; }
> 60: ObjectMonitor* next();
> 61: };
Can MonitorList be defined in the .cpp file? I don't see anything outside of
synchronizer.cpp that refers to it.
src/hotspot/share/runtime/synchronizer.hpp line 28:
> 26: #define SHARE_RUNTIME_SYNCHRONIZER_HPP
> 27:
> 28: #include "logging/logStream.hpp"
If you need to put MonitorList in the header file, use a forward declaration
for LogStream instead of #including logstream.hpp.
src/hotspot/share/runtime/objectMonitor.hpp line 171:
> 169: volatile int _SpinDuration;
> 170:
> 171: jint _contentions; // Number of active contentions in
> enter(). It is used by is_busy()
Future RFE - can we replace jint with int32_t or even int or some C++ types.
We're trying not to have Java types leak into runtime code since this doesn't
directly interface with Java.
-------------
PR: https://git.openjdk.java.net/jdk/pull/642
