RFR(S): 8131129: Attempt to define a duplicate BMH$Species class
Peter Levart
peter.levart at gmail.com
Wed Nov 4 22:31:42 UTC 2015
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've been tracking this patch for a bit since it's startup sensitive,
> and suggested some improvements offline to avoid the reflection
> lookups on non-asserting code that's been rolled into this patch.
>
> 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.
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