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

Make MCTSTree threadsafe #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make MCTSTree threadsafe #64

wants to merge 1 commit into from

Conversation

rejuvyesh
Copy link
Member

Allows simulate to update the tree from multiple threads.

Allows simulate to update the tree from multiple threads
@zsunberg
Copy link
Member

Nice! Interesting to start working on this. I have a few questions/concerns:

  1. Will this slow down single threaded execution at all?
  2. Do we need the locks when we are only reading from the tree instead of writing to it? (e.g. getting the best action node with UCB)
  3. I have wanted for a long time to restructure this package so that it can be more easily used outside of the POMDPs.jl simulation loop. I started writing that out here https://github.com/zsunberg/MonteCarloTreeSearch.jl but have not tried to finish it. If we decide to invest a lot in performance (with, e.g. multithreading), we may want to consider restructuring the interface so that it is not so tied to POMDPs.
  4. From everything I have read, locking the entire tree will result in really poor performance. What people have done successfully instead is not use any locks and just use atomic operations (see https://docs.julialang.org/en/v1/base/multi-threading/#Base.Threads.Atomic for info on atomic operations in Julia) and if it fails sometimes, it is just not that bad. I think this is what they did in alpha go.

Copy link
Member

@zsunberg zsunberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you answer questions 1 and 2 in my comment? Questions 3 and 4 don't need answers for this to get merged, but can you confirm that you have thought about them.

@rejuvyesh
Copy link
Member Author

Thanks @zsunberg!

  1. In my small experiments I haven't seen any slowdowns, but should do more benchmarking first. This PR was more to get the ball rolling.
  2. I ran into some errors when not adding some locks during reading, but I should reconfirm how many of them are actually necessary.

Regarding 4, I wouldn't go as far as saying really poor performance. It's still faster than single threading I think. But yeah, there definitely are ways to optimize this further. I went for the least effort thing partly because of 3, since I wasn't sure what's the long term plan for MCTS.jl.
We have even been building a MultiAgent version of this in MAPOMDPs.jl that supports coordination graphs.

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

Successfully merging this pull request may close these issues.

2 participants