Percona Server moved to https://jira.percona.com/projects/PS

Code review comment for lp:~laurynas-biveinis/percona-server/bp-split-5.6

bp-split-5.6
Merge into 5.6

Revision history for this message

Alexey Kopytov (akopytov) wrote on 2013-09-13:

On Fri, 13 Sep 2013 11:10:36 -0000, Laurynas Biveinis wrote:
>
>
> Right. os_atomic_load_ulint() has additional restrictions on its arg over os_atomic_inc_ulint(). I believe that these restrictions are reasonable. It is a performance bug in any case to perform misaligned atomic ops even with those ops that make it technically possible. I have added ut_ad()s to catch this. I can rename os_atomic_ prefix to os_atomic_aligned_ prefix too, although that one looks like an overkill to me.
>

The same restrictions would apply even if os_atomic_load_ulint() didn't
exist, right? I.e. the same restrictions would apply if we simply
accessed those variables without any helper functions?

Let me ask you a few simple questions and this time around I demand
"yes/no" answers.

- Do you agree that os_atomic_load_ulint() / os_atomic_store_ulint() do
not do what they promise to do?

- Do you agree that naming them os_ordered_load_ulint() /
os_ordered_store_ulint() would better reflect what they do?

- Do you agree that naming them that way also makes it obvious that
using them in most places is simply unnecessary (e.g. in
buf_get_total_stat(), buf_mark_space_corrupt(), buf_print_instance(),
buf_get_n_pending_read_ios(), etc.)?

>
>>> 2. is enforced by platform #ifdefs not providing any other implementation
>> except one for x86/x86_64 with GCC or a GCC-like compiler.
>>>
>>
>> That's correct. I only mentioned #2 for completeness.
>
>
> OK, but I am not sure what does the #2 complete then.
>
>
>>> Thus I provide generic primitives, whose current implementations will work
>> as designed. However the 1. above also seems to be missing "properly-aligned"
>> and that's where the design is debatable. On one hand it is possible to
>> implement misaligned access atomically by LOCK MOV, and document that the
>> primitives may be used with args of any alignment. But a better alternative
>> to me seems to accept that misaligned accesses are bugs and document/allow
>> aligned accesses only. Even though that's enforceable in debug builds only,
>> so that's not ideally perfect, but IMHO acceptable.
>>>
>>
>> You don't.
>
>
> Will you reply to the rest of that paragraph too please? I am acknowledging that alignment is an issue, so let's see how to resolve it.
>

I don't think enforcing requirements in debug builds only is acceptable.
It must be a compile-time assertion, not a run-time one.

On Fri, 13 Sep 2013 11:10:36 -0000, Laurynas Biveinis wrote:
>
>
> Right. os_atomic_load_ulint() has additional restrictions on its arg over os_atomic_inc_ulint().  I believe that these restrictions are reasonable.  It is a performance bug in any case to perform misaligned atomic ops even with those ops that make it technically possible.  I have added ut_ad()s to catch this.  I can rename os_atomic_ prefix to os_atomic_aligned_ prefix too, although that one looks like an overkill to me.
>

The same restrictions would apply even if os_atomic_load_ulint() didn't 
exist, right? I.e. the same restrictions would apply if we simply 
accessed those variables without any helper functions?

Let me ask you a few simple questions and this time around I demand 
"yes/no" answers.

- Do you agree that os_atomic_load_ulint() / os_atomic_store_ulint() do 
not do what they promise to do?

- Do you agree that naming them os_ordered_load_ulint() / 
os_ordered_store_ulint() would better reflect what they do?

- Do you agree that naming them that way also makes it obvious that 
using them in most places is simply unnecessary (e.g. in 
buf_get_total_stat(), buf_mark_space_corrupt(), buf_print_instance(), 
buf_get_n_pending_read_ios(), etc.)?

>
>>> 2. is enforced by platform #ifdefs not providing any other implementation
>> except one for x86/x86_64 with GCC or a GCC-like compiler.
>>>
>>
>> That's correct. I only mentioned #2 for completeness.
>
>
> OK, but I am not sure what does the #2 complete then.
>
>
>>> Thus I provide generic primitives, whose current implementations will work
>> as designed.  However the 1. above also seems to be missing "properly-aligned"
>> and that's where the design is debatable.  On one hand it is possible to
>> implement misaligned access atomically by LOCK MOV, and document that the
>> primitives may be used with args of any alignment.  But a better alternative
>> to me seems to accept that misaligned accesses are bugs and document/allow
>> aligned accesses only.  Even though that's enforceable in debug builds only,
>> so that's not ideally perfect, but IMHO acceptable.
>>>
>>
>> You don't.
>
>
> Will you reply to the rest of that paragraph too please?  I am acknowledging that alignment is an issue, so let's see how to resolve it.
>

I don't think enforcing requirements in debug builds only is acceptable. 
It must be a compile-time assertion, not a run-time one.

« Back to merge proposal