[9] RFR(L): 8046809: vm/mlvm/meth/stress/compiler/deoptimize CodeCache is full.

[Tobias Hartmann]

Hi Albert,

I think having a separate sweeper thread is a good approach.

Some (minor) comments:
- In 'CodeCache::allocate' you should move the following code down to just 
before we return NULL because it should only be executed if the code cache is 
full and not in case of the 'fallback':
  361       MutexUnlockerEx mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
  362       CompileBroker::handle_full_code_cache(code_blob_type);
- I think you should add a comment to the call site of 
'NMethodSweeper::notify()' saying what it does.
- The indentation of lines in 'SmallCodeCacheStartup.java' (line 37) is wrong.
- I think you can remove the "force_sweep" parameter from 
"NMethodSweeper::do_stack_scanning" and only invoke it if you want to enforce 
sweeping. Maybe add a comment to the call site describing why we only check the 
MethodNonProfiled code heap.
- It would be good to have a performance comparison to a baseline version 
without your changes because a different amount of sweeping may affect overall 
performance.
- Don't forget the CCC request because you removed product flags :)

Removing critical allocations together with CodeCacheMinimumFreeSpace is also 
good because it was broken and inconsistent (e.g., method handle intrinsics are 
not critical).

Best,
Tobias


On 09.10.2014 10:24, Albert Noll wrote:
> Hi,
>
> could I get reviews for this patch?
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8046809
>
> Problem:
> The test generates an artificially large number of adapters.
> CodeCacheMinimumFreeSpace in not large enough to hold all adapters,
> if the code cache gets full. Furthermore, the JVM is in a state where no
> safepoint is requested. As a result, stack scanning of active
> methods does not happen and consequently nmethods cannot be flushed from the
> code cache.
>
> Solution:
> The following two changes fix the problem:
>
> 1) Introduce a new VM operation that forces stack scanning of active methods
>
> In the current design, the code cache sweeper can only make progress (i.e.,
> remove compiled code) if
> safepoints are triggered. More specifically, several safepoints must occur to
> concert compiled code from
> state 'not_entrant' to 'unloaded'. If no safepoints are triggered, code cannot
> be removed from the code
> cache. If the code cache fills up and safepoints are triggered too infrequently,
> the sweeper cannot remove
> compiled code from the code cache fast enough.
>
> This patch forces a VM operation to do stack scanning, if there is 10% free
> space in the code cache. Is parameter
> is currently constant. I command line parameter can be added to provide the user
> with explicit control over this
> threshold.  I think we can add such a command line parameter on demand.
>
> 2) Use a dedicated sweeper thread that processes the whole code cache at once
> (remove NmethodSweepFraction)
> Using a separate sweeper thread comes at the cost requiring more memory (mostly
> the stack size of the sweeper
> thread) but has numerous benefits:
> a) Code gets simpler: We can remove NMethodSweepFraction and
> NmethodSweepCheckInterval
> b) More aggressive sweeping: If the code cache gets full, we can process the
> entire code
> cache without taking away cycles that are potentially needed for compilation.
>
> The patch also removes CodeCacheMinimumFreeSpace and 'critical' code cache
> allocations. Due to a bug in
> heap.cpp, the CodeCacheMinimumFreeSpace was in fact not reserved for 'critical'
> allocations. The following
> lines produce an underflow if heap_unallocated_capacity() <
> CodeCacheMinimumFreeSpace:
>
> segments_to_size(number_of_segments) > (heap_unallocated_capacity() -
> CodeCacheMinimumFreeSpace)
>
> Since the critical part in the code cache was never used for its intended
> purpose and we did not have problems
> due to that, we can remove it.
>
> Correctness testing:
> Failing test case, JPRT
>
> Performance testing:
> The bug contains a link to performance results.
>
> Webrev:
> http://cr.openjdk.java.net/~anoll/8046809/webrev.02/
>
> Many thanks in advance,
> Albert