Drizzle

Merge lp:~olafvdspek/drizzle/refactor11 into lp:~drizzle-trunk/drizzle/development

  1. refactor11
  2. Merge into development
Proposed by Olaf van der Spek
Status: Work in progress
Proposed branch: lp:~olafvdspek/drizzle/refactor11
Merge into: lp:~drizzle-trunk/drizzle/development
Diff against target: 329 lines (+48/-100)
4 files modified
drizzled/atomic/gcc_traits.h (+0/-4)
drizzled/atomic/pthread_traits.h (+0/-6)
drizzled/atomics.h (+39/-53)
unittests/pthread_atomics_test.cc (+9/-37)
To merge this branch: bzr merge lp:~olafvdspek/drizzle/refactor11
Reviewer Review Type Date Requested Status
Monty Taylor Needs Information
Review via email: mp+82055@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Mark Atwood (fallenpegasus) wrote :

Hi. Touching the atomics and the trait constructors needs to be reviewed by Brian. I'm asking him to review this before merging.

Reply
Revision history for this message
Monty Taylor (mordred) wrote :

I would be interested in an explanation of what your refactor is doing in the atomic classes. I'm not saying that it's wrong, but the template code in there isn't exactly friendly, so I'd like to know what it is you're shooting for here.

review: Needs Information
Reply
Revision history for this message
Olaf van der Spek (olafvdspek) wrote :

On Tue, Nov 15, 2011 at 7:48 PM, Monty Taylor <email address hidden> wrote:
> Review: Needs Information
>
> I would be interested in an explanation of what your refactor is doing in the atomic classes. I'm not saying that it's wrong, but the template code in there isn't exactly friendly, so I'd like to know what it is you're shooting for here.

I removed emptry constructors and destructors.
Moved atomic_base into atomic_impl.
Removed a type.

--
Olaf

Reply
Revision history for this message
Monty Taylor (mordred) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/15/2011 05:04 PM, Olaf van der Spek wrote:
> On Tue, Nov 15, 2011 at 7:48 PM, Monty Taylor
> <email address hidden> wrote:
>> Review: Needs Information
>>
>> I would be interested in an explanation of what your refactor is
>> doing in the atomic classes. I'm not saying that it's wrong, but
>> the template code in there isn't exactly friendly, so I'd like to
>> know what it is you're shooting for here.
>
> I removed emptry constructors and destructors. Moved atomic_base
> into atomic_impl. Removed a type.

So - more specifically:

What are you accomplishing with the atomic_base -> atomic_impl move.
(not saying it's bad - I just want to know what you wanted to do here)

Why did you remove the empty constructors? They were preventing
default constructors from writing data to memory locations that we
didn't want them to write to?

Additionally - the pthread_atomics_test was there specifically to test
that the pthread-based atomic class worked, even if you happened to be
running the tests on a machine that happened to have real atomic
support. With the change to use the top-level atomic class, the test
no longer tests that.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk7CvGEACgkQ2Jv7/VK1RgHwjwCg7Z+H8fbm8sVVA2T+EJjpEXgm
T2gAnRewAHJ/sEOshwZj1DlyXazdE02s
=aVWw
-----END PGP SIGNATURE-----

Reply
Revision history for this message
Olaf van der Spek (olafvdspek) wrote :

On Tue, Nov 15, 2011 at 8:28 PM, Monty Taylor <email address hidden> wrote:
> What are you accomplishing with the atomic_base -> atomic_impl move.
> (not saying it's bad - I just want to know what you wanted to do here)

Simpler code.

> Why did you remove the empty constructors? They were preventing
> default constructors from writing data to memory locations that we
> didn't want them to write to?

Eh, to what memory would default constructors write?

> Additionally - the pthread_atomics_test was there specifically to test
> that the pthread-based atomic class worked, even if you happened to be
> running the tests on a machine that happened to have real atomic
> support. With the change to use the top-level atomic class, the test
> no longer tests that.

Ah. Does it make sense to test code you're not going to use though?

--
Olaf

Reply
Revision history for this message
Brian Aker (brianaker) wrote :

@Olaf I'd leave the atomic implementation alone. This is an area where in the future someone could want what you are removing (and what has tests).

re: query_id
Right now it is one step closer to being encapsulated for catalogs, why remove that?

For if(), go on and put { } around what you expect it to execute. Over the years I've found just too many unintended bugs based on code getting shuffled, and no one realizing that the behavior of the branch had changed.

Reply
Revision history for this message
Olaf van der Spek (olafvdspek) wrote :

On Wed, Nov 16, 2011 at 10:08 AM, Brian Aker <email address hidden> wrote:
> @Olaf I'd leave the atomic implementation alone. This is an area where in the future someone could want what you are removing (and what has tests).

I'll restore the tests to test the pthread implementation.
But what removed code could someone want in the future?

> re: query_id
> Right now it is one step closer to being encapsulated for catalogs, why remove that?

How does it relate to catalogs?

Olaf

Reply
lp:~olafvdspek/drizzle/refactor11 updated
2462. By Olaf van der Spek

Merge trunk

Revision history for this message
Olaf van der Spek (olafvdspek) wrote :

On Tue, Nov 15, 2011 at 8:37 PM, Olaf van der Spek <email address hidden> wrote:
> On Tue, Nov 15, 2011 at 8:28 PM, Monty Taylor <email address hidden> wrote:
>> What are you accomplishing with the atomic_base -> atomic_impl move.
>> (not saying it's bad - I just want to know what you wanted to do here)
>
> Simpler code.
>
>> Why did you remove the empty constructors? They were preventing
>> default constructors from writing data to memory locations that we
>> didn't want them to write to?
>
> Eh, to what memory would default constructors write?
>
>> Additionally - the pthread_atomics_test was there specifically to test
>> that the pthread-based atomic class worked, even if you happened to be
>> running the tests on a machine that happened to have real atomic
>> support. With the change to use the top-level atomic class, the test
>> no longer tests that.
>
> Ah. Does it make sense to test code you're not going to use though?

Monty?

--
Olaf

Reply
lp:~olafvdspek/drizzle/refactor11 updated
2463. By Olaf van der Spek

Merge trunk

2464. By Olaf van der Spek

Merge trunk

Unmerged revisions

2464. By Olaf van der Spek

Merge trunk

2463. By Olaf van der Spek

Merge trunk

2462. By Olaf van der Spek

Merge trunk

2461. By Olaf van der Spek

Refactor

2460. By Olaf van der Spek

Refactor

2459. By Olaf van der Spek

Refactor

2458. By Olaf van der Spek

Refactor

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'drizzled/atomic/gcc_traits.h'
--- drizzled/atomic/gcc_traits.h 2011-03-14 05:40:28 +0000
+++ drizzled/atomic/gcc_traits.h 2011-12-16 16:07:29 +0000
@@ -25,12 +25,9 @@
25template<typename T, typename D>25template<typename T, typename D>
26class gcc_traits26class gcc_traits
27{27{
28
29public:28public:
30 typedef T value_type;29 typedef T value_type;
3130
32 gcc_traits() {}
33
34 inline value_type add_and_fetch(volatile value_type *value, D addend )31 inline value_type add_and_fetch(volatile value_type *value, D addend )
35 {32 {
36 return __sync_add_and_fetch(value, addend);33 return __sync_add_and_fetch(value, addend);
@@ -91,4 +88,3 @@
9188
92} /* namespace internal */89} /* namespace internal */
93} /* namespace drizzled */90} /* namespace drizzled */
94
9591
=== modified file 'drizzled/atomic/pthread_traits.h'
--- drizzled/atomic/pthread_traits.h 2011-03-14 05:40:28 +0000
+++ drizzled/atomic/pthread_traits.h 2011-12-16 16:07:29 +0000
@@ -21,7 +21,6 @@
21#pragma once21#pragma once
2222
23namespace drizzled {23namespace drizzled {
24
25namespace internal {24namespace internal {
2625
27class mutex_wrapper26class mutex_wrapper
@@ -61,11 +60,8 @@
61 mutex_wrapper my_lock;60 mutex_wrapper my_lock;
6261
63public:62public:
64
65 typedef T value_type;63 typedef T value_type;
6664
67 pthread_traits() {}
68
69 inline value_type add_and_fetch(volatile value_type *value, D addend )65 inline value_type add_and_fetch(volatile value_type *value, D addend )
70 {66 {
71 my_lock.lock();67 my_lock.lock();
@@ -147,5 +143,3 @@
147143
148} /* namespace internal */144} /* namespace internal */
149} /* namespace drizzled */145} /* namespace drizzled */
150
151
152146
=== modified file 'drizzled/atomics.h'
--- drizzled/atomics.h 2011-08-14 17:04:01 +0000
+++ drizzled/atomics.h 2011-12-16 16:07:29 +0000
@@ -38,80 +38,67 @@
38namespace drizzled {38namespace drizzled {
39namespace internal {39namespace internal {
4040
41template<typename I> // Primary template41template<typename T, typename TR>
42struct atomic_base {42class atomic_impl
43 volatile I my_value;
44 atomic_base() : my_value(0) {}
45 virtual ~atomic_base() {}
46};
47
48template<typename I, typename D, typename T >
49class atomic_impl: private atomic_base<I>
50{43{
51 T traits;44 TR traits;
45 volatile T my_value;
52public:46public:
53 typedef I value_type;47 typedef T value_type;
5448
55 atomic_impl() : atomic_base<I>(), traits() {}49 atomic_impl() : my_value(0) {}
5650
57 value_type add_and_fetch( D addend )51 value_type add_and_fetch( T addend )
58 {52 {
59 return traits.add_and_fetch(&this->my_value, addend);53 return traits.add_and_fetch(&my_value, addend);
60 }54 }
6155
62 value_type fetch_and_add( D addend )56 value_type fetch_and_add( T addend )
63 {57 {
64 return traits.fetch_and_add(&this->my_value, addend);58 return traits.fetch_and_add(&my_value, addend);
65 }59 }
6660
67 value_type fetch_and_increment()61 value_type fetch_and_increment()
68 {62 {
69 return traits.fetch_and_increment(&this->my_value);63 return traits.fetch_and_increment(&my_value);
70 }64 }
7165
72 value_type fetch_and_decrement()66 value_type fetch_and_decrement()
73 {67 {
74 return traits.fetch_and_decrement(&this->my_value);68 return traits.fetch_and_decrement(&my_value);
75 }69 }
7670
77 value_type fetch_and_store( value_type value )71 value_type fetch_and_store( value_type value )
78 {72 {
79 return traits.fetch_and_store(&this->my_value, value);73 return traits.fetch_and_store(&my_value, value);
80 }74 }
8175
82 bool compare_and_swap( value_type value, value_type comparand )76 bool compare_and_swap( value_type value, value_type comparand )
83 {77 {
84 return traits.compare_and_swap(&this->my_value, value, comparand);78 return traits.compare_and_swap(&my_value, value, comparand);
85 }79 }
8680
87 operator value_type() const volatile81 operator value_type() const volatile
88 {82 {
89 return traits.fetch(&this->my_value);83 return traits.fetch(&my_value);
90 }84 }
9185
92 value_type& _internal_reference() const86 void store_with_release( value_type value )
93 {87 {
94 return this->my_value;88 traits.store_with_release(&my_value, value);
95 }89 }
9690
97protected:91 atomic_impl& operator+=( T addend )
98 value_type store_with_release( value_type rhs )
99 {
100 return traits.store_with_release(&this->my_value, rhs);
101 }
102
103public:
104 atomic_impl<I,D,T>& operator+=( D addend )
105 {92 {
106 increment(addend);93 increment(addend);
107 return *this;94 return *this;
108 }95 }
10996
110 atomic_impl<I,D,T>& operator-=( D addend )97 atomic_impl& operator-=( T addend )
111 {98 {
112 // Additive inverse of addend computed using binary minus,99 // Additive inverse of addend computed using binary minus,
113 // instead of unary minus, for sake of avoiding compiler warnings.100 // instead of unary minus, for sake of avoiding compiler warnings.
114 return operator+=(D(0)-addend);101 return operator+=(T(0)-addend);
115 }102 }
116103
117 value_type increment()104 value_type increment()
@@ -121,7 +108,7 @@
121108
122 value_type decrement()109 value_type decrement()
123 {110 {
124 return add_and_fetch(D(-1));111 return add_and_fetch(T(-1));
125 }112 }
126};113};
127114
@@ -131,15 +118,16 @@
131/** See the Reference for details.118/** See the Reference for details.
132 @ingroup synchronization */119 @ingroup synchronization */
133template<typename T>120template<typename T>
134struct atomic {121struct atomic
122{
135};123};
136124
137/* *INDENT-OFF* */125/* *INDENT-OFF* */
138#define __DRIZZLE_DECL_ATOMIC(T) \126#define __DRIZZLE_DECL_ATOMIC(T) \
139 template<> struct atomic<T> \127 template<> \
140 : internal::atomic_impl<T,T,ATOMIC_TRAITS<T,T> > { \128 struct atomic<T> : internal::atomic_impl<T, ATOMIC_TRAITS<T, T> > \
141 atomic<T>() : internal::atomic_impl<T,T,ATOMIC_TRAITS<T,T> >() {} \129 { \
142 atomic<T>& operator=( T rhs ) { store_with_release(rhs); return *this; } \130 void operator=(T v) { store_with_release(v); } \
143 };131 };
144/* *INDENT-ON* */132/* *INDENT-ON* */
145133
@@ -164,11 +152,10 @@
164__DRIZZLE_DECL_ATOMIC(unsigned long long)152__DRIZZLE_DECL_ATOMIC(unsigned long long)
165# else153# else
166# define __DRIZZLE_DECL_ATOMIC64(T) \154# define __DRIZZLE_DECL_ATOMIC64(T) \
167 template<> struct atomic<T> \155 template<> \
168 : internal::atomic_impl<T,T,internal::pthread_traits<T,T> > { \156 struct atomic<T> : internal::atomic_impl<T, internal::pthread_traits<T, T> > \
169 atomic<T>() \157 { \
170 : internal::atomic_impl<T,T,internal::pthread_traits<T,T> >() {} \158 void operator=(T v) { store_with_release(v); } \
171 T operator=( T rhs ) { return store_with_release(rhs); } \
172 };159 };
173__DRIZZLE_DECL_ATOMIC64(long long)160__DRIZZLE_DECL_ATOMIC64(long long)
174__DRIZZLE_DECL_ATOMIC64(unsigned long long)161__DRIZZLE_DECL_ATOMIC64(unsigned long long)
@@ -176,4 +163,3 @@
176/* *INDENT-ON* */163/* *INDENT-ON* */
177164
178}165}
179
180166
=== modified file 'unittests/pthread_atomics_test.cc'
--- unittests/pthread_atomics_test.cc 2011-02-17 00:14:13 +0000
+++ unittests/pthread_atomics_test.cc 2011-12-16 16:07:29 +0000
@@ -21,44 +21,16 @@
21#include <config.h>21#include <config.h>
2222
23#define BOOST_TEST_DYN_LINK23#define BOOST_TEST_DYN_LINK
24
24#include <boost/test/unit_test.hpp>25#include <boost/test/unit_test.hpp>
25
26# if defined(__SUNPRO_CC)
27# include <drizzled/atomic/sun_studio.h>
28# endif
29
30# if !defined(__ICC) && (defined(HAVE_GCC_ATOMIC_BUILTINS) || defined(__SUNPRO_CC))
31# include <drizzled/atomic/gcc_traits.h>
32# define ATOMIC_TRAITS internal::gcc_traits
33# else /* use pthread impl */
34# define ATOMIC_TRAITS internal::pthread_traits
35# endif
36
37#include <pthread.h>
38#include <drizzled/atomic/pthread_traits.h>
39
40#include <drizzled/atomics.h>26#include <drizzled/atomics.h>
4127
42using namespace drizzled;28using namespace drizzled;
4329
44template<typename T>
45struct atomic_pthread {
46};
47
48# define __DRIZZLE_DECL_ATOMIC_PTHREAD(T) \
49 template<> struct atomic_pthread<T> \
50 : internal::atomic_impl<T,T, internal::pthread_traits<T,T> > { \
51 atomic_pthread<T>() \
52 : internal::atomic_impl<T,T, internal::pthread_traits<T,T> >() {} \
53 T operator=( T rhs ) { return store_with_release(rhs); } \
54 };
55
56__DRIZZLE_DECL_ATOMIC_PTHREAD(unsigned int)
57
58BOOST_AUTO_TEST_SUITE(PthreadAtomicOperations)30BOOST_AUTO_TEST_SUITE(PthreadAtomicOperations)
59BOOST_AUTO_TEST_CASE(fetch_and_store)31BOOST_AUTO_TEST_CASE(fetch_and_store)
60{32{
61 atomic_pthread<uint32_t> u235;33 atomic<uint32_t> u235;
6234
63 BOOST_REQUIRE_EQUAL(0, u235.fetch_and_store(1));35 BOOST_REQUIRE_EQUAL(0, u235.fetch_and_store(1));
6436
@@ -70,7 +42,7 @@
7042
71BOOST_AUTO_TEST_CASE(fetch_and_increment)43BOOST_AUTO_TEST_CASE(fetch_and_increment)
72{44{
73 atomic_pthread<uint32_t> u235;45 atomic<uint32_t> u235;
7446
75 BOOST_REQUIRE_EQUAL(0, u235.fetch_and_increment());47 BOOST_REQUIRE_EQUAL(0, u235.fetch_and_increment());
76 BOOST_REQUIRE_EQUAL(1, u235);48 BOOST_REQUIRE_EQUAL(1, u235);
@@ -78,7 +50,7 @@
7850
79BOOST_AUTO_TEST_CASE(fetch_and_add)51BOOST_AUTO_TEST_CASE(fetch_and_add)
80{52{
81 atomic_pthread<uint32_t> u235;53 atomic<uint32_t> u235;
8254
83 BOOST_REQUIRE_EQUAL(0, u235.fetch_and_add(2));55 BOOST_REQUIRE_EQUAL(0, u235.fetch_and_add(2));
84 BOOST_REQUIRE_EQUAL(2, u235);56 BOOST_REQUIRE_EQUAL(2, u235);
@@ -86,7 +58,7 @@
8658
87BOOST_AUTO_TEST_CASE(add_and_fetch)59BOOST_AUTO_TEST_CASE(add_and_fetch)
88{60{
89 atomic_pthread<uint32_t> u235;61 atomic<uint32_t> u235;
9062
91 BOOST_REQUIRE_EQUAL(10, u235.add_and_fetch(10));63 BOOST_REQUIRE_EQUAL(10, u235.add_and_fetch(10));
92 BOOST_REQUIRE_EQUAL(10, u235);64 BOOST_REQUIRE_EQUAL(10, u235);
@@ -94,7 +66,7 @@
9466
95BOOST_AUTO_TEST_CASE(fetch_and_decrement)67BOOST_AUTO_TEST_CASE(fetch_and_decrement)
96{68{
97 atomic_pthread<uint32_t> u235;69 atomic<uint32_t> u235;
9870
99 u235.fetch_and_store(15);71 u235.fetch_and_store(15);
10072
@@ -104,7 +76,7 @@
10476
105BOOST_AUTO_TEST_CASE(compare_and_swap)77BOOST_AUTO_TEST_CASE(compare_and_swap)
106{78{
107 atomic_pthread<uint32_t> u235;79 atomic<uint32_t> u235;
10880
109 u235.fetch_and_store(100);81 u235.fetch_and_store(100);
11082
@@ -115,14 +87,14 @@
11587
116BOOST_AUTO_TEST_CASE(increment)88BOOST_AUTO_TEST_CASE(increment)
117{89{
118 atomic_pthread<uint32_t> u235;90 atomic<uint32_t> u235;
119 u235.fetch_and_store(200);91 u235.fetch_and_store(200);
120 BOOST_REQUIRE_EQUAL(201, u235.increment());92 BOOST_REQUIRE_EQUAL(201, u235.increment());
121}93}
12294
123BOOST_AUTO_TEST_CASE(decrement)95BOOST_AUTO_TEST_CASE(decrement)
124{96{
125 atomic_pthread<uint32_t> u235;97 atomic<uint32_t> u235;
126 u235.fetch_and_store(200);98 u235.fetch_and_store(200);
12799
128 BOOST_REQUIRE_EQUAL(199, u235.decrement());100 BOOST_REQUIRE_EQUAL(199, u235.decrement());