CAttributes is a cpu hog

[hendu]

The profiling of my sphere culling showed irr::io::CAttributes::findAttribute taking 7.6% of the app's cpu time. Just FYI

[CuteAlien]

Isn't that usually used just in serialization?

[hendu]

No, the scene manager uses it on every frame.

[CuteAlien]

I see... a debug-feature for counting culled objects in debug. With a define hidden inside a source-file and unfortunately _not_ only enabled in debug. And as I never knew about it Irrlicht 1.8 got released with that define enabled. Way to waste speed *sigh*.

Well, at least Irrlicht 1.8.1 can be a lot faster then. Thanks for that find!

Now the only questions left is how to fix this best... probably disabling by default when checked in (as it's probably meant for debugging) or putting some #ifdef DEBUG around.

[hendu]

I think some of the examples rely on that to report stats.

Also there are many more attribute calls than those surrounded by the ifdef. You see, setAttribute also calls findAttribute inside, and it's a linear search every time.

[CuteAlien]

This setAttribute shouldn't call a findAttribute (it's the index version). I'll check the examples, good you mentioned that.

[hendu]

The ifdef'ed ones do not, but all the others do:

Code: Select all

grep setAttribute CSceneManager.cpp
        Parameters.setAttribute( DEBUG_NORMAL_LENGTH, 1.f );
        Parameters.setAttribute( DEBUG_NORMAL_COLOR, video::SColor(255, 34, 221, 221));
        Parameters.setAttribute ( index, Parameters.getAttributeAsInt ( index ) + 1 );
                Parameters.setAttribute ( index, Parameters.getAttributeAsInt ( index ) + 1 );
        Parameters.setAttribute ( "culled", 0 );
        Parameters.setAttribute ( "calls", 0 );
        Parameters.setAttribute ( "drawn_solid", 0 );
        Parameters.setAttribute ( "drawn_transparent", 0 );
        Parameters.setAttribute ( "drawn_transparent_effect", 0 );
                Parameters.setAttribute ( "drawn_solid", (s32) SolidNodeList.size() );
                Parameters.setAttribute ( "drawn_transparent", (s32) TransparentNodeList.size() );
                Parameters.setAttribute ( "drawn_transparent_effect", (s32) TransparentEffectNodeList.size() );
        getParameters()->setAttribute(COLLADA_CREATE_SCENE_INSTANCES, false);
        getParameters()->setAttribute(COLLADA_CREATE_SCENE_INSTANCES, oldColladaSingleMesh);

Code: Select all

//! Sets a attribute as integer value
void CAttributes::setAttribute(const c8* attributeName, s32 value)
{
        IAttribute* att = getAttributeP(attributeName);
        if (att)
                att->setInt(value);
        else
        {
                Attributes.push_back(new CIntAttribute(attributeName, value));
        }
}
 

getAttributeP is a linear search. Also some of the set* methods duplicate the search code instead of using getAttributeP.

[hendu]

This thing should be cleaned up and made to use a rb tree, it's too volatile to be used with persistent indices.

With the tree the if(exists) checks would also be gone, for calling insert(key, value) would do that inside it - if it exists, replace the value, if not, add it.

[CuteAlien]

Ok, sorry, I only took a very quick look on this yesterday while still at work. And once in a while I mess up my search inside VS by hitting a key which switches from "search in document" to "search in selection" - which probably happened when I was looking for this, because I only did see the first setAttribute *sigh*. My fault.

So yeah, there are certainly more. And as far as I can see there is no reason not to put them all inside the define. But there also seems to be no reason I can see to even use "attributes" for them instead of a normal c++ variable with type. Except that attributes are (as any typeless programming) quicker to create for a quick test. But using a bunch of string-searches inside drawAll makes no sense to me. On first view this all looks a lot to me like it was added for some quick test and then forgotten about. Maybe Hybrid knows more about this.

If it has to stay my solution would be adding a struct SSceneManagerDebugInfo which contains all those infos. And then rename SCENEMANAGER_DEBUG to _IRR_SCENEMANAGER_DEBUG_INFO and put it into IrrCompileConfig where it would be enabled by default when DEBUG is set and otherwise disabled. And _IRR_SCENEMANAGER_DEBUG_INFO should be put around all those lines. And then all those stuff could use SSceneManagerDebugInfo.

Alternatively this could be considered as constantly useful info. As soon as it's no longer using attributes it would be fast enough that it doesn't matter anyway. So instead of SSceneManagerDebugInfo it would be for example struct SSceneManagerDrawInfo and just some info which is always written.

There's no point in optimizing attributes for this as I don't think attributes are thought to be used that way anyway. Also really not sure if using a map would optimize it for such small arrays - would have to run speed-tests - and anyway that would break the public attribute interface as a lot of places now _do_ work with indices which then would no longer be possible.

[hendu]

The indice interfaces are not much use anyway since the indices can change when things are removed from it.

[CuteAlien]

Well, maybe not much used in Irrlicht (not sure). But it's a public interface... so people can use it in their project. And I know that at least I do use indices (they are often faster and you rarely remove attributes), so I rather suppose other people use them as well. Breaking interfaces is always bad and here it would still be slow and not fixing the real problem - which is imho that attributes shouldn't be used for this at all. But let's wait until Hybrid has had a chance to look at this before fixing anything, because maybe I'm missing the real reason why they are used here.

[Mel]

Is it posible to disable this without having to modify the engine (or at least, not very extensively)

[hendu]

Nope, would require checking all places that try to use the scene manager's parameters, wouldn't be enough just to remove those lines from the scene manager itself.

[Mel]

I have been taking a look at it, and as far as i have checked, doesn't make much sense, at least to me, to create and destroy constantly some variables, seems that whenever you set an attribute to 0, it is erased, and later, whenever it is given a value again, it is added.

The attributes have an index, Why not making use of them?, that is, create the useful attributes on manager creation first, the ones that could be considered static, and use indices to access them. This still keeps the posibility to access the attributes for other parts as always, but should relieve a bit the CPU usage of the attributes list during the drawAll call.

[hendu]

Using proper types as cutealien said would be much better.