Menu
▾
▴
Re: [Jython-dev] PyType publication with better concurrency
|
From: Jeff A. <ja...@fa...> - 2017-09-24 18:13:33
|
Thanks Jim. I'm working on presenting this as a reduced number of commits to make the change easier to follow. I'll strip out the debug too. After so many weeks, I shall miss it, but it's too in-your-face. If we need it again, it will be on Bitbucket for a while. +1 for an early 2.7.2: we should make a short list of blockers on its own thread. Jeff On 24/09/2017 17:18, Jim Baker wrote: > Likewise, glad to see it was just a problem in the test setup. > Everything looks good now: the test demonstrates that the Python type > mapping does not prevent class GC, assuming nothing else refers to the > Java class. Indeed the earlier faulty version of the test further > demonstrates that point, given the parent classloader referring to the > class, and preventing GC. > > I have also had a chance to review the rest of the change set, and > everything else looks great. In particular, I like how you have > removed the need to support the hack for hardRef, along with the > management of deferred initialization of types. > > Let's merge this code in, given that it's a substantial improvement > over the old implementation, with all tests passing. We should > consider releasing a 2.7.2 soon, since this and related changes has > fixed serious problems in re and other mixed usage of Java and Python > via callbacks (http://bugs.jython.org/issue2609). Getting this out > would then allow us to work on Java 9 support in 2.7.3, as well as > fixing up sys slowness and related jsr223 (hopefully!). > > - Jim > > > > On Sun, Sep 24, 2017 at 3:19 AM, Jeff Allen <ja...@fa... > <mailto:ja...@fa...>> wrote: > > As I suspected, the critical distinction is between the > *initiating* class loader and the *defining* class loader. > > The defining class loader here turns out to be an instance of > sun.misc.Launcher$AppClassLoader, the grandparent of our > URLClassLoader that gets consulted before it tries the JAR URL we > gave. org.python.tests.Callbacker appears in jython-dev.jar, so it > is found there first. This is the same loader that holds PyObject, > etc., so essentially permanent and keeps our test class alive. > > When this is all fixed by making the parent explicitly None in > make_jar_classloader, everything is GC'd as we'd hope. (Phew!) > > https://bitbucket.org/tournesol/jython-fgtype/commits/4fd2baafb7499929f876ac20a607511668b3e222 > <https://bitbucket.org/tournesol/jython-fgtype/commits/4fd2baafb7499929f876ac20a607511668b3e222> > > Jeff > > > > On 18/09/2017 00:45, Jim Baker wrote: >> Jeff, >> >> I looked at the new test, and it's interesting to see the >> ClassLoader gets collected, but not the corresponding PyJavaType. >> To me this implies the Class was necessarily collected; but it's >> not observable directly in Python code. While you are busy on >> other things, I will try playing with this changeset some more to >> see what is going on here. Maybe it ends just being a simple >> question of setting up a ref queue on Class GC and calling >> ClassValue#remove. >> >> It's rather awesome about the simplification of fromClass, and >> removing the separate implementation of fromClassSkippingInners. >> >> - Jim >> >> On Sun, Sep 17, 2017 at 2:41 AM, Jeff Allen <ja...@fa... >> <mailto:ja...@fa...>> wrote: >> >> Jim: >> >> I figured out needsInners (I think!) and how I could make >> fromClass and fromClassSkippingInners into one method using >> the state of the registry to steer the action to be almost >> the same -- and reach the same conclusion -- with a net >> reduction in code. >> >> https://bitbucket.org/tournesol/jython-fgtype/commits/f781dc1f52c9b3733841b375831f8a3c0d27a93e >> <https://bitbucket.org/tournesol/jython-fgtype/commits/f781dc1f52c9b3733841b375831f8a3c0d27a93e> >> >> I have added a test to prove the PyType (actually PyJavaType) >> is GC'd when we cease to reference it, using >> make_jar_classloader, but it is presently failing. Applied >> retrospectively to successive change sets it begins to fail >> exactly where I introduce ClassValue. :( >> >> Maybe ClassValue doesn't quite work as I thought. Or I'm >> still not using it as John Rose intended. It seems wrong we >> should have to layer weak references on top or have some kind >> of removal queue. >> >> I tried visualvm to see where it might be referenced, but I'm >> not sure how to interpret what it's telling me. However, this >> week, and maybe longer, other things have priority. Ideas >> welcome for when I'm back to this. >> >> For exposed PyObjects, I'm pretty sure classToBuilder always >> was keeping a strong reference that would keep them alive >> now. (But not for the type under test.) I've removed this >> effect. But maybe we don't mind that? >> https://bitbucket.org/tournesol/jython-fgtype/commits/f781dc1f52c9b3733841b375831f8a3c0d27a93e#Lsrc/org/python/core/PyType.javaT1208 >> <https://bitbucket.org/tournesol/jython-fgtype/commits/f781dc1f52c9b3733841b375831f8a3c0d27a93e#Lsrc/org/python/core/PyType.javaT1208> >> >> Jeff >> >> >> On 12/09/2017 01:37, Jim Baker wrote: >>> [I'm been having some problem posting to the jython-dev >>> mailing list, let's see if this response gets through!] >>> >>> On Mon, Sep 11, 2017 at 1:47 PM, Jeff Allen >>> <ja...@fa... <mailto:ja...@fa...>> wrote: >>> >>> Jim: >>> >>> Thanks for the pointers. I agree with you on #2 that >>> what I've got *should* behave correctly with class >>> loaders, but is there a test you can suggest (or one >>> I've missed) that conclusively demonstrates it? >>> >>> >>> So I would start with test_support.make_jar_classloader, >>> just because it's easier to test this behavior in Python. >>> test_classpathimporter seems to use this support >>> functionality nicely. >>> >>> If we want to add testing of our expectations around >>> ClassValue's lifetime, we can also add some sort of weakref >>> scheme, in conjunction with a multi-round GC, to verify that >>> the PyType gets collected as expected. Of course we should >>> expect ClassValue to behave as expected, but this would >>> ensure we are not holding anything, now or in the future >>> implementation, in the Jython runtime that would violate >>> this contract. >>> >>> >>> I'll press in on the question of inners. I tried just >>> calling fromClass, but it fails when the inner extends >>> the outer (test_import_jy:: test_selfreferential_classes >>> fails). You have to initialise the outer, then visit the >>> inners. In a way, it's the PyJavaType counterpart of the >>> deferred initialisation of exposed types that we control >>> by identifying bootstrap types. But here it has to be >>> self-guiding. >>> >>> I have an idea that if I can make fromClass and >>> fromClassSkippingInners the same thing, then that can >>> become the implementation classToValue.computeValue, and >>> I won't need the NotReady exception or findExistingType. >>> >>> >>> So Java classes can form a general type graph (directed, >>> with cycles). It seems to me that the NotReady acts as some >>> sort of coloring in the graph exploration that fromClass is >>> doing. But as I said, I find this code for inner classes >>> rather subtle! Good luck in your coding here, it would be >>> really nice to get some further simplification. >>> >>> >>> Jeff >>> >>> On 10/09/2017 01:12, Jim Baker wrote: >>> >>> Thanks for working on this change set, it's really >>> good stuff. In particular, that's a very nice >>> handoff you do in the PyType.Registry class, >>> specifically moving computed types from >>> classToNewType to classToType; and managing the >>> NotReady state. Perhaps it needs some sort of state >>> diagram to illustrate, but as someone who has been >>> in that code, it makes perfect sense. >>> >>> With respect to GC, the following should be the case: >>> >>> * As long as the ClassLoader for the mapped Java >>> class instance C is >>> around, the corresponding PyType instance T will >>> live. This >>> follows from the definition of ClassValue as you >>> point out in the >>> code comments. (Good!) >>> * As long as Python code holds a hard ref to T, >>> the corresponding C >>> will live. (Good!) (So we would see this in from >>> `some_java_pkg >>> import baz`; and it follows from the >>> underlying_class field >>> holding a hard ref from T to C.) >>> * As long as Python code holds a hard ref to an >>> instance of C, the >>> corresponding T will also live. (Good!) >>> >>> Otherwise, this mapping will allow for GC of T, and >>> subsequently of C, per the definition of ClassValue. >>> I was also looking at Groovy's usage of a similar >>> mapping, and corresponding bug, >>> https://bugs.openjdk.java.net/browse/JDK-8136353 >>> <https://bugs.openjdk.java.net/browse/JDK-8136353>, >>> and it looks like the problem reported there doesn't >>> hold. This is because there are no strong refs >>> outside of PyType itself. >>> >>> So everything looks good for getting this change in. >>> >>> On Sat, Sep 9, 2017 at 2:12 PM, Jeff Allen >>> <ja...@fa... <mailto:ja...@fa...> >>> <mailto:ja...@fa... >>> <mailto:ja...@fa...>>> wrote: >>> >>> Jim, all: >>> >>> By a series of small steps I've come quite a >>> long way in this >>> aspect of PyType. It might be worth a look. >>> https://bitbucket.org/tournesol/jython-fgtype/commits/3fc5d8ff48bf5d20cb57169cd3f43a3cd90df0cd >>> <https://bitbucket.org/tournesol/jython-fgtype/commits/3fc5d8ff48bf5d20cb57169cd3f43a3cd90df0cd> >>> >>> <https://bitbucket.org/tournesol/jython-fgtype/commits/3fc5d8ff48bf5d20cb57169cd3f43a3cd90df0cd >>> <https://bitbucket.org/tournesol/jython-fgtype/commits/3fc5d8ff48bf5d20cb57169cd3f43a3cd90df0cd>> >>> >>> In this last commit I've entirely dispensed with >>> any long-term >>> reference to the created PyType in the type >>> registry: only the >>> ClassValue holds one. The explicit management of >>> reachability goes >>> away and we have just the behaviour given us by >>> ClassValue. The >>> only down-side I see is that a PyType created >>> for a Java class you >>> handle briefly from Python will not be >>> collectable until the class is. >>> >>> The old test for weakness, which I've been >>> running in parallel >>> with the new, requires PyType.getClassToType(), >>> and can no longer >>> be supported. (You can't enumerate the >>> ClassValue.) I believe the >>> replacement tests the same thing -- they used to >>> pass and fail >>> together. >>> >>> >>> Right, it makes sense to remove this test. This is >>> because we don't want to support the necessary >>> mapping, even though it could be readily supported >>> with an appropriate concurrent weak mapping - the >>> only semantics it would be supporting is for this >>> very test. >>> >>> >>> I've moved some actions around, and removed some >>> of the checks. We >>> seemingly expected the desired PyType to be >>> created at any moment >>> by a side effect. Reason and regression testing >>> tell me that >>> doesn't happen in the new arrangement. I think >>> there is slightly >>> less code to execute, but masses more comment >>> and debug -- it's my >>> way of figuring it out. It's all very educational. >>> >>> I still don't understand: >>> >>> 1. Whether we find the downside (PyType not GC'd >>> until the class is) >>> important. >>> >>> >>> So we do know that Jython will continue to work if >>> we remove a mapping entry, since it happens all the >>> time currently in some code like json, re, and >>> socket, where classes are not directly imported :) >>> We could always do some sort of LRU and then call >>> ClassValue#remove, but I don't think it makes sense. >>> There are many other other optimizations in the >>> runtime first. >>> >>> 2. Whether I have broken something that the >>> regression tests don't >>> cover (like use with isolating class loaders). >>> >>> >>> These are different Java classes if in different >>> class loaders. For Python usage of these Java >>> classes, it's all the same, thanks to duck typing, >>> but we need to maintain a different mapping entry. >>> Again #1 demonstrates we have been handling this >>> correctly in the current codebase. >>> >>> 3. What needsInners is really up to. I see >>> *what* it does, but not >>> *why* well enough to consider alternatives. >>> It would simplify >>> computeValue if there were no >>> fromClassSkippingInners: could >>> needsInners be a registry global? And why do >>> we skip processing >>> them, when we do? >>> >>> >>> That codepath is exceedingly subtle, at least to me. >>> But it would be an interesting experiment to see if >>> it could be made global, vs this potentially >>> expanding context that is passed through. >>> >>> - Jim >>> >>> >>> >> >> > > |
