Re: [Jython-dev] PyType publication with better concurrency

[Jim B. <jim...@py...>]

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...> 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
> 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...> 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/f781dc
>> 1f52c9b3733841b375831f8a3c0d27a93e
>>
>> 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/f781dc
>> 1f52c9b3733841b375831f8a3c0d27a93e#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...> 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, 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...>> 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/3fc5d8
>>>> ff48bf5d20cb57169cd3f43a3cd90df0cd
>>>>     <https://bitbucket.org/tournesol/jython-fgtype/commits/3fc5d
>>>> 8ff48bf5d20cb57169cd3f43a3cd90df0cd>
>>>>
>>>>     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
>>>>
>>>
>>>
>>
>>
>
>