RFR: 8290025: Remove the Sweeper [v6]

[Erik Österlund]

On Sat, 6 Aug 2022 00:17:39 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Erik Österlund has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits:
>> 
>>  - Merge branch 'master' into 8290025_remove_sweeper
>>  - Removed more stale things
>>  - Remove incorrect comment
>>  - Feedback v2
>>  - Feedback v1
>>  - 8290025: Remove the Sweeper
>
> src/hotspot/share/code/codeCache.cpp line 984:
> 
>> 982:     nm->flush();
>> 983:     if (next == nm) {
>> 984:       // Self looped means end of list
> 
> Why not nullptr for the end of the list?

Because inline caches. When the end is self looped instead of null, I can check if an nmethod is on the list or not, which is used to ensure that unlinking of an nmethod during class unloading happens only once. Why would it ever happen twice? Because the task that unlinks nmethod might have to restart due to inline cache cleaning failing due to running out of ICStubs. And when the code cache is walked the second time, I don't want to call unlink again if we already did that the first traversal. That's why the end of list is self looped instead of null.

> src/hotspot/share/code/codeCache.cpp line 1312:
> 
>> 1310:       CompiledMethod* cm = old_compiled_method_table->at(i);
>> 1311:       // Only walk !is_unloading nmethods, the other ones will get removed by the GC.
>> 1312:       if (!cm->is_unloading()) {
> 
> I thought we were going to walk the unloading methods?  Or should that be another bug fix?  It's unlikely to ever find a reproducer so I wouldn't mind if it was fixed with this giant patch.

You are correct. However, I think the bug fix should be in a separate bug fix, and not in this RFE.

> src/hotspot/share/code/nmethod.cpp line 1316:
> 
>> 1314: 
>> 1315:     // Change state
>> 1316:     bool success = try_transition(not_entrant);
> 
> I suppose in a further RFE, you could rename try_transition or do the transition inline.  Does it still CAS?

It no longer does CAS because we always told the CompiledMethod_lock now when fiddling with this transition. The transition from not_installed to in_use can still fail, if the nmethod became not_entrant before it became in_use. I think the not_installed state can be removed altogether, but I think that's a separate RFE.

> src/hotspot/share/code/nmethod.hpp line 268:
> 
>> 266: 
>> 267:   // Protected by CompiledMethod_lock
>> 268:   volatile signed char _state;         // {not_installed, in_use, not_entrant}
> 
> There's a fourth state: not_used which you might as well add to the comment ?

Fixed!

> src/hotspot/share/oops/constantPool.cpp line 2222:
> 
>> 2220:   }
>> 2221: 
>> 2222:   // If the condition below is true, it means that the nmethod was found to
> 
> I think this comment should say Method and not nmethod.  I think this was a cut and paste error.  I assume that the is_maybe_on_stack is generalizing "stack" meaning on the execution OR continuation stack ?

I think it really does reference nmethod::is_maybe_on_stack(), and explains that the logic for ConstantPool::is_maybe_on_stack works "similar" to how it works for nmethods.

-------------

PR: https://git.openjdk.org/jdk/pull/9741