Skip to content

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.

5 participants