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

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

Trying one more time to post to jython-dev...

On Mon, Sep 11, 2017 at 6:37 PM, Jim Baker <jim...@py...> 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
>>>
>>
>>
>