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