Change RedisConnection::Query::value_type from String to std::variant<const char*,String>

[Al2Klimov]

(Find TL;DR below)

Seriously speaking, 70ca88a pollutes the diff and should be viewed on its own. It just replaces std::vector<String> with RedisConnection::Query. As RedisConnection::Query already was a std::vector<String>, this is a no-op on its own. But it allows to just change RedisConnection::Query itself and affect all its usages at once.

Why do we have to change String to std::variant<const char*,String> at all? (efe5f22)

Especially our history messages contain lots of hardcoded C string literals "like this one". At runtime, they get translated to pointers to constant global memory, const char*. String malloc(3)s and copies these data every time. In contrast, std::variant<const char*,String> just stores the address if any.

TL;DR

• The JSON RPC message processing involves Icinga DB handlers
• The Icinga DB handlers assemble Redis messages, std::vector<T>
• The faster T is, the better for JSON RPC message processing
• The fastest possible T is const char*, if any, which is just a pointer

fixes #10276
ref/NC/820479

[Al2Klimov]

Strange, it worked on my machine...

[julianbrost]

The idea looks fine in general. Besides getting it to work, you might want to take a look at std::string_view. If you implement a conversion to it once, that should allow to avoid other case-distinctions (as the purpose of string_view is to abstract how exactly the backing string is stored).

[Al2Klimov]

Like, class TODO : public std::variant<const char*,String> { operator std::string_view(); };?

[julianbrost]

Probably something like that. A function for it might work too, but a small class could be nicer overall.

[yhabteab]

Same here as well! If there was a corresponding constructor of the String class that takes std::string_view as an argument, you could just use dest.emplace(std::string_view(hMSet[i]), ...) instead. I've played around a bit with this:

No, really, moving a return value is easy

#include <iostream>
#include <variant>
#include <vector>

struct String {
    std::string data;

    String(): data("Orig") {}
    String(std::string_view sv): data(sv.data(), sv.size()) {}
    String(const String& o): data(o.data+" - Copy") {}
    String(String&& o) noexcept : data(o.data+" - Moved") {}

    String operator=(const String& o) {
        return String(o);
    }

    String operator=(String&& o) noexcept {
        return String(o);
    }
    
    operator std::string_view() const {
        return {data.c_str(), data.size()};
    }
};

struct Variant {
    std::variant<std::string_view, String> Data;

    Variant(std::string_view data): Data(data) {}
    Variant(String data): Data(std::move(data)) {}
    Variant(const char* data): Data(std::string_view(data)) {}

    operator std::string_view() const {
        if (std::holds_alternative<String>(Data)) {
            return std::get<String>(Data);
        }

        return std::get<std::string_view>(Data);
    }
};

int main()
{
    std::vector<Variant> variants{Variant("Const Char"), Variant(String{}), Variant(String{}), Variant(String{})};

    for(auto vv : variants)
        printf("%s\n", std::string_view(vv).data());

    std::cout << "\nStart transforming variants to String()\n";

    std::vector<String> strings;
    for(auto& v : variants) {
        strings.emplace_back(std::string_view(v));
    }

    for(auto& str : strings) {
        printf("%s\n", str.data.c_str());
    }
    std::cout << "End transforming variants to String()\n\n";
    
    for(auto vv : variants)
        std::cout << std::string_view(vv) << std::endl;
}

Const Char
Orig - Moved - Copy - Copy
Orig - Moved - Copy - Copy
Orig - Moved - Copy - Copy

Start transforming variants to String()
Const Char - Moved - Moved
Orig - Moved - Copy - Moved
Orig - Moved - Copy
Orig - Moved - Copy
End transforming variants to String()

Const Char
Orig - Moved - Copy - Copy
Orig - Moved - Copy - Copy
Orig - Moved - Copy - Copy

[Al2Klimov]

How does std::string_view conversion move the original value?

[yhabteab]

How does std::string_view conversion move the original value?

It doesn't but why do you still need that conversion at all? We already discussed about this in person but you still didn't changed it, so why? std::variant<...> already provides a proper implementation of comparison operators and you just have to introduce those operator implementations for the new QueryArg class and forward the request to the underlying std::veriant<...> implementation:

bool operator==(const QueryArg& other) const;
bool operator!=(const QueryArg& other) const;
bool operator<(const QueryArg& other) const;
bool operator>(const QueryArg& other) const;

You can then use QueryArg everywhere if it's needed, YES, even as a std::map<> key.

[Al2Klimov]

How does std::string_view conversion move the original value?

It doesn't but why do you still need that conversion at all?

For the statically typed code to compile.

🤷‍♂️

We already discussed about this in person but you still didn't changed it, so why?

Yes, we already discussed in person that you want me to go down this rabbit hole which will improve literally nothing:

dest is part of ourContent which is divided into ourCheckSums and ourObjects. I.e dest is a mapping from a computed id to some computed fancy stuff. Those runtime-assembled Strings haven't even nearly anything to do with "hardcoded C string literals" which I'm handling here.

you just have to introduce those operator implementations for the new QueryArg class and forward the request to the underlying std::veriant<...> implementation:

bool operator==(const QueryArg& other) const;
bool operator!=(const QueryArg& other) const;
bool operator<(const QueryArg& other) const;
bool operator>(const QueryArg& other) const;

These are at least four, for my implementation one function is enough.

You can then use QueryArg everywhere if it's needed, YES, even as a std::map<> key.

But what exactly for?

[yhabteab]

Yes, we already discussed in person that you want me to go down this rabbit hole which will improve literally nothing:

I have absolutely no idea what you're talking about. Which rabbit hole are you referring to? It's just a simple type change, like you did for the other parts. Did you even try it?

I.e dest is a mapping from a computed id to some computed fancy stuff. Those runtime-assembled Strings haven't even nearly anything to do with "hardcoded C string literals" which I'm handling here.

You are literally undoing thousands of objects serialised using QueryArg, simply because you don't want to change the type of ourContent local variable. This PR is about making Icinga DB more efficient, so I don't know why we're still even talking about it. If you're not willing to provide a full implementation, then why open a PR? As I have already pointed out, it's a fairly simple change and doesn't require any tweaking, but for some reason you don't even want to bother trying.

[Al2Klimov]

Yes, we already discussed in person that you want me to go down this rabbit hole which will improve literally nothing:

I have absolutely no idea what you're talking about. Which rabbit hole are you referring to? It's just a simple type change, like you did for the other parts. Did you even try it?

This by itself is indeed a simple type change:

--- a/lib/icingadb/icingadb-objects.cpp
+++ b/lib/icingadb/icingadb-objects.cpp
@@ -378 +378 @@ void IcingaDB::UpdateAllConfigObjects()
-               std::map<String, std::map<String, String>> ourContent;
+               std::map<String, std::map<RedisConnection::QueryArg, RedisConnection::QueryArg>> ourContent;
@@ -394 +394 @@ void IcingaDB::UpdateAllConfigObjects()
-                                               dest.emplace(variantToString(std::move(hMSet[i])), variantToString(std::move(hMSet[i + 1u])));
+                                               dest.emplace(std::move(hMSet[i]), std::move(hMSet[i + 1u]));

But this won't compile, so we'd need some operators, as you already mentioned. My implementation just needs one function.

I.e dest is a mapping from a computed id to some computed fancy stuff. Those runtime-assembled Strings haven't even nearly anything to do with "hardcoded C string literals" which I'm handling here.

You are literally undoing thousands of objects serialised using QueryArg, simply because you don't want to change the type of ourContent local variable.

Absolutely not! QueryArg is simply about distinguishing two things:

• pointers to C strings which are hardcoded in global memory (borrowed)
• C++/Icinga strings which are computed at runtime and malloc(3)ed (owned)

For the owned it doesn't matter at all whether it's a plain String or wrapped in a QueryArg. For the borrowed one it matters of course!

But!

Show me one QueryArg I pass to variantToString() which is borrowed, not owned. Just one!

This PR is about making Icinga DB more efficient, so I don't know why we're still even talking about it.

And, given what I said above, where exactly the efficiency gain?

If you're not willing to provide a full implementation, then why open a PR?

It is a full implementation according to the scope being efficiency.

[yhabteab]

With the above suggestions applied, this should become obsolete.