[9] RFR(S): 8136458: Remove "marked for reclamation" nmethod state

[Nils Eliasson]

On 2016-03-18 14:34, Tobias Hartmann wrote:
> Hi Nils,
>
> thanks for looking at this!
>
> On 18.03.2016 13:49, Nils Eliasson wrote:
>> Was it the move to a dedicated sweeper thread that make this possible? When the compiler threads swept the code cache, it was done in parts, and then we had to keep the nmethods as marked-for-reclaation until a full sweep was completed. Am I right? Then this makes perfect sense.
> No, the dedicated sweeper thread did not change this behavior. Before 8046809 [1], sweeping was done by the compiler threads in several steps to reduce pressure and establish a good balance between sweeping and compiling. Now having a dedicated thread (that may be interrupted at any time), does not affect the nmethod state cycle but allows the thread scheduler to find the best balance between compilation and sweeping. We still have to wait for a full sweep to finish before a zombie nmethod is flushed. My point is that we do this anyway and don't need the "marked for reclamation" state for this.

The dedicated sweeper is superb - it makes the code so much easier to 
reason about, both the sweeper code and the compilebroker code.

>
> Basically, my assumptions are the following:
> After a nmethod was marked as zombie by the sweeper, it's guaranteed to be not on the stack and no inline caches will be updated to point to this nmethod. However, outdated ICs may still point to it. The sweeper will continue to visit nmethods and only encounter the zombie nmethod again, after a full sweep cycle is finished. This is guaranteed because nmethods are not moved in the code cache. Therefore, all the inline caches of other nmethods that pointer to the zombie nmethod are cleaned now. We can safely flush the nmethod.
>
> I'm not sure why the "marked for reclamation" state was ever necessary (if at all).

I think nmethods could be made zombies in more ways before. (Currently 
it looks like only the sweeper make them zombies.) In the case when 
another threads can make zombies in the middle of a sweep, you need to 
differentiate between the ones that where zombie when the sweep started, 
and the ones that where made zombie in a part of the code cache that is 
already swept.

Thanks for fixing this!
Nils
>
> Best regards,
> Tobias
>
> [1] http://cr.openjdk.java.net/~anoll/8046809/webrev.06/
>
>> Looks good,
>> Nils
>>
>> On 2016-03-18 10:22, Tobias Hartmann wrote:
>>> Hi,
>>>
>>> please review the following patch.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8136458
>>> http://cr.openjdk.java.net/~thartmann/8136458/webrev.00/
>>>
>>> The sweeper removes zombie nmethods only after they were "marked for reclamation" to ensure that there are no inline caches referencing the zombie nmethod. However, this is not required because if a zombie nmethod is encountered again by the sweeper, all ICs pointing to it were already cleaned in the previous sweeper cycle:
>>>
>>> alive -> not-entrant/unloaded (may be on the stack)
>>> cycle 1: not-entrant/unloaded -> zombie (may be referenced by ICs)
>>> cycle 2: zombie -> marked for reclamation
>>> cycle 3: marked for reclamation -> flush
>>>
>>> In each cycle, we clean all inline caches that point to not-entrant/unloaded/zombie nmethods. Therefore, we know already after sweeper cycle 2, that the zombie nmethod is not referenced by any ICs and we could flush it immediately.
>>>
>>> I removed the "marked for reclamation" state. The following testing revealed no problems:
>>> - JPRT
>>> - RBT with hotspot_all and -Xcomp/-Xmixed
>>> - 100 iterations of Nashorn + Octane with -XX:StartAggressiveSweepingAt=100/50 -XX:NmethodSweepActivity=500/100
>>>
>>> Thanks,
>>> Tobias