Namespace: don't acquire shared locks on frozen namespaces

This makes freezing a namespace an irrevocable operation but in return allows
omitting further lock operations. This results in a performance improvement as
reading an atomic bool is faster than acquiring and releasing a shared lock.

ObjectLocks on namespaces remain untouched as these mostly affect write
operations which there should be none of after freezing (if there are some,
they will throw exceptions anyways).

lib/base/function.hpp

@@ -60,28 +60,28 @@ class Function final : public ObjectImpl<Function>
 	INITIALIZE_ONCE_WITH_PRIORITY([]() { \
 		Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), false); \
 		Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \
-		nsp->Set(#name, sf, true, true); \
+		nsp->Set(#name, sf, true); \
 	}, InitializePriority::RegisterFunctions)
 
 #define REGISTER_SAFE_FUNCTION(ns, name, callback, args) \
 	INITIALIZE_ONCE_WITH_PRIORITY([]() { \
 		Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), true); \
 		Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \
-		nsp->Set(#name, sf, true, true); \
+		nsp->Set(#name, sf, true); \
 	}, InitializePriority::RegisterFunctions)
 
 #define REGISTER_FUNCTION_NONCONST(ns, name, callback, args) \
 	INITIALIZE_ONCE_WITH_PRIORITY([]() { \
 		Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), false); \
 		Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \
-		nsp->Set(#name, sf, false, true); \
+		nsp->Set(#name, sf, false); \
 	}, InitializePriority::RegisterFunctions)
 
 #define REGISTER_SAFE_FUNCTION_NONCONST(ns, name, callback, args) \
 	INITIALIZE_ONCE_WITH_PRIORITY([]() { \
 		Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), true); \
 		Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \
-		nsp->SetAttribute(#name, sf, false, true); \
+		nsp->SetAttribute(#name, sf, false); \
 	}, InitializePriority::RegisterFunctions)
 
 }

lib/base/json-script.cpp

@@ -24,5 +24,5 @@ INITIALIZE_ONCE([]() {
 	jsonNS->Freeze();
 
 	Namespace::Ptr systemNS = ScriptGlobal::Get("System");
-	systemNS->Set("Json", jsonNS, true, true);
+	systemNS->Set("Json", jsonNS, true);
 });

lib/base/math-script.cpp

@@ -180,5 +180,5 @@ INITIALIZE_ONCE([]() {
 	mathNS->Freeze();
 
 	Namespace::Ptr systemNS = ScriptGlobal::Get("System");
-	systemNS->Set("Math", mathNS, true, true);
+	systemNS->Set("Math", mathNS, true);
 });

lib/base/namespace.cpp

@@ -33,7 +33,7 @@ Value Namespace::Get(const String& field) const
 
 bool Namespace::Get(const String& field, Value *value) const
 {
-	std::shared_lock<decltype(m_DataMutex)> lock(m_DataMutex);
+	auto lock(ReadLockUnlessFrozen());
 
 	auto nsVal = m_Data.find(field);
 
@@ -45,21 +45,22 @@ bool Namespace::Get(const String& field, Value *value) const
 	return true;
 }
 
-void Namespace::Set(const String& field, const Value& value, bool isConst, bool overrideFrozen, const DebugInfo& debugInfo)
+void Namespace::Set(const String& field, const Value& value, bool isConst, const DebugInfo& debugInfo)
 {
 	ObjectLock olock(this);
+
+	if (m_Frozen) {
+		BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified.", debugInfo));
+	}
+
 	std::unique_lock<decltype(m_DataMutex)> dlock (m_DataMutex);
 
 	auto nsVal = m_Data.find(field);
 
 	if (nsVal == m_Data.end()) {
-		if (m_Frozen && !overrideFrozen) {
-			BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified.", debugInfo));
-		}
-
 		m_Data[field] = NamespaceValue{value, isConst};
 	} else {
-		if (nsVal->second.Const && !overrideFrozen) {
+		if (nsVal->second.Const) {
 			BOOST_THROW_EXCEPTION(ScriptError("Constant must not be modified.", debugInfo));
 		}
 
@@ -74,46 +75,43 @@ void Namespace::Set(const String& field, const Value& value, bool isConst, bool
  */
 size_t Namespace::GetLength() const
 {
-	std::shared_lock<decltype(m_DataMutex)> lock(m_DataMutex);
+	auto lock(ReadLockUnlessFrozen());
 
 	return m_Data.size();
 }
 
 bool Namespace::Contains(const String& field) const
 {
-	std::shared_lock<decltype(m_DataMutex)> lock(m_DataMutex);
+	auto lock (ReadLockUnlessFrozen());
 
 	return m_Data.find(field) != m_Data.end();
 }
 
-void Namespace::Remove(const String& field, bool overrideFrozen)
+void Namespace::Remove(const String& field)
 {
 	ObjectLock olock(this);
 
-	if (m_Frozen && !overrideFrozen) {
+	if (m_Frozen) {
 		BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified."));
 	}
 
 	std::unique_lock<decltype(m_DataMutex)> dlock (m_DataMutex);
 
-	if (!overrideFrozen) {
-		auto attr = m_Data.find(field);
-
-		if (attr != m_Data.end() && attr->second.Const) {
-			BOOST_THROW_EXCEPTION(ScriptError("Constants must not be removed."));
-		}
-	}
-
 	auto it = m_Data.find(field);
 
-	if (it == m_Data.end())
+	if (it == m_Data.end()) {
 		return;
+	}
+
+	if (it->second.Const) {
+		BOOST_THROW_EXCEPTION(ScriptError("Constants must not be removed."));
+	}
 
 	m_Data.erase(it);
 }
 
 /**
- * Freeze the namespace, preventing further updates unless overrideFrozen is set.
+ * Freeze the namespace, preventing further updates.
  *
  * This only prevents inserting, replacing or deleting values from the namespace. This operation has no effect on
  * objects referenced by the values, these remain mutable if they were before.
@@ -124,9 +122,18 @@ void Namespace::Freeze() {
 	m_Frozen = true;
 }
 
+std::shared_lock<std::shared_timed_mutex> Namespace::ReadLockUnlessFrozen() const
+{
+	if (m_Frozen.load(std::memory_order_relaxed)) {
+		return std::shared_lock<std::shared_timed_mutex>();
+	} else {
+		return std::shared_lock<std::shared_timed_mutex>(m_DataMutex);
+	}
+}
+
 Value Namespace::GetFieldByName(const String& field, bool, const DebugInfo& debugInfo) const
 {
-	std::shared_lock<decltype(m_DataMutex)> lock(m_DataMutex);
+	auto lock (ReadLockUnlessFrozen());
 
 	auto nsVal = m_Data.find(field);
 
@@ -138,7 +145,12 @@ Value Namespace::GetFieldByName(const String& field, bool, const DebugInfo& debu
 
 void Namespace::SetFieldByName(const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo)
 {
-	Set(field, value, false, overrideFrozen, debugInfo);
+	// The override frozen parameter is mandated by the interface but ignored here. If the namespace is frozen, this
+	// disables locking for read operations, so it must not be modified again to ensure the consistency of the internal
+	// data structures.
+	(void) overrideFrozen;
+
+	Set(field, value, false, debugInfo);
 }
 
 bool Namespace::HasOwnField(const String& field) const

lib/base/namespace.hpp

@@ -8,6 +8,7 @@
 #include "base/shared-object.hpp"
 #include "base/value.hpp"
 #include "base/debuginfo.hpp"
+#include <atomic>
 #include <map>
 #include <vector>
 #include <memory>
@@ -68,9 +69,9 @@ class Namespace final : public Object
 
 	Value Get(const String& field) const;
 	bool Get(const String& field, Value *value) const;
-	void Set(const String& field, const Value& value, bool isConst = false, bool overrideFrozen = false, const DebugInfo& debugInfo = DebugInfo());
+	void Set(const String& field, const Value& value, bool isConst = false, const DebugInfo& debugInfo = DebugInfo());
 	bool Contains(const String& field) const;
-	void Remove(const String& field, bool overrideFrozen = false);
+	void Remove(const String& field);
 	void Freeze();
 
 	Iterator Begin();
@@ -86,10 +87,12 @@ class Namespace final : public Object
 	static Object::Ptr GetPrototype();
 
 private:
+	std::shared_lock<std::shared_timed_mutex> ReadLockUnlessFrozen() const;
+
 	std::map<String, NamespaceValue> m_Data;
 	mutable std::shared_timed_mutex m_DataMutex;
 	bool m_ConstValues;
-	bool m_Frozen;
+	std::atomic<bool> m_Frozen;
 };
 
 Namespace::Iterator begin(const Namespace::Ptr& x);

lib/base/scriptframe.cpp

@@ -29,18 +29,18 @@ INITIALIZE_ONCE_WITH_PRIORITY([]() {
 	l_SystemNS->Set("BuildHostName", ICINGA_BUILD_HOST_NAME);
 	l_SystemNS->Set("BuildCompilerName", ICINGA_BUILD_COMPILER_NAME);
 	l_SystemNS->Set("BuildCompilerVersion", ICINGA_BUILD_COMPILER_VERSION);
-	globalNS->Set("System", l_SystemNS, false, true);
+	globalNS->Set("System", l_SystemNS, true);
 
-	l_SystemNS->Set("Configuration", new Configuration(), false, true);
+	l_SystemNS->Set("Configuration", new Configuration());
 
 	l_TypesNS = new Namespace(true);
-	globalNS->Set("Types", l_TypesNS, false, true);
+	globalNS->Set("Types", l_TypesNS, true);
 
 	l_StatsNS = new Namespace(true);
-	globalNS->Set("StatsFunctions", l_StatsNS, false, true);
+	globalNS->Set("StatsFunctions", l_StatsNS, true);
 
 	l_InternalNS = new Namespace(true);
-	globalNS->Set("Internal", l_InternalNS, false, true);
+	globalNS->Set("Internal", l_InternalNS, true);
 }, InitializePriority::CreateNamespaces);
 
 INITIALIZE_ONCE_WITH_PRIORITY([]() {

lib/icinga/icingaapplication.cpp

@@ -54,8 +54,8 @@ void IcingaApplication::StaticInitialize()
 	/* Ensure that the System namespace is already initialized. Otherwise this is a programming error. */
 	VERIFY(systemNS);
 
-	systemNS->Set("ApplicationType", "IcingaApplication", false, true);
-	systemNS->Set("ApplicationVersion", Application::GetAppVersion(), false, true);
+	systemNS->Set("ApplicationType", "IcingaApplication", true);
+	systemNS->Set("ApplicationVersion", Application::GetAppVersion(), true);
 
 	Namespace::Ptr globalNS = ScriptGlobal::GetGlobals();
 	VERIFY(globalNS);