C++ inplace_optional_holder optimization opportunities

[AntonSulimenkoHarman]

c++, enhancement, based on zserio 2.12.0

1. Destructor might not reset the m_hasValue flag because the destructed object should no longer be used.

https://github.com/ndsev/zserio/blob/fe74dfc5c3a976901a1953696d390591b174a358/compiler/extensions/cpp/runtime/src/zserio/OptionalHolder.h#L790

~inplace_optional_holder() {
  reset();
}

change to:

~inplace_optional_holder() {
  if (hasValue()){
    m_storage.getObject()->~T();
  }
}

ref: https://github.com/llvm/llvm-project/blob/dd0973be58b8a8d94d63762941f741f2b93aec28/libcxx/include/optional#L304

2. [1] is a general approach, but for trivially_destructible objects inplace_optional_holder's destructor should also be trivial.

ref uses default (empty and trivial) destructor: https://github.com/llvm/llvm-project/blob/dd0973be58b8a8d94d63762941f741f2b93aec28/libcxx/include/optional#L340

2.1 For trivially_destructible objects, move actions might not reset m_hasValue for other (since the following destructor does nothing)

https://github.com/ndsev/zserio/blob/fe74dfc5c3a976901a1953696d390591b174a358/compiler/extensions/cpp/runtime/src/zserio/OptionalHolder.h#L854

if (this != &other) {
  reset();
  if (other.hasValue()) {
    new (m_storage.getStorage()) T(std::move(*other.m_storage.getObject()));
    other.m_hasValue = false; <<<< might be omitted
    m_hasValue = true;
  }
}
return *this;

3. assignment

https://github.com/ndsev/zserio/blob/fe74dfc5c3a976901a1953696d390591b174a358/compiler/extensions/cpp/runtime/src/zserio/OptionalHolder.h#L800

inplace_optional_holder& operator=(const inplace_optional_holder& other) {
  if (this != &other) {
    reset();
    if (other.hasValue()) {
      new (m_storage.getStorage()) T(*other.m_storage.getObject());
      m_hasValue = true;
    }
  }
  return *this;
}

ref: https://github.com/llvm/llvm-project/blob/dd0973be58b8a8d94d63762941f741f2b93aec28/libcxx/include/optional#L528

3.1. The check for self-assignment is unnecessary.

3.2. (this.hasValue && other.hasValue) reset() should not be called. Call a copy/move assignment operator instead.

3.3. (!this.hasValue && other.hasValue) reset() should not be called. Call only constructor.

3.4. (!this.hasValue && !other.hasValue) reset() should not be called. Do nothing.

4. Inconsistent copy/move constructors

copy ctor: https://github.com/ndsev/zserio/blob/fe74dfc5c3a976901a1953696d390591b174a358/compiler/extensions/cpp/runtime/src/zserio/OptionalHolder.h#L718 move assignment (but should be move ctor. right?): https://github.com/ndsev/zserio/blob/fe74dfc5c3a976901a1953696d390591b174a358/compiler/extensions/cpp/runtime/src/zserio/OptionalHolder.h#L749

5. is_trivially_copy_constructible, is_trivially_move_constructible ... might give next optimization opportunities. e.g.:

__optional_copy_base https://github.com/llvm/llvm-project/blob/dd0973be58b8a8d94d63762941f741f2b93aec28/libcxx/include/optional#L544C21-L544C21 __optional_move_base https://github.com/llvm/llvm-project/blob/dd0973be58b8a8d94d63762941f741f2b93aec28/libcxx/include/optional#L571 __optional_copy_assign_base https://github.com/llvm/llvm-project/blob/dd0973be58b8a8d94d63762941f741f2b93aec28/libcxx/include/optional#L605 __optional_move_assign_base https://github.com/llvm/llvm-project/blob/dd0973be58b8a8d94d63762941f741f2b93aec28/libcxx/include/optional#L637

[!NOTE] Disclaimer: different optional implementations (like boost, MSVC, LLVM etc) have different approaches, but I use LLVM as a reference.

Motivation for these changes: we observe tons of optional destructors in the profiler, but stored types are trivially_destructible.

[mikir]

Thanks a lot for a comprehensive description of the problem. We will investigate it and come back to you.