Improvements on HJson creation

[lawnighthawk]

This change adds functions for insertion into the map and push_back into vector that use r-value references, allowing for functions that build HJson::Value recursively to skip allocations and copies.

[trobro]

std::map::insert_or_assign() was introduced in C++17 but hjson-cpp only requires C++11. I'm reluctant to drop support for C++11.

[lawnighthawk]

Could this function be gated behind a #ifdef ?  — Lawrence On Dec 17, 2023, at 07:42, trobro ***@***.***> wrote: std::map::insert_or_assign() was introduced in C++17 but hjson-cpp only requires C++11. I'm reluctant to drop support for C++11. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

[lawnighthawk]

@trobro There is a new version now that has #define guards

[trobro]

Having c++ version guards in the implementation is fine, but having c++ version guards in the header can cause confusing problems for users. This project compiles into a library that users might install on their system. But perhaps they compile hjson-cpp as c++11, and then at some point in the future try to link it from an application built as c++17. The header seen by the c++17 appliction would then not represent the content of the hjson-cpp lib, there would be functions declared in the header that are not present in the lib.

I'm wondering if the performance gains in this PR are worth the added complexity? Have you benchmarked it?

If the performance gains are worth it, perhaps the move can be implemented without insert_or_assign()?

[trobro]

What would cause std::out_of_range to be thrown, and why should it be ignored?

[lawnighthawk]

You are right in that it does not make a lot of sense, maybe just not have a catch and throw whatever exception came from the container?

[trobro]

Yes better to not have a catch.

[trobro]

The implementation seems to support type Undefined too, not just type Map.

[lawnighthawk]

true, this implementation follows a similar scheme as to that of operator [] , where if the Value is of type undefined, we assume the user wanted map and thus we convert it to a map type and then treat it as a map insertion.

[lawnighthawk]

@trobro I have tried to build a benchmark showing the value of doing this, but with the latest master the benchmark showed 10% improvement in some cases, none in others. This might not be enough to justify the headaches, I'll take more time to dig deeper. The original use case for this was building jsons on the fly as a way to have a data structure to share variable field information between functions/processes. Thus the speed of json creation was important, and the jsons were many and large.

[trobro]

A consistent 10 % improvement of performance could justify adding complexity. But I noticed now one operation missing from your implementation: adding new keys to the vector that keeps track of the insertion order in the map.

Perhaps you can change your implementation into this before you benchmark:

void Value::insert_or_assign(const std::string& key, Value&& other) {
  if (prv->type == Type::Undefined) {
    prv->~ValueImpl();
    // Recreate the private object using the same memory block.
    new(&(*prv)) ValueImpl(Type::Map);
  } else if (prv->type != Type::Map) {
    throw type_mismatch("Must be of type Undefined or Map for that operation.");
  }

  auto it = prv->m->m.find(name);
  if (it == prv->m->m.end()) {
    // If the key is new we must add it to the order vector also.
    prv->m->v.push_back(key);

    prv->m->m.emplace(key, std::move(other));
  } else {
    it.second = std::move(other);
  }
}