SharedInterpreters now share MemoryManager and java PyObjects. #421
Conversation
| t.join(); | ||
| String test2 = list.getAttr("pop", PyCallable.class).callAs(String.class); | ||
| String test = list.getAttr("pop", PyCallable.class).callAs(String.class); | ||
| if (!test.equals("test")) { |
There was a problem hiding this comment.
Best practice is to put the constant string on the left side of the .equals() call. !"test".equals(test)
| if (!test.equals("test")) { | ||
| throw new IllegalStateException("Expecting 'test', not " + test); | ||
| } | ||
| if (!test2.equals("test2")) { |
There was a problem hiding this comment.
Best practice is to put the constant string on the left side of the .equals() call. !"test2".equals(test2)
| @@ -80,9 +75,9 @@ protected PyPointer(PyObject referrent, MemoryManager memoryManager, | |||
| */ | |||
| protected synchronized void dispose() throws JepException { | |||
| if (!disposed) { | |||
| decref(memoryManager.getThreadState(), pyObject); | |||
| disposed = true; | |||
There was a problem hiding this comment.
Why did you put the decref ahead of disposed = true? If decref throws an exception then you could keep attempting to dispose it over and over again, which seems kinda bad.
There was a problem hiding this comment.
I didn't consider that, I changed it to set the disosed before decref.
| try (Interpreter interp2 = new SubInterpreter()) { | ||
| list.getAttr("append", PyCallable.class).call("test"); | ||
| pass[0] = false; | ||
| } catch (JepException e) { |
There was a problem hiding this comment.
What is the message on the exception that is thrown? It's not clear to me how you made it work for SharedInterpreters but not SubInterpreters.
There was a problem hiding this comment.
The message is "Invalid thread access" from line 122 of MemoryManager.
The MemoryManager is keeping track of which interpreters are sharing memory and uses a ThreadLocal to track which threads those interpreters are valid on. When PyPointer or PyObject gets the thread state for any native operation it is now going through the MemoryManager which will return the thread state if there is an interpreter for that MemoryManager on that thread. If there is no interpreter for a memory manager then the "Invalid thread access" is thrown.
The implementation of the MemoryManager and PyPointer do not specifically behave differently for SubInterpreter vs. SharedInterpreter, But during construction the SharedInterpreters wil use a static MemroyManager that is shared and SubInterpreters will construct a new MemoryManager and this difference in construction causes different behavior.
| try (Interpreter interp2 = new SubInterpreter()) { | ||
| interp2.set("l", list); | ||
| pass[0] = false; | ||
| } catch (JepException e) { |
There was a problem hiding this comment.
What is the message on the exception that is thrown? It's not clear to me how you made it work for SharedInterpreters but not SubInterpreters.
There was a problem hiding this comment.
The message is "Invalid thread access" from line 122 of MemoryManager.
This enables all SharedInterpreters to use the same MemoryManager so that a PyPointer from any SharedInterpreter can be used on threads from any other SharedInterpreters. This requires internal changes to MemoryManager and the way it interacts with PyPointer because there is no longer a one-to-one relationship between a tstate and a MemoryManager, instead the same MemoryManager is valid for multiple tstates.
This should introduce more flexibility for developers. For ThreadPools performing the same task in different interpreters being able to reuse PyObjects should make them a more appealing feature.
I am hoping to expand this feature in a future release so that the MainInterpreter can clean up references from the shared MemoryManager, which would remove the requirement to keep an interpreter open by allowing cleanup without another Interpreter.