RFR(S): 8027593: performance drop with constrained codecache starting with hs25 b111

[Albert Noll]

I forgot to thank Vladimir Kozlov and Chris Plummer for their help in 
bringing up and analyzing the problem.

Best,
Albert


On 11/05/2013 03:44 PM, Albert Noll wrote:
> Hi,
>
> could I get reviews for this small patch?
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8027593
> webrev: http://cr.openjdk.java.net/~anoll/8027593/webrev.00/
>
> Problem: The implementation of the sweeper (8020151) causes a 
> performance regression for small code cache sizes. There are two 
> issues that cause this regression:
>   1) NmethodSweepFraction is only adjusted according to the 
> ReservedCodecacheSize if TieredCompilation is enabled. As a result, 
> NmethodSweepFraction remains 16 (if TieredCompilation is not used). 
> This is way too large for small code cache sizes (e.g., <5m).
>  2) _request_mark_phase (sweeper.cpp) is initialized to false. As a 
> result, mark_active_nmethods() did not set _invocations and _current, 
> which results in not invoking the sweeper (calling sweep_code_cache()) 
> at all. When TieredCompilation is enabled this was not an issue, since 
> NmethodSweeper::notify() (which sets _request_mark_phase) is called 
> much more frequently.
>
> Solution: 1) Move setting of NmethodSweepFraction so that it is always 
> executed.
> Solution: 2) Remove need_marking_phase(), request_nmethod_marking(), 
> and reset_nmetod_marking().
>                    I think that these checks are not needed since 
> 8020151, since we do stack scanning of
>                    active nmethods irrespective of the value of what 
> need_marking_phase() returns. Since
>                    the patch removes need_marking_phase() printing out 
> the warning (line 327 in
>                    sweeper.cpp) is incorrect, i.e., we continue to 
> invoke the sweeper. I removed the warning
>                    and the associated code.
>
>
> Also, I think that we can either remove -XX:MethodFlushing or 
> -XX:UseCodeCacheFlushing. Since 8020151, one of them is redundant and 
> can be removed. I am not quite sure if we should do that now so it is 
> not included in the patch.
>
> Testing
> bug: https://bugs.openjdk.java.net/browse/JDK-8027593 also shows a 
> performance evaluation.
>
> Many thanks for looking at the patch.
> Best,
> Albert