RFR(S): 8131129: Attempt to define a duplicate BMH$Species class

[Claes Redestad]

On 2015-11-04 23:31, Peter Levart wrote:
> Hi Claes,
>
> On 11/04/2015 09:12 PM, Claes Redestad wrote:
>> Hi,
>>
>> On 2015-11-04 13:18, Peter Levart wrote:
>>> Here's what I am thinking, in code:
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/BMH.race/webrev.02/
>>>
>>> Now that definition of BMH subclass is atomic, caching of 
>>> SpeciesData can be simplified. We don't need special placeholder 
>>> instances as locks and synchronized static methods. To make BMH 
>>> subclass definition atomic, we can leverage CHM.computeIfAbsent that 
>>> does the similar "placeholder" dance, but in much more sophisticated 
>>> way. BMH logic is much more straightforward and easier to grasp that 
>>> way.
>>>
>>> So what do you think of this version. Your version is logically 
>>> correct too, so you decide which one is better.
>>
>> I gave both patches here a spin and noticed that Peter's variant 
>> pulls in some 6 extra classes on a jigsaw Hello World test I'm 
>> playing with (such as ConcurrentHashMap$BaseIterator). Not a strong 
>> argument in itself, but if there's no stronger reason for your 
>> version than to clean this up a bit I'd vote in favor of Michael's 
>> approach...
>
> The extra classes needed are not a consequence of using 
> ConcurrentHashMap per se (as it is already used in CacheLoader to hold 
> locks), but the result of iteration that is performed here:
>
>  431             for (SpeciesData d : CACHE.values()) {
>  432                 d.initForBootstrap();
>  433             }
>
>
> ...if this iteration is replaced by iteration over staticSpeciesData 
> array, there should not be any additional class loaded...
>
> I just don't know why this is needed:
>
>  367         static { CACHE.put("", EMPTY); }  // make bootstrap 
> predictable
>
> If this is there to force HashMap (or ConcurrentHashMap) to initialize 
> it's internal table (which it does lazily) and the entry is otherwise 
> not used, then iterating over staticSpeciesData array becomes 
> equivalent to iterating over ConcurrentHashMap's values... If this 
> EMPTY value is used, it can be EMPTY.initForBootstrap()ed explicitly 
> out of loop.

I'd be happy with getting rid of both loops and the staticSpeciesData 
array. Refactor the second two rows into a private static method or not 
and merge with the static block at line 367:

private static void initSpeciesData(SpeciesData speciesData) {
     CACHE.put(speciesData.typeChars, speciesData);
     speciesData.initForBootstrap();
}

static {
     // Pre-fill the BMH species-data cache with BMH's inner subclasses. 
All of these classes' SPECIES_DATA
     // fields must be added to ensure proper cache population.
     initSpeciesData(EMPTY);
     initSpeciesData(Species_L.SPECIES_DATA);

     assert speciesDataCachePopulated();
...

The assert should take care of failing (a lot of) tests if someone 
forgets to statically add initSpeciesData for any future (static) inner 
subclasses of BMH, so I can't think of a real reason to not simplify 
like this.

/Claes

>
> Regards, Peter
>
>>
>>> Regards, Peter
>>>
>>> On 10/29/2015 04:20 PM, Michael Haupt wrote:
>>>> Hi Vladimir, Peter,
>>>>
>>>> once more, thanks for all your comments. The revised webrev is at 
>>>> http://cr.openjdk.java.net/~mhaupt/8131129/webrev.01/.
>>
>>  however, the access to FAILED_SPECIES_CACHE doesn't seem to be 
>> thread-safe and needs to be synchronized with a static lock object in 
>> BoundMethodHandle (initiating different SpeciesData concurrently 
>> might lead to ConcurrentModificationException when accessing or 
>> putting values into FAILED_SPECIES_CACHE.
>>
>> I'd suggest cleaning up the synchronized methods to lock on specific 
>> objects while we're at it, and maybe should initialize 
>> FAILED_SPECIES_CACHE as Collections.emptyList(), since it'll 
>> typically never be used anyhow:
>>
>> http://cr.openjdk.java.net/~redestad/scratch/bmh.race.01/
>>
>> Perhaps this clunky implementation is an argument in favor of Peter's 
>> approach, but it keeps class count in check.
>>
>> Thanks!
>>
>> /Claes
>>
>>>>
>>>> Best,
>>>>
>>>> Michael 
>>
>