jython-dev

[Jython-dev] PyType publication with better concurrency

[Jeff A. <ja...@fa...>]

Jim, all:

Apologies for the near repeat, but my first attempt to give this a life 
of its own on Jython-dev ended up nested under issue 2487 again. Hope 
this lands as a fresh topic.

On 05/08/2017 20:34, in Issue #2387 (http://bugs.jython.org/issue2487), 
Jim Baker wrote:
> ...
>
> Next, let's look at our caching, and its corresponding
> performance. Using weak keys/weak values in class_to_type means that
> when working with Java objects (calling methods, accessing
> attributes), Python code that does not maintain a strong reference to
> the Java class will cause the Jython runtime to constantly rebuild
> these entries in class_to_type. Here are the ways that such strong
> references would be established:
>
> 1. exposedTypes (so Jython's internals)
> https://hg.python.org/jython/file/1a14d360dc8d/src/org/python/core/PyType.java#l104 
>
>
> 2. imports of Java classes into a Python namespace
>
> 3. references to `type(java_obj)`
>
> 4. subclassing of a Java class by a Python class
>
> References to Java *objects*, including objects from factory methods;
> indirect construction, such as `java.util.HashMap()`; and callbacks
> (Netty, re/json/strptime) would not introduce such references to
> Python types for Java *classes*. But calling methods, using
> attributes, or otherwise using getting the Python type on the Java
> object, would require this entry in class_to_type to be added,
> potentially to be potentially quickly discarded by generational
> GC. Computing these entries is relatively expensive, especially if a
> given class C has inner classes (and this is a general type graph
> expansion).
>
> See also the analysis in https://bugs.openjdk.java.net/browse/JDK-6389107
> (scenario 3); our mapping in class_to_type is a common pattern seen in
> Java systems.
>
> What if instead of current one-level cache, we had a two-level cache?
> Level 1 uses weak key/weak value for the class/type entries (C, T);
> (C, T) entries that are expired from Level 1 are moved to Level 2;
> Level 2 uses an expiration time so a ClassLoader could be unloaded in
> say some reasonable amount of time.
>
> To implement:
>
> 1. Level 1 cache is a direct replacment of class_to_type with
>     CacheBuilder weak keys/weak values/removal listener. Also we will
>     not attempt to expose it as a Map going forward; adjust
>     class_to_type usage accordingly.
>
> 2. Level 2 cache is weak keys/strong values/expires after write. We
>     can tune this expiration as was done with the regex cache and the
>     python.sre.cachespec option,
> https://github.com/jythontools/jython/blob/master/src/org/python/modules/sre/SRE_STATE.java#L1253). 
>
>
> 3. For removalListener: (C, T) entries that are removed from Level 1
>     will use removalListener to get placed in Level 2 (safe because
>     Level 1 is concurrent; and obviously hard refs would prevent it
>     from being removed while under construction).
>
> 4. Try both caches when looking up the type T for class C. Because
>     synchronized, there's no visible inconsistency; if no entry for C
>     because it was removed, simply recompute T'. There is a small
>     window of time that it could be in the process of being moved to
>     Level 2, but this does not affect correctness; (C, T) will be
>     eventually expired from Level 2 and (C, T') is a valid repllacement
>     for it (because there can be no hard refs to T outside the level 2
>     cache itself).
>
> Lastly, 
> https://docs.oracle.com/javase/7/docs/api/java/lang/ClassValue.html
> looks potentially useful as an alternative for publishing PyType
> objects into a cache. But we need to address two key questions:
>
> 1. Whether ClassValue#computeValue would work properly in the
>     re-entrant case where the class graph from a given class C is
>     explored, as is done for inners, possibly referring back to C. This
>     is why we could not get the publication of the type in the map done
>     after the init (as one would usually want to do!) in
> https://github.com/jythontools/jython/blob/master/src/org/python/core/PyType.java#L1515 
>
>     (putIfAbsent); class C would be referenced, and it would attempt to
>     build again (and stack overflow). No solution I tried could break
>     this problem.
>
> 2. When to call ClassValue#removeValue ! We still have to keep track
>     of the cache with respect to PyType.

I have an idea that I can use ClassValue with this, but it sits as a 
(lock-free) cache in front of the same (properly locked) apparatus we 
have now.

Clients see a static PyType.forClass() that is not synchronised and 
simply calls the (ClassValue) pyType.get(c). A miss results in 
pyType.computeValue(c), which uses the existing synchronised apparatus. 
When we wrestle with types internally to the type registry, we call an 
internal Registry.forClass(), and other methods, that are essentially 
the ones we're familiar with. We don't use the ClassValue pyType for 
look-up internally because that would expose incomplete types to other 
threads. In the first instance, this means I can leave the existing code 
essentially unmodified, apart from turning it into a private delegate.

The limitation of ClassValue is that it doesn't let you supply 
information in the look-up call that can be passed to computeValue: it 
has to be possible using only the class c itself, and information 
provided beforehand. In our case, the extra argument is just the boolean 
hardRef that is supposed to decide whether the PyType created ends up in 
exposedTypes.

I think I can deal with it, awkwardly, but that API seems questionable: 
many places the first call is PyType.forClass(this, false) and a later 
(default) call has hardRef=true, but comes too late. The creation of 
'object' and 'type' are notable examples of core types missing from 
exposedTypes. In other places, an explicit false or implicit true is 
given when the thought may only be to look something up, and the 
possibility it will be created at that point may not have been 
considered. I wonder if we could decide, either by looking at the class 
(name) or through a list specified in advance, that certain classes 
should be strongly referenced?

I think I understand the intent of the two-level cache, and trying 
ClassValue doesn't invalidate it. It might do part of the work for us. 
In particular, a ClassValue does not prevent unloading of the class. 
(It's a weak-keys cache.) It keeps its value alive until the 
corresponding class is unloaded (unless the value is explicitly a weak 
reference).

Thing is, I don't understand in what circumstances we ought to create 
the strong reference: what need are we actually seeking to meet? Is the 
critical use case that the PyType could be freed when Python code stops 
using it, and yet the Java class has reason to remain?

Jeff

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

[Jeff A. <ja...@fa...>]

PyType really is quite hairy code. It has certainly tested my 
understanding of Java initialisation. I've not changed anything 
materially yet, but produced a version plastered with print statements 
that others might find amusing:

https://bitbucket.org/tournesol/jython-fgtype/commits/094f87e1ddaa5d30b4d1be9fcf3209e0ff6432bd

It produces output like this:

addBuilder(class o.p.c.PyShadowString, class o.p.c.PyShadowString$PyExposer)
--- --- --- ---
fromClass(class o.p.c.PyShadowString, true) -> null
|   addFromClass(class o.p.c.PyShadowString)
|   createType(class o.p.c.PyShadowString)
|   getBuilder(class o.p.c.PyShadowString)
|   Class.forName(class o.p.c.PyShadowString) with static init.
|   fromClass(class o.p.c.PyType, false) -> <type 'type'>
|   PyType.init(class o.p.c.PyShadowString)
|   fromClass(class o.p.c.PyString, false) -> <type 'str'>
|   fromClass(class o.p.c.PyMethodDescr, false) -> <type 'method_descriptor'>
|   fromClass(class o.p.c.PyMethodDescr, false) -> <type 'method_descriptor'>
|   fromClass(class o.p.c.PyMethodDescr, false) -> <type 'method_descriptor'>
|   fromClass(class o.p.c.PyMethodDescr, false) -> <type 'method_descriptor'>
|   fromClass(class o.p.c.PyMethodDescr, false) -> <type 'method_descriptor'>
|   fromClass(class o.p.c.PyMethodDescr, false) -> <type 'method_descriptor'>
|   +-- init'd <type 'shadowstr'> (from class o.p.c.PyShadowString)
|   +-- created <type 'shadowstr'> from class o.p.c.PyShadowString
+-> <type 'shadowstr'> (from class o.p.c.PyShadowString)
--- --- --- ---

There is some odd-but-not-incorrect behaviour amongst the 15000 or so 
lines, including PyArray initialised twice and _codecs$EncodingMap left 
in the bootstrap types at the end (until you look up the right kind of 
codec). Now I understand the logic better, and have a baseline, I'll try 
the ClassValue idea, and exhibit it in the same place.

Jeff

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

[Jeff A. <ja...@fa...>]

Jim, all:

I had a first go at using ClassValue to provide non-blocking look-up. It 
seems to work. Fork at https://bitbucket.org/tournesol/jython-fgtype .

I have moved the relevant static methods of PyType into an inner class 
(PyType.Registry) with the data structures they manipulate. These 
methods are essentially unchanged by the move. One needs forwarding 
methods for fromClass and addBuilder at the outer level like this:

     public static PyType fromClass(Class<?> c, boolean hardRef) {
         return Registry.fromClass(c, hardRef);
     }

Data structures in the Registry relevant to this discussion are:

     private static final class Registry {

         /** Mapping of Java classes to their PyTypes. */
         private static final Map<Class<?>, PyType> classToType = ... ;

         /** Classes added to classToType where the PyType in not completely init'd */
         private static final Set<Class<?>> classToTypeNotReady = new HashSet<>();

         /** Acts as a non-blocking cache for PyType look-up. */
         private static ClassValue<PyType> cv = new ClassValue<PyType>() {...};

Answering the points about ClassValue below, the set classToTypeNotReady 
tracks unfinished inners and bootstrap types. The way to prevent caching 
a result (point 1), I suddenly realised, is to terminate 
ClassValue.computeValue abnormally:

         private static ClassValue<PyType> cv = new ClassValue<PyType>() {
             @Override protected PyType computeValue(Class<?> c) throws NotReady {
                 synchronized (Registry.class) {
                     PyType type = classToType.get(c);
                     if (type == null || classToTypeNotReady.contains(c)) {
                         // Avoid anything being cached in the class value.
                         throw new NotReady();
                     }
                     return type;
                 }
             }
         };

In that case, the outer PyType.fromClass is made to fall back on the 
familiar version, which (as now) allows re-entrancy in the owning 
thread, but blocks concurrent access:

     public static PyType fromClass(Class<?> c, boolean hardRef) {
         try {
             return Registry.cv.get(c);
         } catch (NotReady nr) {
             return Registry.fromClass(c, hardRef);
         }
     }

This all passes the regression tests (ant regrtest), slightly quicker if 
anything.

Answering point 2, it does this without holding onto PyTypes when we 
have finished with them. (See the revised test in test_jy_internals.) 
This last observation surprised me. classToType is still a double-weak 
map, but I my use of cv has no explicit weakness or removal code. It 
must hold the PyType reachable as long as the Class it refers to is 
reachable, by contract, but apparently no longer. Is this not exactly 
what we need?

Jeff

On 09/08/2017 08:24, Jeff Allen wrote:
> ...
> On 05/08/2017 20:34, in Issue #2387 
> (http://bugs.jython.org/issue2487), Jim Baker wrote:
>> ...
>> Lastly, 
>> https://docs.oracle.com/javase/7/docs/api/java/lang/ClassValue.html
>> looks potentially useful as an alternative for publishing PyType
>> objects into a cache. But we need to address two key questions:
>>
>> 1. Whether ClassValue#computeValue would work properly in the
>>     re-entrant case where the class graph from a given class C is
>>     explored, as is done for inners, possibly referring back to C. This
>>     is why we could not get the publication of the type in the map done
>>     after the init (as one would usually want to do!) in
>> https://github.com/jythontools/jython/blob/master/src/org/python/core/PyType.java#L1515 
>>
>>     (putIfAbsent); class C would be referenced, and it would attempt to
>>     build again (and stack overflow). No solution I tried could break
>>     this problem.
>>
>> 2. When to call ClassValue#removeValue ! We still have to keep track
>>     of the cache with respect to PyType.
>

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

[Jeff A. <ja...@fa...>]

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

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.

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.
 2. Whether I have broken something that the regression tests don't
    cover (like use with isolating class loaders).
 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?


Jeff

On 09/08/2017 08:24, Jeff Allen wrote:
> Jim, all:
>
> Apologies for the near repeat, but my first attempt to give this a 
> life of its own on Jython-dev ended up nested under issue 2487 again. 
> Hope this lands as a fresh topic.
>
> On 05/08/2017 20:34, in Issue #2387 
> (http://bugs.jython.org/issue2487), Jim Baker wrote:
>>
>> What if instead of current one-level cache, we had a two-level cache?
>> Level 1 uses weak key/weak value for the class/type entries (C, T);
>> (C, T) entries that are expired from Level 1 are moved to Level 2;
>> Level 2 uses an expiration time so a ClassLoader could be unloaded in
>> say some reasonable amount of time.
>> ...
>>
>> Lastly, 
>> https://docs.oracle.com/javase/7/docs/api/java/lang/ClassValue.html
>> looks potentially useful as an alternative for publishing PyType
>> objects into a cache. But we need to address two key questions:
>>
>> 1. Whether ClassValue#computeValue would work properly in the
>>     re-entrant case where the class graph from a given class C is
>>     explored, as is done for inners, possibly referring back to C. This
>>     is why we could not get the publication of the type in the map done
>>     after the init (as one would usually want to do!) in
>> https://github.com/jythontools/jython/blob/master/src/org/python/core/PyType.java#L1515 
>>
>>     (putIfAbsent); class C would be referenced, and it would attempt to
>>     build again (and stack overflow). No solution I tried could break
>>     this problem.
>>
>> 2. When to call ClassValue#removeValue ! We still have to keep track
>>     of the cache with respect to PyType.
>
> I have an idea that I can use ClassValue with this, but it sits as a 
> (lock-free) cache in front of the same (properly locked) apparatus we 
> have now.
> ...
> Thing is, I don't understand in what circumstances we ought to create 
> the strong reference: what need are we actually seeking to meet? Is 
> the critical use case that the PyType could be freed when Python code 
> stops using it, and yet the Java class has reason to remain?

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

[Jeff A. <ja...@fa...>]

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?

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.

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

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
>>>
>>
>>
>

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

[Jeff A. <ja...@fa...>]

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... 
> <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
>
>
>

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

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

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/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
>>>
>>
>>
>
>

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

[Jeff A. <ja...@fa...>]

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... 
> <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
>>
>>
>>
>
>

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
>>>>
>>>
>>>
>>
>>
>
>

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

[Jeff A. <ja...@fa...>]

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
>>>
>>>
>>>
>>
>>
>
>

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
>