On 11/05/2015 12:09 AM, Claes Redestad wrote:
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.
Ok, here it is:
http://cr.openjdk.java.net/~plevart/jdk9-dev/BMH.race/webrev.03/
I just moved CACHE registration into SpeciesData.initForBootstrap()
method, so no new method is needed. Note that patched source now
contains the same number of lines as original. How does the jigsaw
HelloWorld score now?
Regards, Peter
/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
