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

Trajectory Writer performance worse than Legacy Writer #94

Closed
AsadJeewa opened this issue Mar 14, 2022 · 4 comments
Closed

Trajectory Writer performance worse than Legacy Writer #94

AsadJeewa opened this issue Mar 14, 2022 · 4 comments

Comments

@AsadJeewa
Copy link

AsadJeewa commented Mar 14, 2022

We are working on Mava https://github.com/instadeepai/Mava which uses Reverb for adders (similar to the way Acme does). The trajectory writer seemed to be working slower than the legacy writer so I decided to do some benchmarking with dummy data and compare the writers directly, (isolated from Mava) by extending the Reverb tutorial code (https://gist.github.com/AsadJeewa/18ed6df8875a33c2cb7893738211a82d). It clearly shows that regardless of the number of episodes, sequence length, or other parameters, the trajectory writer is slower (as per my comments here (#78).
I have attached an example.

The first graph shows cumulative time for write() calls.
image

The second shows the total execution time.
image

I have more results to share as required.

@sabelaraga
Copy link
Collaborator

Hey Asad,

The trajectory writer that some more work in Python so it may cause a slowdown when using small steps (not sure if that is your case, though). One thing you can try is the new StructuredWriter, we found some performance improvements with that one and it should be possible to add it to your benchmark.

Some examples on how that writer is used are in the test (https://github.com/deepmind/reverb/blob/master/reverb/structured_writer_test.py).

Let us know if that helps!

Sabela.

@AsadJeewa
Copy link
Author

Thanks, I have been trying to add it to my benchmarks. Am I understanding this correctly? The point of the structured writer is to specify what to store via configs/ patterns. We can implement any writer that used the trajectory writers with the structured writer (and vice versa)

@acassirer
Copy link
Contributor

It is possible to get the same behaviour using the StructuredWriter most of the time. If you have to do some custom logic at runtime to decide which part of an episode to add, or something along these lines, then it might not be possible.

@qstanczyk
Copy link
Collaborator

I think there were some improvements to the performance of Trajectory Writer, so closing this issue. Please reopen if this is still problematic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants