RFR: 8208601: Introduce native oop barriers in C2 for OopHandle

Thu Aug 2 18:03:45 UTC 2018

On 8/2/18 1:03 PM, Vladimir Kozlov wrote:
> Thanks Erik
>
> Wow! I got 18 copies of your response ;)

Does it count as 18 code reviews?

Coleen
>
> On 8/2/18 2:39 AM, Erik Österlund wrote:
>> Hi Vladimir,
>>
>> Thank you for reviewing.
>>
>> Incremental:
>> http://cr.openjdk.java.net/~eosterlund/8208601/webrev.00_01/
>>
>> Full:
>> http://cr.openjdk.java.net/~eosterlund/8208601/webrev.01/
>>
>>> On 01 Aug 2018, at 20:03, Vladimir Kozlov 
>>> <vladimir.kozlov at oracle.com> wrote:
>>>
>>> Hi, Erik
>>>
>>> 1. is_gc_barrier_node(b) is virtual so even if it return 'false' I 
>>> am not sure C++ will collapse new search loops in phaseX.cpp and 
>>> checks in other cases. Can we use some static check to allow C++ 
>>> collapse that code in case there is no load barriers?
>>
>> I see your point. I introduced a has_load_barrier() function in 
>> BarrierSetC2 that I can check outside of the loop, to make the extra 
>> load barrier search conditional. Was it something like this you had 
>> in mind?
>
> Yes, thanks.
>
>>
>>> 2. Can you explain Phi case more. I would like to see an assert 
>>> which verify cush shape of code to avoid cases when Phi come from 
>>> loop, for example.
>>
>> Sure. The load barrier has different shapes before and after macro 
>> expansion. Before macro expansion it is just a LoadBarrierNode. After 
>> expanding the barrier, the pseudo code for the inflated barrier looks 
>> something like this: value = is_bad(some_oop) ? slowpath_load : 
>> some_oop. The value in that pseudo code is the particular phi I am 
>> matching. What is unique about this phi (corresponding to the barrier 
>> region node depicting the good_or_null vs bad control flows), 
>> compared to an arbitrary phi, is that in(1) is the slowpath_load 
>> (LoadBarrierSlowPathRegNode) taken if the oop is bad. A phi in e.g. a 
>> loop would point to another phi with this property inside e.g. the 
>> loop body, never directly to a slowpath_load 
>> (LoadBarrierSlowPathRegNode).
>
> Thank you for explanation. My confusion come that you phi->in(2) is 
> points to a value on one of branches which is inside original macro 
> node.  But looking on your pseud code it is some_oop which is what you 
> want.
>
> One suggesting I would give is to add comments to each of 3 cases in 
> step_over_gc_barrier() to be clear what function is looking for.
>
> Also for first case (Proj node) it would be nice to have assert to 
> check  node you expect this Proj is connected to.
>
>>
>> Note that there is a basic and optimized macro expansion of the load 
>> barrier. The matching I use catches the optimized variant only (which 
>> uses the LoadBarrierSlowPathRegNode for the slow path). The basic 
>> barrier expansion is only used for atomic xchg (because self-healing 
>> the pointer would be an error in that path).
>>
>> In the new version, I added some assertions right where load barriers 
>> are produced and expanded, to assert that it matches and can be 
>> stepped over reliably. That way, if we change the barriers, the 
>> matching code will not silently stop working. To make these asserts 
>> work, I added support for matching barriers on weak barriers as well, 
>> that use LoadBarrierWeakSlowPathRegNode. While that isn’t needed for 
>> my purposes for loading the mirror, it is better to be more complete. 
>> Now you can see at the assertions in 
>> ZBarrierSetC2::expand_loadbarrier_optimized the exact phi that I am 
>> matching.
>>
>> I have still intentionally left matching of the “basic” load barrier 
>> used by atomic xchg for another day, as nobody has any need for that 
>> yet, and it is trickier to match.
>>
>> BTW, it would be nice if in C2 the Node class had a Node* 
>> _expanded_from member that could track the node that macro expanded a 
>> node, and remember that through cloning of nodes, so that things like 
>> this would reduce to simply node->expanded_from()->is_LoadBarrier(), 
>> rather than trying to recognize the shape of the expanded barriers. 
>> Perhaps that would be useful for others? I did try something like 
>> that originally (but tracked with a hashtable), but unfortunately ran 
>> into trouble with cloned nodes not retaining the information, and 
>> made the decision to go for the shape recognition solution instead. 
>> But perhaps it could be generally useful for other shape recognition 
>> problems as well… But that might be outside the scope of this RFE.
>
> Yes, it is different and not hard to do. Nobody asked it before.
>
> Thanks,
> Vladimir
>
>>
>> Thanks,
>> /Erik
>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 8/1/18 4:17 AM, Erik Österlund wrote:
>>>> Hi,
>>>> There is currently no way of doing IN_NATIVE accesses in C2 using 
>>>> its access API. These are required to properly access OopHandle, 
>>>> used to access the Java class mirrors (because they will now start 
>>>> requiring load barriers). In order to support (concurrent) class 
>>>> unloading in ZGC, this support must be added.
>>>> In this patch, I add an access_load API for loading IN_NATIVE oops, 
>>>> and use it to load class mirrors (which is logically an 
>>>> OopHandle::resolve, which happened to have been an indirect load 
>>>> before as nobody had load barriers on it). In the various code 
>>>> recognizing the shape of a mirror load to optimize the code, I have 
>>>> added a check if a node is a GC barrier and to then step over it in 
>>>> order to match the mirror load.
>>>> In order to recognize and step over the load barriers in ZGC 
>>>> properly, I added support for recognizing the barrier shapes not 
>>>> just before macro expansion (LoadBarrierNode), but also after macro 
>>>> expansion (as required by the matching code), which involves 
>>>> checking for phi nodes with with LoadBarrierSlowRegNode phi->in(1), 
>>>> and then stepping over to phi->in(2), as well as recognizing 
>>>> projections to such shapes. LoadBarrierSlowRegNode is used in all 
>>>> barrier expansions except for atomic xchg, but that is fine as we 
>>>> don't use that on class mirrors.
>>>> I have checked that the shapes are recognized and that no 
>>>> regressions are introduced with these changes through a bunch of 
>>>> benchmarks in gc-test-suite.
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8208601/webrev.00/ 
>>>> <https://link.getmailspring.com/link/1533049674.local-48cfadc1-4a64-v1.3.0-fd741eb7@getmailspring.com/0?redirect=http%3A%2F%2Fcr.openjdk.java.net%2F%7Eeosterlund%2F8208582%2Fwebrev.00%2F&recipient=aG90c3BvdC1jb21waWxlci1kZXZAb3Blbmpkay5qYXZhLm5ldA%3D%3D> 
>>>> Bug URL:
>>>> https://bugs.openjdk.java.net/browse/JDK-8208601 
>>>> <https://link.getmailspring.com/link/1533049674.local-48cfadc1-4a64-v1.3.0-fd741eb7@getmailspring.com/1?redirect=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8208582&recipient=aG90c3BvdC1jb21waWxlci1kZXZAb3Blbmpkay5qYXZhLm5ldA%3D%3D> 
>>>> Thanks,
>>>> /Erik
>>>> Open Tracking
>>