Skip to content

Put RefCell around frames member of VirtualMachine. #706

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

Merged
merged 1 commit into from
Mar 22, 2019

Conversation

windelbouwman
Copy link
Contributor

This allows for VirtualMachine to be immutable. This is a first work into the direction that VirtualMachine is immutable.

This enables a few things:

  • Store a virtualmachine reference into each python object (a sort of handle). This would reduce the amount of VirtualMachine passing around.
  • Share the VirtualMachine accross threads in a memory safe way.

@OddCoincidence
Copy link
Contributor

OddCoincidence commented Mar 20, 2019

Share the VirtualMachine accross threads in a memory safe way.

Note that RefCell is not Sync so VirtualMachine will not be accessible from multiple threads with this change. A possible way around this would be to change the RefCell to a Mutex and go with @cthulahoops's solution by returning a PyRef<Frame>. (Because Mutex has no map method.)

(This comment actually applies to every other use of RefCell in this project also.)

(This isn't to say we shouldn't merge this PR as it still moves in the right direction of allowing us to change from &mut VirtualMachine to &VirtualMachine everywhere.)

@windelbouwman
Copy link
Contributor Author

I'm aware that this makes it not fully cross thread safe, but it is a good step. We can change each occurence of RefCell into the proper thread safe thing to achieve this.

Note that I would like to merge this first, before the second change which involves a lot of search and replace.

@windelbouwman
Copy link
Contributor Author

I also replaced 'some' occurrences of mut VirtualMachine now.

@windelbouwman
Copy link
Contributor Author

Okay, how to proceed with this change? I see a lot of merge conflicts coming up. Maybe wait with this a little while and redo the changes? Advice and opinions welcome!

@OddCoincidence
Copy link
Contributor

Is there a reason to wait? I thought this was ready to merge (prior to the conflicts)

@cthulahoops
Copy link
Collaborator

I would merge the first commit now - it's approved and ready to go.

Then do the big remove all the muts as separate PR - no need to review we all know it's coming and what it does. It's going to cause conflicts with in progress work whenever we do it. (I guess that's equivalent to just fixing the merge conflicts and merging this now.)

@coolreader18
Copy link
Member

Yeah, I'd recommend rolling back the &mut VM -> &VM to make merging this easier, and then doing everything at once with sed or something.

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.

4 participants