Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anim<T>::operator const T& is a potential performance hazard #1806

Closed
axjxwright opened this Issue Apr 5, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@axjxwright
Copy link
Contributor

axjxwright commented Apr 5, 2017

I just ran into an issue where not using the call operator on an Anim<T> brought my application to a complete halt (Literally the difference between starting to lag at 5000 particles before and 100,000 particles after the fix) The Pertinent information is:

struct Particle
{
    vec3        Position;
    Anim<float> Size;
}

void App::update ( )
{
    auto m = _positionData->mapAttrib4f( geom::POSITION );
    
    for ( auto& p : _particles )
    {
        *m++ = vec4 ( f.Position, f.Size );
        // *m++ = vec4 ( f.Position, f.Size() );
        // --------------------------------^ Performance problems, be gone!
    }
    
    m.unmap();
}

I suppose this could just as easily be considered poor programming on my side by not correctly managing the copy semantics / lifetimes of my objects, but i feel like it's a very simple mistake to make in what is fairly idiomatic usage of Anim<T>, similar to the whole glm::length(vec) vs vec.length() issue when glm was first introduced.

@andrewfb

This comment has been minimized.

Copy link
Collaborator

andrewfb commented Apr 5, 2017

Does this make sense to you? I haven't dug into it yet, but I believe operator() and the cast operator are roughly equivalent, or am I missing something? Is it the same behavior in both Debug and Release?

@axjxwright

This comment has been minimized.

Copy link
Contributor Author

axjxwright commented Apr 5, 2017

image

pushData() is equivalent to update() in the sample code i provided.

I only profiled in release, but i suspect that would be the best case. If you say that those two are roughly the same, I wonder if this is more to do with the vec4 constructor? As you can see it's triggering a copy of the Anim so a heap of time is spent in its destructor. If that's the case i guess it's outside the scope of cinder.

@andrewfb

This comment has been minimized.

Copy link
Collaborator

andrewfb commented Apr 5, 2017

Well, it's definitely a bit mysterious to me. operator() code versus the cast operator code. Seem similar, but clearly something is different. Perhaps the compiler is not inlining one of them?

@axjxwright

This comment has been minimized.

Copy link
Contributor Author

axjxwright commented Apr 5, 2017

Just changed the way i constructed the vec4, allowing the cast operator to be invoked and this doesn't have the same performance problems, So i guess that the vec4 constructor is taking a copy of the a Anim before attempting to convert it.

vec4 v = vec4 ( f.Position, 0 );
v.w = f.Size;
*m++ = v;

Sorry to waste your time.

@andrewfb

This comment has been minimized.

Copy link
Collaborator

andrewfb commented Apr 5, 2017

Well, interesting problem - never seen that before, and not sure I would have suspected the culprit. Will mark this closed though.

@andrewfb andrewfb closed this Apr 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.