Replace dynamic_cast's in MWClass

[ghost]

I'm trying to eliminate per-frame dynamic_cast's and this was one of the hotspots. Almost 1% of the frame loop is spent in Class::getCreatureStats, Class::getContainerStore and similar functions.

Replacing with a static_cast can be deemed safe since only the MWClass sets the custom data, and it knows which custom data it's set. The only issue is if that assumption were to be violated for some reason, we would get a hard crash instead of a bad cast exception (though exceptions are likely to abort the save process, render the game unplayable or result in other issues anyway if they're not caught properly).

If static_cast's are a no go, my next best suggestion would be to hack a fast version of dynamic_cast using virtual functions:

namespace MWClass
{
    class CreatureCustomData;
}

namespace MWWorld
{

    /// \brief Base class for the MW-class-specific part of RefData
    class CustomData
    {
        public:

            virtual ~CustomData() {}

            /// Would be overridden in CreatureCustomData to return this;
            virtual CreatureCustomData* asCreatureCustomData() { return 0; }

            virtual CustomData *clone() const = 0;
    };

[zinnschlag]

I am conflicted about this. The dynamic_casts are useful in my opinion. In most places we are catching exceptions properly, so the game should not end up in an unplayable state.

Do we have any data on how much this changes improves performance? You mentioned a value of 1%. I presume that this is with dynamic_cast. What would it be with static_cast instead?

[ghost]

In most places we are catching exceptions properly, so the game should not end up in an unplayable state.

Exceptions thrown during the frame update are difficult to handle. What usually happens is the try ... catch in the frame loop takes effect, aborting the frame in a half-complete state. Since nothing was done to mitigate the issue, the next frame will likely throw the same exception. Often this results in the controls no longer working and the framerate taking a bath.

Do we have any data on how much this changes improves performance?

The overhead of dynamic_cast's called by these functions was 0.8%. Since the static_cast's are compile time the new figure is 0%. This figure was obtained standing next to the silt strider in Balmora and may be higher in AI-heavier scenes.

I feel like get... functions ought to be dead-cheap. Even simple checks like "is this actor dead?" currently incur a dynamic_cast because they rely on the CreatureStats cast. It's dangerous to cache the CreatureStats& or ContainerStore& objects on the caller side since the pointers would go stale when the object changes cells.

[zinnschlag]

The overhead of dynamic_cast's called by these functions was 0.8%. Since the static_cast's are compile time the new figure is 0%. This figure was obtained standing next to the silt strider in Balmora and may be higher in AI-heavier scenes.

Ouch!

Regarding exceptions in the main loop: At least in some places we have try catch clauses that allow the main loop to continue (e.g. script execution). And of course just because a function that threw an exception was called in one frame doesn't mean necessarily that it will be called in the next frame too.
It is important that the main loop deals with exceptions as gracefully as possible, because exceptions are our main way of dealing with errors. We may need to improve the main loop a bit in this regard, though I am actually not aware of any recent reports of the game becoming unplayable because of main loop exceptions.

Anyway, but to topic: Your alternative proposal sounds reasonable. But I would throw an exception instead of returning 0 in the base class. That will save us from having to check for 0 in the get functions.

[ghost]

though I am actually not aware of any recent reports of the game becoming unplayable because of main loop exceptions.

https://bugs.openmw.org/issues/2959

[zinnschlag]

Interesting. Do you happened to have a stack backtrace of this exception? I am wondering in what part of the main loop this is triggered. Maybe we should look into toughening up our main loop implementation.