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

[Darjus L. <da...@gm...>]

Nice!

On Mon, Sep 25, 2017 at 4:13 AM Jeff Allen <ja...@fa...> wrote:

> 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...> 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/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
>>>
>>> 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/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
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Jython-dev mailing list
> Jyt...@li...
> https://lists.sourceforge.net/lists/listinfo/jython-dev
>