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