Request review: 4360113: Evict nmethods when code cache gets full

[Vladimir Kozlov]

Eric Caspole wrote:
> 
> On Jan 6, 2010, at 3:42 PM, Vladimir Kozlov wrote:
> 
>> Eric,
>>
>> I like these changes. The only question left:
>>
>> Is it possible to avoid VM exit if (CompileTheWorld || ExitOnFullCodeCache)
>> before code cache cleanup attempt? If it requires a lot of changes 
>> leave as it is.
>>
> 
> I have not considered working with these flags yet, I'll look at it and 
> get back to you. If cleaning is on, would you still want 
> ExitOnFullCodeCache to exit? I am not sure what this flag is used for.

Both these flags is for debugging. And I don't care about ExitOnFullCodeCache
since nobody used it for long time. But we always use CompileTheWorld
for our testing. Also with recent Tom's changes for CompileTheWorld
we may not have as much problems as before with filled code cache.
This is why I said it should not be high priority for you only if
it is simple to fix. And if you do the fix it could be applied to both
flags - exit VM only if not space left even after cleanup.

> 
>> In compileBroker.cpp don't mix int with boolean (some compilers may 
>> complain):
>>
>> + volatile jint CompileBroker::_should_compile_new_jobs = 1;
>>
>> +   static bool should_compile_new_jobs() { return UseCompiler && 
>> (_should_compile_new_jobs == 1); }
> 
> (_should_compile_new_jobs == 1) is a boolean, right? I really want 
> _should_compile_new_jobs to be a boolean, but there are no atomic 
> boolean setters.

Actually, I don't understand why it is need to be atomic.
It is always one direction change:
compiler broker change it to "false" and
only sweeper (at safepoint) change it to "true".

Vladimir

> 
>>
>> Thanks,
>> Vladimir
>>
>> Eric Caspole wrote:
>>> Hi Vladimir,
>>> I updated my webrev to .02: I added a new field into CompileBroker to 
>>> use for toggling the compiler on/off when the code cache gets full or 
>>> when enough space is later cleared, to avoid changing UseCompiler on 
>>> the fly. Also I consolidated all the places that had some "turn off 
>>> compiler" code into one place in CompileBroker as we discussed before 
>>> the holidays.
>>> Regards,
>>> Eric
>>> On Dec 16, 2009, at 3:29 PM, Vladimir Kozlov wrote:
>>>>
>>>> Eric Caspole wrote:
>>>>> I see what you mean. If there are other already disconnected 
>>>>> methods (nm->method()->code() != nm) due to normal operation, can I 
>>>>> safely assume they are already marked not entrant and they will go 
>>>>> away by normal sweeping? Or should I hurry them along here? I  see 
>>>>> how not
>>>>
>>>> The assert(nm->method()->code()->is_not_entrant() is enough, I think.
>>>>
>>>>> having the extra check means I might reset the counters many times 
>>>>> for no reason, I'll change that.
>>>>
>>>>>> It seems other thread set UseCompiler to false between the check 
>>>>>> and assert.
>>>>> So that means this could have happened in the existing system if 
>>>>> the broker shut off the compiler due to fullness, racing with the 
>>>>> java thread making this call. Yuck.
>>>>
>>>> Yes. With your changes it could happened more frequently.
>>>> I think, it is related to what we talked about today on meeting:
>>>>
>>>> # Consolidate "shut off compiler" logic into one place, and 
>>>> reconsider logic to turn compilers on and off.  Currently this logic 
>>>> is spread across three different places even without the unloading 
>>>> change.
>>>>
>>>> May be we should use separate flag SuspendCompilation when we think
>>>> codecache could be freed and switch of UseCompiler when we give up.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>>
>>>>>> Vladimir
>>>>>>
>>>>>>>> % java -XX:ReservedCodeCacheSize=6M -XX:+PrintCodeCacheExtension 
>>>>>>>> -XX:+UseCodeCacheFlushing -XX:MinCodeCacheFlushingInterval=300 
>>>>>>>> -XX:+PrintCompilation -XX:+PrintMethodFlushing -jar 
>>>>>>>> SPECjvm2008.jar -ikv compiler.sunflow compiler.compiler
>>>>>>>>
>>>>>>>> #  Internal Error 
>>>>>>>> (/net/irkutsk/export/home/kvn/work2/hg/4360113/src/share/vm/runtime/compilationPolicy.cpp:124), 
>>>>>>>> pid=26531, tid=51
>>>>>>>> #  Error: assert(UseCompiler || CompileTheWorld,"UseCompiler 
>>>>>>>> should be set by now.")
>>>>>>>>
>>>>>>>> Current thread (0x08c84400):  JavaThread "BenchmarkThread 
>>>>>>>> compiler.sunflow 6" [_thread_in_vm, id=51, 
>>>>>>>> stack(0xfa2c9000,0xfa319000)]
>>>>>>>>
>>>>>>>> Stack: [0xfa2c9000,0xfa319000],  sp=0xfa317ad8,  free 
>>>>>>>> space=13afa319000k
>>>>>>>> Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, 
>>>>>>>> C=native code)
>>>>>>>> V  [libjvm.so+0x170235a] void VMError::report(outputStream*) + 
>>>>>>>> 0x5be
>>>>>>>> V  [libjvm.so+0x1703336] void VMError::report_and_die() + 0x586
>>>>>>>> V  [libjvm.so+0x7bf95d] void report_assertion_failure(const 
>>>>>>>> char*,int,const char*) + 0x61
>>>>>>>> V  [libjvm.so+0x6ccd4e] void 
>>>>>>>> SimpleCompPolicy::method_invocation_event(methodHandle,Thread*) 
>>>>>>>> + 0x20a
>>>>>>>> V  [libjvm.so+0xa68949] 
>>>>>>>> nmethod*InterpreterRuntime::frequency_counter_overflow_inner(JavaThread*,unsigned 
>>>>>>>> char*) + 0x1551
>>>>>>>>
>>>>>>>>
>>>>>>>> Eric Caspole wrote:
>>>>>>>>> 4360113: Evict nmethods when code cache gets full
>>>>>>>>> http://cr.openjdk.java.net/~ecaspole/4360113/
>>>>>>>>> In this change, under a flag and off by default, when compilers 
>>>>>>>>> notice the code cache is getting full, they will call a vm op 
>>>>>>>>> that calls new code that attempts to speculatively unload the 
>>>>>>>>> oldest half of the nmethods (based on the compile job id) by 
>>>>>>>>> hiding the methodOop's ref to the nmethod in the new 
>>>>>>>>> _saved_code field. Then execution resumes. After inline cache 
>>>>>>>>> cleaning, callers will have to go back to the methodOop to get 
>>>>>>>>> the verified entry point.
>>>>>>>>> At that point, the methodOop's _code field is restored from the 
>>>>>>>>> _saved_code field and the methodOop/nmethod go back to their 
>>>>>>>>> normal state.
>>>>>>>>> If a method so marked is not called by the second sweep cycle 
>>>>>>>>> after the one where forced unloading happened, the nmethod will 
>>>>>>>>> be marked non-entrant and got rid of by normal sweeping. That 
>>>>>>>>> gives the app a few seconds to make progress and call its 
>>>>>>>>> hottest methods.
>>>>>>>>> We chose to target the oldest half of nmethods due to a 
>>>>>>>>> customer experience with a long-running app server, and despite 
>>>>>>>>> multiple redeployments of the same web app, something was 
>>>>>>>>> preventing old instances of the web app from ever getting 
>>>>>>>>> unloaded. In that case, they ran into the code cache full 
>>>>>>>>> problem so the most recent redeployment was running interpreter 
>>>>>>>>> only. We have also observed that for many applications a lot of 
>>>>>>>>> methods get compiled and used during the startup phase that are 
>>>>>>>>> never used again.
>>>>>>>>> In this change there is also a timer based backoff, default of 
>>>>>>>>> 30 seconds, so that if the normal state of the app is 
>>>>>>>>> constantly triggering unloading, the unloading will stop and it 
>>>>>>>>> will fall back to the existing situation of disabling the 
>>>>>>>>> compiler.
>>>>>>>>> In my testing, this allows the program to quickly resume normal 
>>>>>>>>> operation with no noticeable performance degradation.
>>>>>>>>> Thanks for your comments,
>>>>>>>>> Eric
>>>>>>>>
>>>>>>
>>>>
>>
> 
>