RFR(S): 8131129: Attempt to define a duplicate BMH$Species class
Claes Redestad
claes.redestad at oracle.com
Wed Nov 4 23:09:42 UTC 2015
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
>>
>
More information about the core-libs-dev
mailing list