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

Performance regression in mj_copyData (2.3.0 vs 2.1.0 ) #568

Closed
ThoenigAdrian opened this issue Nov 8, 2022 · 7 comments
Closed

Performance regression in mj_copyData (2.3.0 vs 2.1.0 ) #568

ThoenigAdrian opened this issue Nov 8, 2022 · 7 comments
Labels
bug Something isn't working

Comments

@ThoenigAdrian
Copy link

ThoenigAdrian commented Nov 8, 2022

Hi,

I recently migrated a mujoco project from mujoco version 2.1.0 to 2.3.0. I discovered that my application is running way slower. I ran a performance profiler to see why.

image

image

Turns out mj_copyData is way slower than before.
For those who are wondering about my motivations and why I'm doing this: I am copying the data, because i have 2 threads one in which i run the simulation and the main thread where i render/visualize.

Reproduce: I can provide a minimal working example if needed. However in this case I think it should be very easy to reproduce and compare. But if it isn't, I can provide something just let me know.

  • Include the following context:
    • Windows 10 x64
    • MuJoCo version 2.3.0 vs MuJoCo version 2.1.0
@ThoenigAdrian ThoenigAdrian added the bug Something isn't working label Nov 8, 2022
@yuvaltassa
Copy link
Collaborator

Thanks, we will look into this.

In order to help us diagnose, could you please print mjData.nstack and mjData.nbuffer in both cases?

However, you should know that mj_copyData is not the recommended way to duplicate an mjData. This is mentioned here. As explained here, MuJoCo has a well-defined state. You shuld just be copying over the state. In your case, for visualisation purposes, it is enough to copy qpos and then mj_fwdPosition, or if you want to visualise forces then copy the entire integration state and call mj_forward. I expect this will be faster than mj_copyData. If you still want to use mj_copyData, perhaps for convenience, you can make it faster by reducing allocated memory. The latest version of simulate (not in any release, at head) reports the maximum amount of memory actually used. You reduce memory by using the <size memory/> directive. Look at 5004a4c for examples.

@ThoenigAdrian
Copy link
Author

ThoenigAdrian commented Nov 8, 2022

Hi Yuval,

thanks for looking into this and also thx for pointing out the more efficient way.

Diagnostic information:

mujoco version 2.1.0:

nbuffer: 5052592
nstack: 2062480

mujoco version 2.3.0:

nbuffer: 285568
nstack: 2663290

@saran-t
Copy link
Member

saran-t commented Nov 9, 2022

@ThoenigAdrian I find it rather suspicious that mj_step is 3x faster now compared to 2.1.0. I'm not aware of anything that we've done that could've caused it to run quite that much faster.

Are your numbers reliably reproducible? Would you mind running it against a few different versions in between?

@ThoenigAdrian
Copy link
Author

ThoenigAdrian commented Nov 9, 2022

@saran-t
Yes it's reliably reproducible.

I can understand your suspicion. I thought the same in the beginning.
But this is just because mj_copyData takes longer therefore mj_step has less time to run.

And since the time I let my application run is fixed (around 20 seconds) less time is spent in mj_step (since it's called fewer times)

@ThoenigAdrian
Copy link
Author

I currently don't have enough time to run it against a few different versions. But when I do in the future I'll let you know the results.

@yuvaltassa
Copy link
Collaborator

@ThoenigAdrian we will look at the mj_copyData slowdown at some point in any case, but if you could let us know here whether copying just the state and recreating the contents of mjData did or did not solve your problem, that would help us prioritise the investigation. Thanks.

@ThoenigAdrian
Copy link
Author

@yuvaltassa copying the state and recreating the contents of mjData works fine for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants