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

Fri Aug 3 18:10:47 UTC 2018

Hi Vladimir,

Thanks for the review.

/Erik

> On 3 Aug 2018, at 19:58, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> This version looks good to me. I think you need second review.
> 
> Thanks,
> Vladimir
> 
>> On 8/3/18 9:01 AM, Erik Österlund wrote:
>> Hi Vladimir,
>> Thanks for reviewing.
>> Full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8208601/webrev.02/ <http://cr.openjdk.java.net/~eosterlund/8208601/webrev.02/>
>> Incremental:
>> http://cr.openjdk.java.net/~eosterlund/8208601/webrev.01_02/
>>> On 02 Aug 2018, at 19:03, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>> 
>>> Thanks Erik
>>> 
>>> Wow! I got 18 copies of your response ;)
>> Oops, sorry about that. My mail client said it didn’t send - so I pressed send a few times more. Looks like they were all sent in the end. >_<
>>>> 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.
>> Fixed.
>>> Also for first case (Proj node) it would be nice to have assert to check  node you expect this Proj is connected to.
>> I added an assert checking that we only find oop projs of the load barrier (before getting macro expanded). I guess finding an expanded load barrier through a proj should be fine as well. But I haven’t found any in my testing, so I thought having this stronger assert is good so we get the chance to reflect on that if surprising things happen, even though it is possibly harmless.
>> Thanks,
>> /Erik
>>> 
>>>> 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