[9] RFR(XS): 8176513 Poor code quality for ByteBuffers

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Roland Westrelin-3

http://cr.openjdk.java.net/~roland/8176513/webrev.01/

This changes relaxes the condition under which MemBarCPUOrder nodes are
added around unsafe accesses. If the object being accessed is an array
and the access is mismatched, the current code add useless barriers
(there's only one memory slice for the entire array so no risk of
breaking alias analysis). If the base is not known to be not null,
membars are added as well. These membars are not added for arrays in 8u
and that hasn't caused any known problem. So in order to keep this
change simple, we propose removing that condition (this was suggested by
Vladimir Ivanov in the comments for this bug).

Roland.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Vladimir Kozlov
Good for JDK 9. Since Vladimir I. suggested the fix I assume he reviewer too.

Roland, did you get ByteBuffers some performance back?

I will need to run pre-integration testing before pushing.

Thanks,
Vladimir

On 3/15/17 2:48 AM, Roland Westrelin wrote:

>
> http://cr.openjdk.java.net/~roland/8176513/webrev.01/
>
> This changes relaxes the condition under which MemBarCPUOrder nodes are
> added around unsafe accesses. If the object being accessed is an array
> and the access is mismatched, the current code add useless barriers
> (there's only one memory slice for the entire array so no risk of
> breaking alias analysis). If the base is not known to be not null,
> membars are added as well. These membars are not added for arrays in 8u
> and that hasn't caused any known problem. So in order to keep this
> change simple, we propose removing that condition (this was suggested by
> Vladimir Ivanov in the comments for this bug).
>
> Roland.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Vladimir Ivanov
> Good for JDK 9. Since Vladimir I. suggested the fix I assume he reviewer
> too.
>
> Roland, did you get ByteBuffers some performance back?
>
> I will need to run pre-integration testing before pushing.

FYI I've just submitted the test batch.

Slightly adjusted the fix as well:

-          need_mem_bar = mismatched || can_access_non_heap;
+          need_mem_bar = mismatched && !adr_type->isa_aryptr();

Best regards,
Vladimir Ivanov

> On 3/15/17 2:48 AM, Roland Westrelin wrote:
>>
>> http://cr.openjdk.java.net/~roland/8176513/webrev.01/
>>
>> This changes relaxes the condition under which MemBarCPUOrder nodes are
>> added around unsafe accesses. If the object being accessed is an array
>> and the access is mismatched, the current code add useless barriers
>> (there's only one memory slice for the entire array so no risk of
>> breaking alias analysis). If the base is not known to be not null,
>> membars are added as well. These membars are not added for arrays in 8u
>> and that hasn't caused any known problem. So in order to keep this
>> change simple, we propose removing that condition (this was suggested by
>> Vladimir Ivanov in the comments for this bug).
>>
>> Roland.
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Vladimir Ivanov
In reply to this post by Roland Westrelin-3
I think alias_type->adr_type() should be used instead, because
adr_type->isa_aryptr() is true for pointers into array header as well,
but we care about array body here. Alias analysis should flatten them to
InstPtr.

Best regards,
Vladimir Ivanov

On 3/15/17 12:48 PM, Roland Westrelin wrote:

>
> http://cr.openjdk.java.net/~roland/8176513/webrev.01/
>
> This changes relaxes the condition under which MemBarCPUOrder nodes are
> added around unsafe accesses. If the object being accessed is an array
> and the access is mismatched, the current code add useless barriers
> (there's only one memory slice for the entire array so no risk of
> breaking alias analysis). If the base is not known to be not null,
> membars are added as well. These membars are not added for arrays in 8u
> and that hasn't caused any known problem. So in order to keep this
> change simple, we propose removing that condition (this was suggested by
> Vladimir Ivanov in the comments for this bug).
>
> Roland.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Roland Westrelin-3
In reply to this post by Vladimir Kozlov

Thanks for looking at this, Vladimir.

> Roland, did you get ByteBuffers some performance back?

Code quality does look much better with this.

Roland.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Roland Westrelin-3
In reply to this post by Vladimir Ivanov

> FYI I've just submitted the test batch.
>
> Slightly adjusted the fix as well:
>
> -          need_mem_bar = mismatched || can_access_non_heap;
> +          need_mem_bar = mismatched && !adr_type->isa_aryptr();

Thanks for fixing this and taking care of testing.

Roland.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Roland Westrelin-3
In reply to this post by Vladimir Ivanov

> I think alias_type->adr_type() should be used instead, because
> adr_type->isa_aryptr() is true for pointers into array header as well,
> but we care about array body here. Alias analysis should flatten them to
> InstPtr.

Aren't those forbidden anyway?

  if (alias_type->adr_type() == TypeInstPtr::KLASS ||
      alias_type->adr_type() == TypeAryPtr::RANGE) {
    return false; // not supported
  }

Roland.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Vladimir Ivanov
Good point, Roland. Missed those checks.

What is missing is TypeInstPtr::MARK, but I'm fine with fixing that later.

Best regards,
Vladimir Ivanov

On 3/15/17 6:34 PM, Roland Westrelin wrote:

>
>> I think alias_type->adr_type() should be used instead, because
>> adr_type->isa_aryptr() is true for pointers into array header as well,
>> but we care about array body here. Alias analysis should flatten them to
>> InstPtr.
>
> Aren't those forbidden anyway?
>
>   if (alias_type->adr_type() == TypeInstPtr::KLASS ||
>       alias_type->adr_type() == TypeAryPtr::RANGE) {
>     return false; // not supported
>   }
>
> Roland.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Vladimir Kozlov
So what is final fix looks like?

Vladimir K


Sent from my iPhone

> On Mar 15, 2017, at 8:50 AM, Vladimir Ivanov <[hidden email]> wrote:
>
> Good point, Roland. Missed those checks.
>
> What is missing is TypeInstPtr::MARK, but I'm fine with fixing that later.
>
> Best regards,
> Vladimir Ivanov
>
>> On 3/15/17 6:34 PM, Roland Westrelin wrote:
>>
>>> I think alias_type->adr_type() should be used instead, because
>>> adr_type->isa_aryptr() is true for pointers into array header as well,
>>> but we care about array body here. Alias analysis should flatten them to
>>> InstPtr.
>>
>> Aren't those forbidden anyway?
>>
>>  if (alias_type->adr_type() == TypeInstPtr::KLASS ||
>>      alias_type->adr_type() == TypeAryPtr::RANGE) {
>>    return false; // not supported
>>  }
>>
>> Roland.
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

John Rose-3
In reply to this post by Roland Westrelin-3
On Mar 15, 2017, at 2:48 AM, Roland Westrelin <[hidden email]> wrote:

This changes relaxes the condition under which MemBarCPUOrder nodes are
added around unsafe accesses. If the object being accessed is an array
and the access is mismatched, the current code add useless barriers
(there's only one memory slice for the entire array so no risk of
breaking alias analysis).

This is reasonable.  The CPUOrder is intended to prevent an unsafe access
from reordering with a nominal (non-unsafe) field access, since it is impossible
(in general!) to tell if an unsafe access might alias with a field access.

From the JMM point of view, a *single* array element works like a field
(both are variables in the heap).  Thus, we also don't want to reorder
an unsafe access *to a particular array element* with a normal "*aload"
or "*astore" access.

Reordering accesses between unsafe loads or stores is less of a problem
I think, because, well, they are unsafe.  User responsibility is to avoid alias
issues, and even fix with explicit fences if necessary.

There also a possible argument of "user responsibility" for the interaction
of unsafe access with normal access; when a bunch of unsafe access code
is done, the effects must post correctly to normal access, without reordering.
(Same point backwards in time/HB-relation also.)  We might say users should
put fences around all unsafe code, but they don't (today) so a little more
care is appropriate here.  I recommend aiming to put implicit CPU order
fences between unsafe-access code and "innocent" normal code, when
possible.  The current overuse of fences conservatively achieves this.

If the base is not known to be not null,
membars are added as well.

These are present "just in case" the accessed field was in fact volatile.
They are not relevant to normal fields.  They are not relevant to array
elements (since array variables are never volatile, nor, alas, final).

These membars are not added for arrays in 8u
and that hasn't caused any known problem. So in order to keep this
change simple, we propose removing that condition (this was suggested by
Vladimir Ivanov in the comments for this bug).

Thank you for working on this.

— John
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Vladimir Ivanov
In reply to this post by Vladimir Kozlov
The following one is being tested:

diff --git a/src/share/vm/opto/library_call.cpp
b/src/share/vm/opto/library_call.cpp
--- a/src/share/vm/opto/library_call.cpp
+++ b/src/share/vm/opto/library_call.cpp
@@ -2375,7 +2375,7 @@
    bool need_mem_bar;
    switch (kind) {
        case Relaxed:
-          need_mem_bar = mismatched || can_access_non_heap;
+          need_mem_bar = mismatched && !adr_type->isa_aryptr();
            break;
        case Opaque:
            // Opaque uses CPUOrder membars for protection against code
movement.

Best regards,
Vladimir Ivanov

On 3/15/17 7:54 PM, Vladimir Kozlov wrote:

> So what is final fix looks like?
>
> Vladimir K
>
>
> Sent from my iPhone
>> On Mar 15, 2017, at 8:50 AM, Vladimir Ivanov <[hidden email]> wrote:
>>
>> Good point, Roland. Missed those checks.
>>
>> What is missing is TypeInstPtr::MARK, but I'm fine with fixing that later.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>>> On 3/15/17 6:34 PM, Roland Westrelin wrote:
>>>
>>>> I think alias_type->adr_type() should be used instead, because
>>>> adr_type->isa_aryptr() is true for pointers into array header as well,
>>>> but we care about array body here. Alias analysis should flatten them to
>>>> InstPtr.
>>>
>>> Aren't those forbidden anyway?
>>>
>>>  if (alias_type->adr_type() == TypeInstPtr::KLASS ||
>>>      alias_type->adr_type() == TypeAryPtr::RANGE) {
>>>    return false; // not supported
>>>  }
>>>
>>> Roland.
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Roland Westrelin-3
In reply to this post by John Rose-3

Thanks for looking at this, John.

If you get a chance, can you take a look at 8176506 (it's for 10 so not
as urgent):

http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-March/025818.html

Roland.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(XS): 8176513 Poor code quality for ByteBuffers

Roland Westrelin-3
In reply to this post by Roland Westrelin-3

I saw it's pushed. Thanks!

Roland.
Loading...