RFR(M): 8244660: Code cache sweeper heuristics is broken

[Man Cao]

Hi Nils,

Thanks for the updated code!
Below is my feedback after a closer look, ordered by importance.

1. sweeper_loop()
NMethodSweeper::sweeper_loop() seems to be missing an inner loop to check
for a wakeup condition.
It could suffer from lost wakeup and spurious wakeup problems, as described
in [1].
Similar code like ServiceThread::service_thread_entry() also has nested
loops to check for wakeup conditions.
We could add a boolean variable "should_sweep", guarded by the
CodeSweeper_lock.
[1]
https://www.modernescpp.com/index.php/c-core-guidelines-be-aware-of-the-traps-of-condition-variables

One wild idea is to let the ServiceThread handle the code cache sweep work,
and remove the sweeper thread.
Has anyone considered this idea before?

2. Rank of CodeSweeper_lock
In mutexLocker.cpp:
 def(CodeSweeper_lock             , PaddedMonitor, special+1,   true,
 _safepoint_check_never);

Should the rank be "special - 1", like the CompiledMethod_lock?
We want to check that this lock acquisition order is valid, but not vice
versa:
{ MonitorLocker(CodeCache_lock); MonitorLocker(CodeSweeper_lock); }
Reading the code in Mutex::set_owner_implementation(), the deadlock
avoidance rule enforces that
the inner lock should have a lower rank than the outer lock.
"special + 1" has the same value as Mutex::suspend_resume, which disables
the deadlock avoidance check.

3. Data race on _bytes_changed
  74    static volatile int _bytes_changed;
The "volatile" keyword likely intends to avoid data races and atomicity
issues,
but the accesses use "_bytes_changed +=" and "=" to do loads and stores.
Should those accesses use Atomic::add(&_bytes_changed, value) and other
Atomic functions.

4. NMethodSweeper::_sweep_threshold
Is it better to make it a "size_t" instead of "int"? Then we can use
"ulong" in metadata.xml, and SIZE_FORMAT in the log_info()).
Also it's probably better to name it as _sweep_threshold_bytes or
_sweep_threshold_bytes, to differentiate from the
SweeperThreshold percentage value.
_sweep_threshold's type should probably be consistent with _bytes_changed,
so perhaps _bytes_changed
could be changed to size_t.

5.
887 void CompileBroker::init_compiler_sweeper_threads() {
888   NMethodSweeper::set_sweep_threshold((SweeperThreshold / 100) *
ReservedCodeCacheSize);
Is it better to use "static_cast<size_t>()" to explicitly mark type cast?

-Man