Skip to content

IcingaDB: consistently use const char* literals everywhere#10744

Open
yhabteab wants to merge 3 commits intomasterfrom
use-string-view-consistently
Open

IcingaDB: consistently use const char* literals everywhere#10744
yhabteab wants to merge 3 commits intomasterfrom
use-string-view-consistently

Conversation

@yhabteab
Copy link
Member

@yhabteab yhabteab commented Mar 4, 2026

Previously, we used a m_PrefixConfigObject and m_PrefixConfigCheckSum member variables to store the Redis key prefixes for configuration objects and their checksums. So, every time we needed to use these prefixes, we used such constructs as m_PrefixConfigObject + "some:suffix" to create the full Redis key. As a result, we give up the opportunity to just use const char* literals as the resulted Redis keys, since we always convert them to our String type unconditionally. However, after #10391 has been merged, we can now use const char* literals consistently everywhere, unless it's absolutely necessary to use String objects. Yesterday, @julianbrost had a genial idea how we can get rid of the m_PrefixConfigObject and m_PrefixConfigCheckSum member variables and just use const char* literals everywhere, while still keeping similar code readability and maintainability. The idea is to make these prefixes a macro, so that we can use such constructs as CONFIG_REDIS_KEY_PREFIX "host:state" to create the full Redis key, and the macro will expand to the appropriate const char* literal. This PR implements this idea, and I tried my best to reduce the number of places where we need to use String objects to the absolute minimum. You might think that it would be better to store the const char* literals somewhere in a variable now, but I think it's not necessary, as the compiler will optimize the code and deduplicate all the string literals anyway, so we won't have any performance issues apart from being duplicated in the source code.

Just to test, that the compiler/linker does indeed deduplicate the string literals, you can run the following command and grep for whatever string literal you want to check and it should be present only once in the output:

$ strings release/Bin/RelWithDebInfo/icinga2 | grep 'notification:recipient'
icinga:notification:recipient
$ grep -Ri 'notification:recipient' lib/icingadb
lib/icingadb/icingadb-objects.cpp:			CONFIG_REDIS_KEY_PREFIX "notification:recipient",
lib/icingadb/icingadb-objects.cpp:			auto& notificationRecipients (hMSets[CONFIG_REDIS_KEY_PREFIX "notification:recipient"]);
lib/icingadb/icingadb-objects.cpp:						AddObjectDataToRuntimeUpdates(runtimeUpdates, recipientId, CONFIG_REDIS_KEY_PREFIX "notification:recipient", recipientData);
lib/icingadb/icingadb-objects.cpp:		auto& notificationRecipients (hMSets[CONFIG_REDIS_KEY_PREFIX "notification:recipient"]);
lib/icingadb/icingadb-objects.cpp:				AddObjectDataToRuntimeUpdates(runtimeUpdates, id, CONFIG_REDIS_KEY_PREFIX "notification:recipient", data);
lib/icingadb/icingadb-objects.cpp:				AddObjectDataToRuntimeUpdates(runtimeUpdates, id, CONFIG_REDIS_KEY_PREFIX "notification:recipient", groupData);
lib/icingadb/icingadb-objects.cpp:					AddObjectDataToRuntimeUpdates(runtimeUpdates, recipientId, CONFIG_REDIS_KEY_PREFIX "notification:recipient", userData);
lib/icingadb/icingadb-objects.cpp:		DeleteRelationship(id, CONFIG_REDIS_KEY_PREFIX "notification:recipient");
lib/icingadb/icingadb-objects.cpp:		DeleteRelationship(id, CONFIG_REDIS_KEY_PREFIX "notification:recipient");
lib/icingadb/icingadb-objects.cpp:			DeleteRelationship(userId, CONFIG_REDIS_KEY_PREFIX "notification:recipient");
lib/icingadb/icingadb-objects.cpp:				DeleteRelationship(userId, CONFIG_REDIS_KEY_PREFIX "notification:recipient")

ref #10619

@cla-bot cla-bot bot added the cla/signed label Mar 4, 2026
@yhabteab yhabteab requested a review from julianbrost March 4, 2026 11:51
@yhabteab
Copy link
Member Author

yhabteab commented Mar 4, 2026

Re-added the checksum support to the IcingaDB::DeleteState method again as it's required for #10619.

@yhabteab yhabteab force-pushed the use-string-view-consistently branch from ad94743 to 7a1ce8d Compare March 10, 2026 10:03
@yhabteab yhabteab force-pushed the use-string-view-consistently branch from 7a1ce8d to 44fd2a0 Compare March 11, 2026 10:23
@yhabteab yhabteab force-pushed the use-string-view-consistently branch from 44fd2a0 to e103209 Compare March 11, 2026 14:15
@yhabteab yhabteab force-pushed the use-string-view-consistently branch 2 times, most recently from 57fa8bd to 45af026 Compare March 11, 2026 14:34
@yhabteab
Copy link
Member Author

Addressed all open requested changes now!

@yhabteab
Copy link
Member Author

Apparently, not all boost versions we support provide boost::span 😩.

@yhabteab
Copy link
Member Author

Apparently, not all boost versions we support provide boost::span 😩.

Is available since 1.78 only.

Added boost::span, a C++11 implementation of C++20's std::span (Glen Fernandes).

@yhabteab yhabteab force-pushed the use-string-view-consistently branch from 45af026 to 73a892f Compare March 11, 2026 15:23
@yhabteab
Copy link
Member Author

Apparently, not all boost versions we support provide boost::span 😩.

Is available since 1.78 only.

Added boost::span, a C++11 implementation of C++20's std::span (Glen Fernandes).

Changed to std::vector with an open TODO comment that it should be changed to std::span once we upgrade to C++20.

@yhabteab
Copy link
Member Author

Fixed one bug 🙈!

Copy link
Contributor

@jschmidt-icinga jschmidt-icinga left a comment

Choose a reason for hiding this comment

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

Other than the following I think this looks good now.
Shame about boost::span, but it should be fine since the copy doesn't happen often and hopefully we'll be able to move to std::span soon.

@yhabteab yhabteab force-pushed the use-string-view-consistently branch from f4b8620 to b257083 Compare March 12, 2026 12:26
@yhabteab yhabteab added this to the 2.16.0 milestone Mar 12, 2026
@yhabteab yhabteab added the area/icingadb New backend label Mar 12, 2026
@yhabteab yhabteab force-pushed the use-string-view-consistently branch 2 times, most recently from fd588d8 to 09d5ed0 Compare March 12, 2026 16:43
@julianbrost
Copy link
Member

#10744 (review) was just the result of a quick look over the new code if I quickly spot something that can be improved. That has been addressed though I didn't do a full review. @jschmidt-icinga As you've reviewed the full PR before, can you please take another look at it? If you're happy with the latest changes as well, then it's fine for me.

@yhabteab yhabteab force-pushed the use-string-view-consistently branch from 09d5ed0 to 2fbf083 Compare March 13, 2026 15:17
@yhabteab
Copy link
Member Author

Rebased to make use of #10756, so I can pass l_SyncableTypes directly to ParallelFor and dropped the m_IndexedTypes set.

@yhabteab yhabteab force-pushed the use-string-view-consistently branch from 2fbf083 to fa514e6 Compare March 16, 2026 07:39
@yhabteab
Copy link
Member Author

Just inlined the now superfluous GetTypeDumpSignalKeys function and removed it entirely.

Copy link
Contributor

@jschmidt-icinga jschmidt-icinga left a comment

Choose a reason for hiding this comment

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

I gave this another quick look and I think it's fine. There are no functional changes except what we talked about in GetTypeOverwriteKeys() and what was previously GetTypeDumpSignalKeys() and the rest is a straight forward refactoring to use the string literals instead of runtime strings.

There is a little remaining worry about using the linear-time lookups in actually hot code, but I think it's at least not going to be slower than if/else comparisons to the const Type::Ptr& and probably not slower than the std::unordered_set of the actual Type::Ptr, which doesn't have the additional dereference, but adds the hash function (and the set's binary search probably isn't faster than linear search at these small data-sizes either). It would have been nice if the lookup could've been constant time, but there's no way to do that with the way our reflection types work currently.

@yhabteab
Copy link
Member Author

If you're happy with the latest changes as well, then it's fine for me.

@julianbrost can this now be merged or do you want to have a final quick look at it again? Otherwise, please merge it! Ty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants