Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
est31 committed Nov 4, 2015
1 parent f9b0936 commit 8f03995
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 48 deletions.
37 changes: 23 additions & 14 deletions src/environment.cpp
Expand Up @@ -49,11 +49,12 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#define PP(x) "("<<(x).X<<","<<(x).Y<<","<<(x).Z<<")"

Environment::Environment():
m_time_of_day_speed(0),
m_time_of_day(9000),
m_time_of_day_f(9000./24000),
m_time_of_day_speed(0),
m_time_conversion_skew(0),
m_day_night_ratio_override_storage(0)
m_time_conversion_skew(0.0f),
m_enable_day_night_ratio_override(false),
m_day_night_ratio_override(0.0f)
{
m_cache_enable_shaders = g_settings->getBool("enable_shaders");
}
Expand Down Expand Up @@ -179,10 +180,9 @@ std::vector<Player*> Environment::getPlayers(bool ignore_disconnected)

u32 Environment::getDayNightRatio()
{
u64 day_night_st = m_day_night_ratio_override_storage;
if (day_night_st & ((u64)1 << 63))
return day_night_st & U32_MAX;
MutexAutoLock lock(this->m_time_floats_lock);
MutexAutoLock lock(this->m_time_lock);
if (m_enable_day_night_ratio_override)
return m_day_night_ratio_override;
return time_to_daynight_ratio(m_time_of_day_f * 24000, m_cache_enable_shaders);
}

Expand All @@ -196,29 +196,38 @@ float Environment::getTimeOfDaySpeed()
return m_time_of_day_speed;
}

void Environment::setDayNightRatioOverride(bool enable, u32 value)
{
MutexAutoLock lock(this->m_time_lock);
m_enable_day_night_ratio_override = enable;
m_day_night_ratio_override = value;
}

void Environment::setTimeOfDay(u32 time)
{
MutexAutoLock lock(this->m_time_floats_lock);
MutexAutoLock lock(this->m_time_lock);
m_time_of_day = time;
m_time_of_day_f = (float)time / 24000.0;
}

u32 Environment::getTimeOfDay()
{
MutexAutoLock lock(this->m_time_lock);
return m_time_of_day;
}

float Environment::getTimeOfDayF()
{
MutexAutoLock lock(this->m_time_floats_lock);
MutexAutoLock lock(this->m_time_lock);
return m_time_of_day_f;
}

void Environment::stepTimeOfDay(float dtime)
{
MutexAutoLock lock(this->m_time_floats_lock);
MutexAutoLock lock(this->m_time_lock);
f32 speed = m_time_of_day_speed * 24000. / (24. * 3600);
u32 units = (u32)((dtime + m_time_conversion_skew) * speed);
m_time_conversion_skew += dtime;
u32 units = (u32)(m_time_conversion_skew * speed);
bool sync_f = false;
if (units > 0) {
// Sync at overflow
Expand All @@ -232,7 +241,7 @@ void Environment::stepTimeOfDay(float dtime)
m_time_conversion_skew -= (f32)units / speed;
}
if (!sync_f) {
m_time_of_day_f += m_time_of_day_speed / (24. * 3600.) * dtime;
m_time_of_day_f += speed * dtime;
if (m_time_of_day_f > 1.0)
m_time_of_day_f -= 1.0;
if (m_time_of_day_f < 0.0)
Expand Down Expand Up @@ -527,10 +536,10 @@ void ServerEnvironment::loadMeta()
}

try {
m_time_of_day = args.getU64("time_of_day");
setTimeOfDay(args.getU64("time_of_day"));
} catch (SettingNotFoundException &e) {
// This is not as important
m_time_of_day = 9000;
setTimeOfDay(9000);
}
}

Expand Down
24 changes: 10 additions & 14 deletions src/environment.h
Expand Up @@ -93,10 +93,7 @@ class Environment
void setTimeOfDaySpeed(float speed);
float getTimeOfDaySpeed();

void setDayNightRatioOverride(bool enable, u32 value)
{
m_day_night_ratio_override_storage = value | ((u64)enable << 63);
}
void setDayNightRatioOverride(bool enable, u32 value);

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

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

/*
* Below: values managed by m_time_floats_lock
* Below: values managed by m_time_lock
*/
// Time of day in milli-hours (0-23999); determines day and night
u32 m_time_of_day;
// Time of day in 0...1
float m_time_of_day_f;
float m_time_of_day_speed;
// Stores the skew created by the float -> u32 conversion
// to be applied at next conversion, so that there is no real skew.
float m_time_conversion_skew;
// Overriding the day-night ratio is useful for custom sky visuals
bool m_enable_day_night_ratio_override;
u32 m_day_night_ratio_override;
/*
* Above: values managed by m_time_floats_lock
* Above: values managed by m_time_lock
*/

// Overriding the day-night ratio is useful for custom sky visuals
// lowest 32 bits store the overriden ratio, highest bit stores whether its enabled
Atomic<u64> m_day_night_ratio_override_storage;

/* TODO: Add a callback function so these can be updated when a setting
* changes. At this point in time it doesn't matter (e.g. /set
* is documented to change server settings only)
Expand All @@ -137,7 +133,7 @@ class Environment
bool m_cache_enable_shaders;

private:
Mutex m_time_floats_lock;
Mutex m_time_lock;

DISABLE_CLASS_COPY(Environment);
};
Expand Down
64 changes: 44 additions & 20 deletions src/threading/atomic.h
Expand Up @@ -24,19 +24,25 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#if __cplusplus >= 201103L
#include <atomic>
template<typename T> using Atomic = std::atomic<T>;
template<typename T> using GenericAtomic = std::atomic<T>;
#else

#define GCC_VERSION (__GNUC__ * 100 + __GNUC_MINOR__)
#define CLANG_VERSION (__clang_major__ * 100 + __clang_minor__)
#if GCC_VERSION >= 407 || CLANG_VERSION >= 302
#define ATOMIC_LOAD_GENERIC(T, v) do { \
T _val; \
__atomic_load(&(v), &(_val), __ATOMIC_SEQ_CST); \
return _val; \
} while(0)
#define ATOMIC_LOAD(T, v) return __atomic_load_n (&(v), __ATOMIC_SEQ_CST)
#define ATOMIC_STORE(T, v, x) __atomic_store (&(v), &(x), __ATOMIC_SEQ_CST); return x
#define ATOMIC_EXCHANGE(T, v, x) return __atomic_exchange_n(&(v), (x), __ATOMIC_SEQ_CST)
#define ATOMIC_EXCHANGE(T, v, x) return __atomic_exchange (&(v), &(x), __ATOMIC_SEQ_CST)
#define ATOMIC_ADD_EQ(T, v, x) return __atomic_add_fetch (&(v), (x), __ATOMIC_SEQ_CST)
#define ATOMIC_SUB_EQ(T, v, x) return __atomic_sub_fetch (&(v), (x), __ATOMIC_SEQ_CST)
#define ATOMIC_POST_INC(T, v) return __atomic_fetch_add (&(v), 1, __ATOMIC_SEQ_CST)
#define ATOMIC_POST_DEC(T, v) return __atomic_fetch_sub (&(v), 1, __ATOMIC_SEQ_CST)
#define ATOMIC_CAS(T, v, e, d) return __atomic_compare_exchange_n(&(v), &(e), (d), \
#define ATOMIC_CAS(T, v, e, d) return __atomic_compare_exchange(&(v), &(e), &(d), \
false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST)
#else
#define ATOMIC_USE_LOCK
Expand All @@ -56,12 +62,9 @@ with this program; if not, write to the Free Software Foundation, Inc.,
m_mutex.unlock(); \
return _eq; \
} while (0)
#define ATOMIC_LOAD(T, v) \
if (sizeof(T) <= sizeof(void*)) return v; \
else ATOMIC_LOCK_OP(T, v);
#define ATOMIC_STORE(T, v, x) \
if (sizeof(T) <= sizeof(void*)) return v = x; \
else ATOMIC_LOCK_OP(T, v = x);
#define ATOMIC_LOAD(T, v) ATOMIC_LOCK_OP(T, v)
#define ATOMIC_LOAD_GENERIC(T, v) ATOMIC_LOAD(T, v)
#define ATOMIC_STORE(T, v, x) ATOMIC_LOCK_OP(T, v = x)
#define ATOMIC_EXCHANGE(T, v, x) do { \
m_mutex.lock(); \
T _val = v; \
Expand All @@ -84,32 +87,53 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#endif
#endif


// For usage with integral types.
template<typename T>
class Atomic {
public:
Atomic(const T &v = 0) : val(v) {}
Atomic(const T &v = 0) : m_val(v) {}

operator T () { ATOMIC_LOAD(T, val); }
operator T () { ATOMIC_LOAD(T, m_val); }

T exchange(T x) { ATOMIC_EXCHANGE(T, val, x); }
bool compare_exchange_strong(T &expected, T desired) { ATOMIC_CAS(T, val, expected, desired); }
T exchange(T x) { ATOMIC_EXCHANGE(T, m_val, x); }
bool compare_exchange_strong(T &expected, T desired) { ATOMIC_CAS(T, m_val, expected, desired); }

T operator = (T x) { ATOMIC_STORE(T, val, x); }
T operator += (T x) { ATOMIC_ADD_EQ(T, val, x); }
T operator -= (T x) { ATOMIC_SUB_EQ(T, val, x); }
T operator = (T x) { ATOMIC_STORE(T, m_val, x); }
T operator += (T x) { ATOMIC_ADD_EQ(T, m_val, x); }
T operator -= (T x) { ATOMIC_SUB_EQ(T, m_val, x); }
T operator ++ () { return *this += 1; }
T operator -- () { return *this -= 1; }
T operator ++ (int) { ATOMIC_POST_INC(T, val); }
T operator -- (int) { ATOMIC_POST_DEC(T, val); }
T operator ++ (int) { ATOMIC_POST_INC(T, m_val); }
T operator -- (int) { ATOMIC_POST_DEC(T, m_val); }
private:
T val;
T m_val;
#ifdef ATOMIC_USE_LOCK
Mutex m_mutex;
#endif
};

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

operator T () { ATOMIC_LOAD_GENERIC(T, m_val); }

T exchange(T x) { ATOMIC_EXCHANGE(T, m_val, x); }
bool compare_exchange_strong(T &expected, T desired) { ATOMIC_CAS(T, m_val, expected, desired); }

T operator = (T x) { ATOMIC_STORE(T, m_val, x); }
private:
T m_val;
#ifdef ATOMIC_USE_LOCK
Mutex m_mutex;
#endif
};

#endif // C++11

#endif

2 comments on commit 8f03995

@ShadowNinja
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@est31
Copy link
Contributor Author

@est31 est31 commented on 8f03995 Nov 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.