Re: [Maria-developers] [Commits] Rev 3965: MDEV-5403 - Reduce usage of LOCK_open: tc_count in lp:maria/10.0
Sergey Vojtovich <svoj@mariadb.org> writes:
+#ifdef __cplusplus +template <typename T, int S> class Atomic_type_triat; + + +template <typename T> class Atomic_type_triat<T, 4>
+template <typename T> class Atomic_type_triat<T, 8>
+template <typename T, typename S= Atomic_type_triat<T, sizeof(T)> > class Atomic
+ T operator++() { return add(1) + 1; }
[lots and lots of more template code snipped] I am vetoing this. We really, _really_ do not want to package up the atomic stuff in even more abstractions and API layers than we already have. Atomics and lock-free stuff is in itself plenty complicated. It is crucial that one can at least see easily in the code what is going on, without also having to spend effort to understand multiple layers of wrapping. As far as I can see, the only thing the templates do is hide stuff (like the data type used and the wrapping in locks on platforms without atomic support?), while what we need is to make those details clear to people reading the code, not to hide them! - Kristian.
Hi Kristian, thanks for your feedback. Yes I added these templates to hide data type and warapping in locks, which I still find reasonable. OTOH I also hid the fact of use of atomic ops. I tend to agree it is wrong. Do you think keeping the former and fixing the latter is acceptable? Thanks, Sergey On Fri, Jan 24, 2014 at 09:05:04AM +0100, Kristian Nielsen wrote:
Sergey Vojtovich <svoj@mariadb.org> writes:
+#ifdef __cplusplus +template <typename T, int S> class Atomic_type_triat; + + +template <typename T> class Atomic_type_triat<T, 4>
+template <typename T> class Atomic_type_triat<T, 8>
+template <typename T, typename S= Atomic_type_triat<T, sizeof(T)> > class Atomic
+ T operator++() { return add(1) + 1; }
[lots and lots of more template code snipped]
I am vetoing this.
We really, _really_ do not want to package up the atomic stuff in even more abstractions and API layers than we already have.
Atomics and lock-free stuff is in itself plenty complicated. It is crucial that one can at least see easily in the code what is going on, without also having to spend effort to understand multiple layers of wrapping.
As far as I can see, the only thing the templates do is hide stuff (like the data type used and the wrapping in locks on platforms without atomic support?), while what we need is to make those details clear to people reading the code, not to hide them!
- Kristian.
I really do not like hiding the type. If you did not think carefully about the type of the underlying variable, you _really_ should not be using something as complex and subtle as an atomic operation. And I _really_ do not like that we would now have two _different_ ways to do atomic operations, one for C and one for C++. It is hard enough to learn one API, having two different ones, that's just stupid. Seriously. We have so many problems with our existing implementation of atomics: - They are very unclear on the essential aspect of memory barrier semantics. They do not provide read-read or write-write barriers. - It even confuse the memory barrier semantics - they call something "atomic store" when it is really a memory-barrier store (synced/locked store). They do not provide any atomic store that does not involve a full memory barrier (on platforms where that is possible). - The code is _completely_ impossible to read, with all the macro hell. Instead of a simple set of inline function definitions for each definition, they do macro magic that basically requires running the preprocessor to get any idea of what they are actually doing. - ... So, if people really want to spend time on improving my_atomic.h - why on earth would they waste their effort on something of as little importance as on whether code says atomic_add() or atomic_add32() ... :-( - Kristian.
Kristian, On Fri, Jan 24, 2014 at 10:49:26AM +0100, Kristian Nielsen wrote:
I really do not like hiding the type. If you did not think carefully about the type of the underlying variable, you _really_ should not be using something as complex and subtle as an atomic operation. Sorry, still don't get what's wrong about hidding the type. I assume you're otherwise fine with hidding locks wrapping?
And I _really_ do not like that we would now have two _different_ ways to do atomic operations, one for C and one for C++. It is hard enough to learn one API, having two different ones, that's just stupid. I would agree if we weren't using C++ at all. Otherwise API duplication seem to be quite common: e.g. include/my_bitmap.h vs sql/sql_bitmap.h, include/my_list.h vs sql/sql_plist.h, include/my_sys.h (DYNAMIC_ARRAY) vs sql/sql_array.h, etc...
Seriously. We have so many problems with our existing implementation of atomics:
- They are very unclear on the essential aspect of memory barrier semantics. They do not provide read-read or write-write barriers.
- It even confuse the memory barrier semantics - they call something "atomic store" when it is really a memory-barrier store (synced/locked store). They do not provide any atomic store that does not involve a full memory barrier (on platforms where that is possible).
Agree.
- The code is _completely_ impossible to read, with all the macro hell. Instead of a simple set of inline function definitions for each definition, they do macro magic that basically requires running the preprocessor to get any idea of what they are actually doing.
Call me masochist, but I enjoyed reading that. :)
- ...
So, if people really want to spend time on improving my_atomic.h - why on earth would they waste their effort on something of as little importance as on whether code says atomic_add() or atomic_add32() ... :-(
- Kristian.
Thanks, Sergey
Kristian, sorry forgot to answer your last question. On Fri, Jan 24, 2014 at 10:49:26AM +0100, Kristian Nielsen wrote: ...
So, if people really want to spend time on improving my_atomic.h - why on earth would they waste their effort on something of as little importance as on whether code says atomic_add() or atomic_add32() ... :-( The reason is simple: it annoyed me while I was implementing my task. OTOH I didn't get that far to get annoyed by excessive memory barriers.
Regards, Sergey
Hi, Kristian! On Jan 24, Kristian Nielsen wrote:
Sergey Vojtovich <svoj@mariadb.org> writes:
+template <typename T, typename S= Atomic_type_triat<T, sizeof(T)> > class Atomic
+ T operator++() { return add(1) + 1; }
[lots and lots of more template code snipped]
I am vetoing this.
We really, _really_ do not want to package up the atomic stuff in even more abstractions and API layers than we already have.
Atomics and lock-free stuff is in itself plenty complicated. It is crucial that one can at least see easily in the code what is going on, without also having to spend effort to understand multiple layers of wrapping.
As far as I can see, the only thing the templates do is hide stuff (like the data type used and the wrapping in locks on platforms without atomic support?), while what we need is to make those details clear to people reading the code, not to hide them!
I partially agree. I don't like that a++; may be an atomic operation intenally. But I do like that one can write my_atomic_add(a, 1); // for example without explicitly specifying the bit width of the 'a'. This is pretty standard feature, it works even in assembler :) Regards, Sergei
participants (3)
-
Kristian Nielsen -
Sergei Golubchik -
Sergey Vojtovich