Skip to content

Commit 8f03995

Browse files
est31est31
committedNov 4, 2015
Time: use locks again
The Atomic implementation was only partially correct, and was very complex. Use locks for sake of simplicity, following KISS principle. Only remaining atomic operation use is time of day speed, because that really is only read + written. Also fixes a bug with m_time_conversion_skew only being decremented, never incremented (Regresion from previous commit). atomic.h changes: * Add GenericAtomic<T> class for non-integral types like floats. * Remove some last remainders from atomic.h of the volatile use.
·
5.11.00.4.14
1 parent f9b0936 commit 8f03995

File tree

3 files changed

+77
-48
lines changed

3 files changed

+77
-48
lines changed
 

‎src/environment.cpp

+23-14
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,12 @@ with this program; if not, write to the Free Software Foundation, Inc.,
4949
#define PP(x) "("<<(x).X<<","<<(x).Y<<","<<(x).Z<<")"
5050

5151
Environment::Environment():
52+
m_time_of_day_speed(0),
5253
m_time_of_day(9000),
5354
m_time_of_day_f(9000./24000),
54-
m_time_of_day_speed(0),
55-
m_time_conversion_skew(0),
56-
m_day_night_ratio_override_storage(0)
55+
m_time_conversion_skew(0.0f),
56+
m_enable_day_night_ratio_override(false),
57+
m_day_night_ratio_override(0.0f)
5758
{
5859
m_cache_enable_shaders = g_settings->getBool("enable_shaders");
5960
}
@@ -179,10 +180,9 @@ std::vector<Player*> Environment::getPlayers(bool ignore_disconnected)
179180

180181
u32 Environment::getDayNightRatio()
181182
{
182-
u64 day_night_st = m_day_night_ratio_override_storage;
183-
if (day_night_st & ((u64)1 << 63))
184-
return day_night_st & U32_MAX;
185-
MutexAutoLock lock(this->m_time_floats_lock);
183+
MutexAutoLock lock(this->m_time_lock);
184+
if (m_enable_day_night_ratio_override)
185+
return m_day_night_ratio_override;
186186
return time_to_daynight_ratio(m_time_of_day_f * 24000, m_cache_enable_shaders);
187187
}
188188

@@ -196,29 +196,38 @@ float Environment::getTimeOfDaySpeed()
196196
return m_time_of_day_speed;
197197
}
198198

199+
void Environment::setDayNightRatioOverride(bool enable, u32 value)
200+
{
201+
MutexAutoLock lock(this->m_time_lock);
202+
m_enable_day_night_ratio_override = enable;
203+
m_day_night_ratio_override = value;
204+
}
205+
199206
void Environment::setTimeOfDay(u32 time)
200207
{
201-
MutexAutoLock lock(this->m_time_floats_lock);
208+
MutexAutoLock lock(this->m_time_lock);
202209
m_time_of_day = time;
203210
m_time_of_day_f = (float)time / 24000.0;
204211
}
205212

206213
u32 Environment::getTimeOfDay()
207214
{
215+
MutexAutoLock lock(this->m_time_lock);
208216
return m_time_of_day;
209217
}
210218

211219
float Environment::getTimeOfDayF()
212220
{
213-
MutexAutoLock lock(this->m_time_floats_lock);
221+
MutexAutoLock lock(this->m_time_lock);
214222
return m_time_of_day_f;
215223
}
216224

217225
void Environment::stepTimeOfDay(float dtime)
218226
{
219-
MutexAutoLock lock(this->m_time_floats_lock);
227+
MutexAutoLock lock(this->m_time_lock);
220228
f32 speed = m_time_of_day_speed * 24000. / (24. * 3600);
221-
u32 units = (u32)((dtime + m_time_conversion_skew) * speed);
229+
m_time_conversion_skew += dtime;
230+
u32 units = (u32)(m_time_conversion_skew * speed);
222231
bool sync_f = false;
223232
if (units > 0) {
224233
// Sync at overflow
@@ -232,7 +241,7 @@ void Environment::stepTimeOfDay(float dtime)
232241
m_time_conversion_skew -= (f32)units / speed;
233242
}
234243
if (!sync_f) {
235-
m_time_of_day_f += m_time_of_day_speed / (24. * 3600.) * dtime;
244+
m_time_of_day_f += speed * dtime;
236245
if (m_time_of_day_f > 1.0)
237246
m_time_of_day_f -= 1.0;
238247
if (m_time_of_day_f < 0.0)
@@ -527,10 +536,10 @@ void ServerEnvironment::loadMeta()
527536
}
528537

529538
try {
530-
m_time_of_day = args.getU64("time_of_day");
539+
setTimeOfDay(args.getU64("time_of_day"));
531540
} catch (SettingNotFoundException &e) {
532541
// This is not as important
533-
m_time_of_day = 9000;
542+
setTimeOfDay(9000);
534543
}
535544
}
536545

‎src/environment.h

+10-14
Original file line numberDiff line numberDiff line change
@@ -93,10 +93,7 @@ class Environment
9393
void setTimeOfDaySpeed(float speed);
9494
float getTimeOfDaySpeed();
9595

96-
void setDayNightRatioOverride(bool enable, u32 value)
97-
{
98-
m_day_night_ratio_override_storage = value | ((u64)enable << 63);
99-
}
96+
void setDayNightRatioOverride(bool enable, u32 value);
10097

10198
// counter used internally when triggering ABMs
10299
u32 m_added_objects;
@@ -105,26 +102,25 @@ class Environment
105102
// peer_ids in here should be unique, except that there may be many 0s
106103
std::vector<Player*> m_players;
107104

108-
// Time of day in milli-hours (0-23999); determines day and night
109-
Atomic<u32> m_time_of_day;
105+
GenericAtomic<float> m_time_of_day_speed;
110106

111107
/*
112-
* Below: values managed by m_time_floats_lock
108+
* Below: values managed by m_time_lock
113109
*/
110+
// Time of day in milli-hours (0-23999); determines day and night
111+
u32 m_time_of_day;
114112
// Time of day in 0...1
115113
float m_time_of_day_f;
116-
float m_time_of_day_speed;
117114
// Stores the skew created by the float -> u32 conversion
118115
// to be applied at next conversion, so that there is no real skew.
119116
float m_time_conversion_skew;
117+
// Overriding the day-night ratio is useful for custom sky visuals
118+
bool m_enable_day_night_ratio_override;
119+
u32 m_day_night_ratio_override;
120120
/*
121-
* Above: values managed by m_time_floats_lock
121+
* Above: values managed by m_time_lock
122122
*/
123123

124-
// Overriding the day-night ratio is useful for custom sky visuals
125-
// lowest 32 bits store the overriden ratio, highest bit stores whether its enabled
126-
Atomic<u64> m_day_night_ratio_override_storage;
127-
128124
/* TODO: Add a callback function so these can be updated when a setting
129125
* changes. At this point in time it doesn't matter (e.g. /set
130126
* is documented to change server settings only)
@@ -137,7 +133,7 @@ class Environment
137133
bool m_cache_enable_shaders;
138134

139135
private:
140-
Mutex m_time_floats_lock;
136+
Mutex m_time_lock;
141137

142138
DISABLE_CLASS_COPY(Environment);
143139
};

‎src/threading/atomic.h

+44-20
Original file line numberDiff line numberDiff line change
@@ -24,19 +24,25 @@ with this program; if not, write to the Free Software Foundation, Inc.,
2424
#if __cplusplus >= 201103L
2525
#include <atomic>
2626
template<typename T> using Atomic = std::atomic<T>;
27+
template<typename T> using GenericAtomic = std::atomic<T>;
2728
#else
2829

2930
#define GCC_VERSION (__GNUC__ * 100 + __GNUC_MINOR__)
3031
#define CLANG_VERSION (__clang_major__ * 100 + __clang_minor__)
3132
#if GCC_VERSION >= 407 || CLANG_VERSION >= 302
33+
#define ATOMIC_LOAD_GENERIC(T, v) do { \
34+
T _val; \
35+
__atomic_load(&(v), &(_val), __ATOMIC_SEQ_CST); \
36+
return _val; \
37+
} while(0)
3238
#define ATOMIC_LOAD(T, v) return __atomic_load_n (&(v), __ATOMIC_SEQ_CST)
3339
#define ATOMIC_STORE(T, v, x) __atomic_store (&(v), &(x), __ATOMIC_SEQ_CST); return x
34-
#define ATOMIC_EXCHANGE(T, v, x) return __atomic_exchange_n(&(v), (x), __ATOMIC_SEQ_CST)
40+
#define ATOMIC_EXCHANGE(T, v, x) return __atomic_exchange (&(v), &(x), __ATOMIC_SEQ_CST)
3541
#define ATOMIC_ADD_EQ(T, v, x) return __atomic_add_fetch (&(v), (x), __ATOMIC_SEQ_CST)
3642
#define ATOMIC_SUB_EQ(T, v, x) return __atomic_sub_fetch (&(v), (x), __ATOMIC_SEQ_CST)
3743
#define ATOMIC_POST_INC(T, v) return __atomic_fetch_add (&(v), 1, __ATOMIC_SEQ_CST)
3844
#define ATOMIC_POST_DEC(T, v) return __atomic_fetch_sub (&(v), 1, __ATOMIC_SEQ_CST)
39-
#define ATOMIC_CAS(T, v, e, d) return __atomic_compare_exchange_n(&(v), &(e), (d), \
45+
#define ATOMIC_CAS(T, v, e, d) return __atomic_compare_exchange(&(v), &(e), &(d), \
4046
false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST)
4147
#else
4248
#define ATOMIC_USE_LOCK
@@ -56,12 +62,9 @@ with this program; if not, write to the Free Software Foundation, Inc.,
5662
m_mutex.unlock(); \
5763
return _eq; \
5864
} while (0)
59-
#define ATOMIC_LOAD(T, v) \
60-
if (sizeof(T) <= sizeof(void*)) return v; \
61-
else ATOMIC_LOCK_OP(T, v);
62-
#define ATOMIC_STORE(T, v, x) \
63-
if (sizeof(T) <= sizeof(void*)) return v = x; \
64-
else ATOMIC_LOCK_OP(T, v = x);
65+
#define ATOMIC_LOAD(T, v) ATOMIC_LOCK_OP(T, v)
66+
#define ATOMIC_LOAD_GENERIC(T, v) ATOMIC_LOAD(T, v)
67+
#define ATOMIC_STORE(T, v, x) ATOMIC_LOCK_OP(T, v = x)
6568
#define ATOMIC_EXCHANGE(T, v, x) do { \
6669
m_mutex.lock(); \
6770
T _val = v; \
@@ -84,32 +87,53 @@ with this program; if not, write to the Free Software Foundation, Inc.,
8487
#endif
8588
#endif
8689

87-
90+
// For usage with integral types.
8891
template<typename T>
8992
class Atomic {
9093
public:
91-
Atomic(const T &v = 0) : val(v) {}
94+
Atomic(const T &v = 0) : m_val(v) {}
9295

93-
operator T () { ATOMIC_LOAD(T, val); }
96+
operator T () { ATOMIC_LOAD(T, m_val); }
9497

95-
T exchange(T x) { ATOMIC_EXCHANGE(T, val, x); }
96-
bool compare_exchange_strong(T &expected, T desired) { ATOMIC_CAS(T, val, expected, desired); }
98+
T exchange(T x) { ATOMIC_EXCHANGE(T, m_val, x); }
99+
bool compare_exchange_strong(T &expected, T desired) { ATOMIC_CAS(T, m_val, expected, desired); }
97100

98-
T operator = (T x) { ATOMIC_STORE(T, val, x); }
99-
T operator += (T x) { ATOMIC_ADD_EQ(T, val, x); }
100-
T operator -= (T x) { ATOMIC_SUB_EQ(T, val, x); }
101+
T operator = (T x) { ATOMIC_STORE(T, m_val, x); }
102+
T operator += (T x) { ATOMIC_ADD_EQ(T, m_val, x); }
103+
T operator -= (T x) { ATOMIC_SUB_EQ(T, m_val, x); }
101104
T operator ++ () { return *this += 1; }
102105
T operator -- () { return *this -= 1; }
103-
T operator ++ (int) { ATOMIC_POST_INC(T, val); }
104-
T operator -- (int) { ATOMIC_POST_DEC(T, val); }
106+
T operator ++ (int) { ATOMIC_POST_INC(T, m_val); }
107+
T operator -- (int) { ATOMIC_POST_DEC(T, m_val); }
105108
private:
106-
T val;
109+
T m_val;
107110
#ifdef ATOMIC_USE_LOCK
108111
Mutex m_mutex;
109112
#endif
110113
};
111114

112-
#endif // C++11
115+
// For usage with non-integral types like float for example.
116+
// Needed because the other operations aren't provided by gcc
117+
// for non-integral types:
118+
// https://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/_005f_005fatomic-Builtins.html
119+
template<typename T>
120+
class GenericAtomic {
121+
public:
122+
GenericAtomic(const T &v = 0) : m_val(v) {}
113123

124+
operator T () { ATOMIC_LOAD_GENERIC(T, m_val); }
125+
126+
T exchange(T x) { ATOMIC_EXCHANGE(T, m_val, x); }
127+
bool compare_exchange_strong(T &expected, T desired) { ATOMIC_CAS(T, m_val, expected, desired); }
128+
129+
T operator = (T x) { ATOMIC_STORE(T, m_val, x); }
130+
private:
131+
T m_val;
132+
#ifdef ATOMIC_USE_LOCK
133+
Mutex m_mutex;
114134
#endif
135+
};
115136

137+
#endif // C++11
138+
139+
#endif

2 commit comments

Comments
 (2)

ShadowNinja commented on Nov 12, 2015

@ShadowNinja
ShadowNinja
on Nov 12, 2015
Contributor

Instead of adding a GenericAtomic class you should just add locks for the unsuported operation/type combinations.

est31 commented on Nov 12, 2015

@est31
est31
on Nov 12, 2015
ContributorAuthor

That's bad IMO because then people will use these operations, assuming they were fast. Atomic should stay as close to hardware as possible, that's the whole point why it exists the first place.

Please sign in to comment.