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

[Nils Eliasson]

Hi Man,

On 2020-05-22 10:13, Man Cao wrote:
> 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
Reasonable.

I re-added _should_sweep and did some minor refactorings.

>
> 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?

I think it has but I don't remember the details. Code cache sweeping 
would block the service thread for quite some time.

>
> 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.

Good catch - but the rank should be "special - 2". There is one code 
path through InstanceKlass::add_osr_nmethod that calls make_not_entrant 
on an nmethod while holding the CompileMethod_lock. So the rank need to 
be one less.

I updated enum lock_types to add one more level of special.


>
> 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.

Fixed.
>
> 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.

ok, fixed.
>
> 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?
sure.
>
> -Man

New webrev: http://cr.openjdk.java.net/~neliasso/8244660/webrev.03/

Best regards,
Nils