Attribute refactoring

Here is semi-major attribute refactoring.
I would be happy if someone review code and/or test changes before I push it.
https://github.com/urho3d/Urho3D/pull/2091

  1. Arbitrary attribute metadata was added.

Attribute metadata is per-attribute VariantMap with custom values.
The only way to set metadata is to call SetMetadata on URHO3D_ATTRIBUTE-like macros. SetMetadata calls may be chained.

Metadata may be acquired via AttributeInfo::GetMetadata or AttributeInfo::GetMetadata<T>. It is also exposed to Angel Script.

  1. Offset-based attributes were removed due to unsafety.

URHO3D_ATTRIBUTE and URHO3D_ENUM_ATTRIBUTE are aliased to accesor attributes now. So, offset can’t be used to identify attribute anymore. Use attribute name or metadata. See Constraint for example.

Attribute type hacking will get broken. If you lied with attribute type, code won’t compile. E.g. Light::CascadeParameters::splits_ were float[4] and serialized like Vector4. Don’t lie to your compiler anymore.

  1. Vector structure elements and corresponding defines were removed, @KonstantTom.

Use metadata variable AttributeMetadata::P_VECTOR_STRUCT_ELEMENTS instead. It shall be initialized with StringVector instead of const char*[], no trailing zero is needed.

1 Like

Can you compare the performance difference between these changes, especially the retiring the offset part.

These changes are fine to me. Arbitrary metadata is much more flexible. It looks good for me.

It looks good to me.

The only thing I don’t like is the new line for .SetMetadata but it’s just a matter of personal taste.

Performance is not affected. The test below take the same time with both old and new version (75++5 ms for Debug, 10-+0.5 ms for Release). I didn’t test metadata-related changes because they have very limited area.

Code
#include <Urho3D/Scene/Serializable.h>
#include <Urho3D/IO/VectorBuffer.h>
#include <Urho3D/Core/StringUtils.h>
#include <chrono>

using namespace Urho3D;

#define URHO3D_NEW_ATTRIBUTE(name, typeName, variable, defaultValue, mode) URHO3D_ACCESSOR_ATTRIBUTE_FREE(name, [](const ClassName* classPtr) -> typename AttributeTrait<typeName >::ReturnType { return classPtr->variable; }, [](ClassName* classPtr, typename AttributeTrait<typeName >::ParameterType value) { classPtr->variable = value; }, typeName, defaultValue, mode)
#undef URHO3D_ATTRIBUTE
#define URHO3D_ATTRIBUTE(name, typeName, variable, defaultValue, mode) URHO3D_NEW_ATTRIBUTE(name, typeName, variable, defaultValue, mode)

class TestSerializable : public Serializable
{
   URHO3D_OBJECT(TestSerializable, Serializable);

public:
   TestSerializable(Context* context) : Serializable(context)
   {
      bool_ = !!Random(0, 1);
      int_ = Random(1, 100);
      float_ = Random(1.0f, 100.0f);
      string_ = String(Random(1, 100));
      vector_ = { Random(1, 100), Random(1, 100), Random(1, 100) };
      map_ = { { "1", Random(1, 100) }, { "2", Random(1, 100) }, { "3", Random(1, 100) } };
   }
   static void RegisterObject(Context* context)
   {
      context->RegisterFactory<TestSerializable>();

      URHO3D_ATTRIBUTE("Bool", bool, bool_, false, AM_DEFAULT);
      URHO3D_ATTRIBUTE("Int", int, int_, 0, AM_DEFAULT);
      URHO3D_ATTRIBUTE("Float", float, float_, 0.0f, AM_DEFAULT);
      URHO3D_ATTRIBUTE("String", String, string_, String::EMPTY, AM_DEFAULT);
      URHO3D_ATTRIBUTE("VariantVector", VariantVector, vector_, Variant::emptyVariantVector, AM_DEFAULT);
      URHO3D_ATTRIBUTE("VariantMap", VariantMap, map_, Variant::emptyVariantMap, AM_DEFAULT);
   }
private:
   bool bool_;
   int int_;
   float float_;
   String string_;
   VariantVector vector_;
   VariantMap map_;
};

size_t Test(Context* context)
{
   TestSerializable::RegisterObject(context);
   Vector<SharedPtr<TestSerializable>> objects;
   for (int i = 0; i < 10000; ++i)
      objects.Push(MakeShared<TestSerializable>(context));
   VectorBuffer output;
   output.Resize(1024 * 1024);

   auto t1 = std::chrono::high_resolution_clock::now();
   for (TestSerializable* object : objects)
      object->Save(output);
   auto t2 = std::chrono::high_resolution_clock::now();
   size_t us = (size_t)std::chrono::duration_cast<std::chrono::microseconds>(t2 - t1).count();

   File file(context);
   file.Open("C:/output.bin", FILE_WRITE);
   file.Write(output.GetData(), output.GetSize());
   file.Close();
   return us;
}

The idea is pretty simple here. Have you heard about “single line -
single declaration” rule in C++? Here is the same situation. Metadata is the declaration of the new variable inside the map, so it shall be placed on its own line.
The same is about variadic SendEvent: I’d like to put every event parameter on its own line.

In this test code you’re not really testing the performance of object->Save(), you’re testing the hundreds of memory allocations that are occurring in VectorBuffer, which are comparatively slow to serialization.

I can’t test it for myself now but recommend reserving space on the vector buffer before doing the test and see if it makes any difference.

output.Resize(sizeof(TestSerializable) * 10000);  // Not entirely sure how attributes contribute to the size of the serialized data, so maybe you need to increase this factor

Hundreds of memory allocations take only about 10% of overall time. New test results:

Debug Old: 65…75 ms;
Debug New: 70…76 ms;
Release Old: 8.8…10 ms;
Release New: 9.3…10 ms.

How are the meanings of these results? Does that mean Release Old use 8.8 to 10 ms while Release New use 9.3 to 10 ms?
Also VauleAnimation and ObjectAnimation relies on AttributeAnimation. We may need to test their performance differences before commiting such a fundamental code change as well.

Please come back often and give the valuable opinions. It may take you ten minutes but will save a lot of arguments and point the right direction for this amazing engine. Like the retiring the attribute offset thing, do you think it will affect the overall performance since AttributeAnimation, ValueAnimation and ObjectAnimation are all depending on it?

I don’t understand why do you care about performance at all.

  1. offset attributes is an old hack that should be removed ASAP.
  2. Performance can’t be worse than accessor attributes.
  3. Only 25% of urho attributes are affected.

Debug Old: 65…75 ms;
Debug New: 70…76 ms;
Release Old: 8.8…10 ms;
Release New: 9.3…10 ms.

Judging from this test result, new performance is actually a bit slower, consistently in both debug and release.

You should treat these results as ‘accessor attributes are negligibly slower (~5% in release mode) than offset attributes due to two extra pointer-to-function calls’.
Then, my refactorig is obligatory migration to accessor attributes without breaking API for safety reasons.

I understand fixing Undefined Behavior with the offset usage (just use lambda accessor instead?).

I don’t understand why:

  1. you need attribute meta info
  2. overhead is introduced

I just wanted to add attribute metadata at some point and get rid of kludge named ‘vector structure elements’. When I tried to remove offset attributes, I understood that the point is now. See changes in Constraint.cpp in my PR.

What overhead do you mean?

5% slower in release mode consistently is not negligible for such a core feature because many other features are depending on it, like ValueAnimation, ObjectAnimation, Serialization, Deserialization and Networking. Using offset exists safely for many years and should not be changed just for so called safety at the price of performance. And this change cannot be opt out. I suggest only commit such a change with thorough test and ensure no negative performance introduced at all.

Looked at it a bit, undocumented and looks quite messy. I’m not sure I understand what is it used for (stores names of members encoded into a VariantVector?).
Also it only seems to be used in a single place in the editor, which is also undocumented.

Internally Urho uses URHO3D_ACCESSOR_VARIANT_VECTOR_STRUCTURE_ATTRIBUTE for two classes: StaticModelGroup and SplinePath.

They seem to use VariantVector to store known ID type + size count as the first element, which doesn’t make sense (just use a vector<T> of the known type).
Is it because serialization doesn’t support vector or something?

To me it seems this kludge should be completely removed and replaced with a proper solution.
If meta info suppose to replace it, it still doesn’t solve the underlying kludge that caused it in the first place.

5% slower in release mode consistently is not negligible for such a core feature

So, it’s the problem of accessor attributes introduced six years ago. Accessor attributes were designed to make code more simple and safe despite some performance loss. Do you think it was bad decision? Why nobody cared about it for six years?

should not be changed just for so called safety at the price of performance

Undefined Behavior must not be used. Consider this as axiom.

Think the opposite: there is no reason to use unsafe code unless it is noticeably faster.
Now the difference is too small to care about. Note that random performance fluctuation is much bigger than performace loss. So, performace loss is negligibly small.
image

100 tries, test duration measured in microseconds.

You should label charts.

Also there are benchmarking frameworks for getting statistically sound results.
https://github.com/nickbruun/hayai
https://github.com/DigitalInBlue/Celero
https://github.com/google/benchmark

What do you call ‘proper solution’? Metadata is designed to be something that is not used by the engine core.

stores names of members encoded into a VariantVector?

It stores display names of VariantVector elements.

On the performance of attribute animations, regardless of whether set accessor or offset access is used, the path to getting the value into the intended destination is complex and ugly. If there are 100’s or 1000’s of scene objects which need to animate this way, so that the time taken to apply the animations is significant, then optimizing to an explicit update could be beneficial. Also note the call to ApplyAttributes() which is necessary according to how the attributes work and how they may have late-applied side-effects. In most cases when you know what you’re animating, it should be unnecessary, but the animation system cannot decide that.