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

[Vladimir Kozlov]

On 3/21/16 1:33 AM, Tobias Hartmann wrote:
> Hi Vladimir,
>
> thanks for having a look!
>
> On 18.03.2016 17:21, Vladimir Kozlov wrote:
>> But sweeper should sweep at least twice before cleanin all IC pointing to zombi method because a method could be marked in the middle of sweep. Right?
>
> What do you mean by "could be marked" in the middle of the sweep? That a nmethod could be converted to zombie while sweeping?

Okay, that answered my question (I should have read conversion in this thread). Sweeper did full cycle which means it 
visited *all* nmethods.

Changes are good.

Thanks,
Vladimir

>
> As I explained above, this is not possible anymore because the sweeper is the only component that can set a nmethod to zombie. If we encounter a zombie nmethod in the sweeper, it's guaranteed that we visited this nmethod twice: once when setting it to zombie and again after a full cycle of flushing the ICs of all other nmethods that potentially still reference this nmethod. We can therefore flush it on the second encounter.
>
> Before JDK-8075805, we had CodeCache::make_marked_nmethods_zombies() which could also set a nmethod to zombie (outside of the sweeper). I assume that the "marked for reclamation state" was introduced to make sure that we visited such a nmethod in the sweeper twice.
>
> Or did I miss something?
>
> Thanks,
> Tobias
>
>> Thanks,
>> Vladimir
>>
>> On 3/18/16 8:50 AM, Tobias Hartmann wrote:
>>> Hi Nils,
>>>
>>> On 18.03.2016 16:33, Nils Eliasson wrote:
>>>> 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.
>>>
>>> Yes, I agree. It's important to keep this code simple and fast.
>>>
>>>>> 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.
>>>
>>> Right, we had CodeCache::make_marked_nmethods_zombies() which I removed with JDK-8075805 [1] because it caused other problems. The sweeper is now (and should remain) the only place where nmethods can transition to zombie.
>>>
>>> Thanks again for the review!
>>>
>>> Best regards,
>>> Tobias
>>>
>>> [1] http://cr.openjdk.java.net/~thartmann/8075805/webrev.01/
>>>
>>>> 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
>>>>