RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData
Hi, the current implementation of ClassSpecializer.findSpecies may cause excessive blocking due to a potentially expensive computeIfAbsent, and we have reason to believe this might have been cause for a few very rare bootstrap issues in tests that attach debuggers to VM in the middle of this. Breaking this apart so that code generation and linking is done outside of the computeIfAbsent helps minimize the risk that a thread gets interrupted by a debugger at a critical point in time, while also removing a few limitations on what we can do from a findSpecies, e.g., look up other SpeciesData. Bug: https://bugs.openjdk.java.net/browse/JDK-8202184 Webrev: http://cr.openjdk.java.net/~redestad/8202184/open.00/ This implementation inserts a placeholder, unresolved SpeciesData, and then replaces it with another instance that is linked together fully before publishing it, which ensure safe publication. There might be alternatives involving volatiles and/or careful fencing, but I've not been able to measure any added startup cost from this anyway. Thanks! /Claes
Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData
Hi, This looks good. Took me a while to understand the interactions: you need to replace not update otherwise there is a race on isResolved (which currently queries multiple state, there is no singular guard here). We could push isResolved into the synchronized block and simplify but every findSpecies call may take a small hit (or there are potentially other ways looking more closely at how ClassSpecializer.Factory initializes state, i.e. a fenced static field write, but we go further down the rabbit hole) I think this might fix some rare and intermittent recursive exceptions from ConcurrentHashMap cache we have been observing, where a collision occurs on keys for recursive updates (rather than for the same key). Paul. > On Apr 24, 2018, at 9:57 AM, Claes Redestad wrote: > > Hi, > > the current implementation of ClassSpecializer.findSpecies may cause > excessive blocking due to a potentially expensive computeIfAbsent, and > we have reason to believe this might have been cause for a few very > rare bootstrap issues in tests that attach debuggers to VM in the middle > of this. > > Breaking this apart so that code generation and linking is done outside > of the computeIfAbsent helps minimize the risk that a thread gets > interrupted by a debugger at a critical point in time, while also removing > a few limitations on what we can do from a findSpecies, e.g., look up other > SpeciesData. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8202184 > Webrev: http://cr.openjdk.java.net/~redestad/8202184/open.00/ > > This implementation inserts a placeholder, unresolved SpeciesData, > and then replaces it with another instance that is linked together > fully before publishing it, which ensure safe publication. There might > be alternatives involving volatiles and/or careful fencing, but I've not > been able to measure any added startup cost from this anyway. > > Thanks! > > /Claes
Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData
Hi Claes,
Nice play with CHM and safe publication.
If findSpecies() is on a hot concurrent path, you might want to wrap the
preface:
176 S speciesData = cache.computeIfAbsent(key, new Function<>() {
177 @Override
178 public S apply(K key1) {
179 return newSpeciesData(key1);
180 }
181 });
with a simple:
S speciesData = cache.get(key);
if (speciesData == null) {
speciesData = cache.computeIfAbsent();
}
or even use putIfAbsent instead if redundant concurrent newSpeciesData
is OK.
This way you can avoid hot-path synchronization with locks that CHM
always performs in computeIfAbsent.
Regards, Peter
On 04/24/18 18:57, Claes Redestad wrote:
Hi,
the current implementation of ClassSpecializer.findSpecies may cause
excessive blocking due to a potentially expensive computeIfAbsent, and
we have reason to believe this might have been cause for a few very
rare bootstrap issues in tests that attach debuggers to VM in the middle
of this.
Breaking this apart so that code generation and linking is done outside
of the computeIfAbsent helps minimize the risk that a thread gets
interrupted by a debugger at a critical point in time, while also
removing
a few limitations on what we can do from a findSpecies, e.g., look up
other
SpeciesData.
Bug: https://bugs.openjdk.java.net/browse/JDK-8202184
Webrev: http://cr.openjdk.java.net/~redestad/8202184/open.00/
This implementation inserts a placeholder, unresolved SpeciesData,
and then replaces it with another instance that is linked together
fully before publishing it, which ensure safe publication. There might
be alternatives involving volatiles and/or careful fencing, but I've not
been able to measure any added startup cost from this anyway.
Thanks!
/Claes
Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData
Hi Peter, On 2018-04-25 08:36, Peter Levart wrote: Hi Claes, Nice play with CHM and safe publication. thanks, I was curious how you'd react to this. :-) If findSpecies() is on a hot concurrent path, [...] It'd be surprising if it was: findSpecies is typically called once for a specific SpeciesData, and sometimes every now and then during setup of certain method handles (in particular the static speciesData_L* methods in jli.BoundMethodHandle are begging to be turned into lazy constants). Besides, CHM.computeIfAbsent has a non-synchronizing fast-path for when the key exists, lines 1731-1734: else if (fh == h // check first node without acquiring lock && ((fk = f.key) == key || (fk != null && key.equals(fk))) && (fv = f.val) != null) return fv; .. so I'm not sure we'd gain much from wrapping the preface with a get even if it was hot and contended. /Claes
Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData
Hi Paul, On 2018-04-24 21:01, Paul Sandoz wrote: Hi, This looks good. thanks! Took me a while to understand the interactions: you need to replace not update otherwise there is a race on isResolved (which currently queries multiple state, there is no singular guard here). We could push isResolved into the synchronized block and simplify but every findSpecies call may take a small hit (or there are potentially other ways looking more closely at how ClassSpecializer.Factory initializes state, i.e. a fenced static field write, but we go further down the rabbit hole) Right, I've been down that hole unsuccessfully and came up with this 'play' (a much nicer word than 'hack'; thanks Peter!) as a means to escape it. :-) I think this might fix some rare and intermittent recursive exceptions from ConcurrentHashMap cache we have been observing, where a collision occurs on keys for recursive updates (rather than for the same key). I think so, too, but as I've not been able to reproduce any of these rare intermittent issues locally I've no real evidence to that effect. /Claes
Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData
On 04/25/2018 10:06 AM, Claes Redestad wrote: Besides, CHM.computeIfAbsent has a non-synchronizing fast-path for when the key exists, lines 1731-1734: else if (fh == h // check first node without acquiring lock && ((fk = f.key) == key || (fk != null && key.equals(fk))) && (fv = f.val) != null) return fv; Sorry, you're (almost) right! I confused it with CHM.compute()... The almost part is that lock is avoided only when the match is found in the 1st linked node of the bucket. If there is a hash collision (very unlikely) and the entry is in a 2nd or subsequent node in the list, the lock is still used. So there's almost no locks used... And if there's no hot contention going on, there's no need for prefacing with .get(). Regards, Peter
Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData
Hi Claes, Your patch is OK from logical point of view, but something bothers me a little. For each species data that gets cached, at least 3 objects are created: - the Function passed to computeIfAbsent - the unresolved SpeciesData object that serves as a placeholder - the 2nd resolved SpeciesData object that replaces the 1st The 1st SpeciesData object serves just as placeholder, so it could be lighter-weight. An java.lang.Object perhaps? Creation of Function object could be eliminated by using putIfAbsent instead of computeIfAbsent then. For the price of some unsafe casts that are contained to a single method that is the sole user of CHM cache. For example, like this: http://cr.openjdk.java.net/~plevart/jdk-dev/8202184_ClassSpecializer/webrev.01/ I won't oppose to your version if you find it easier to understand. I just hat to try to do it without redundant creation of placeholder SpeciesData object. What do you think? Regards, Peter On 04/24/2018 06:57 PM, Claes Redestad wrote: Hi, the current implementation of ClassSpecializer.findSpecies may cause excessive blocking due to a potentially expensive computeIfAbsent, and we have reason to believe this might have been cause for a few very rare bootstrap issues in tests that attach debuggers to VM in the middle of this. Breaking this apart so that code generation and linking is done outside of the computeIfAbsent helps minimize the risk that a thread gets interrupted by a debugger at a critical point in time, while also removing a few limitations on what we can do from a findSpecies, e.g., look up other SpeciesData. Bug: https://bugs.openjdk.java.net/browse/JDK-8202184 Webrev: http://cr.openjdk.java.net/~redestad/8202184/open.00/ This implementation inserts a placeholder, unresolved SpeciesData, and then replaces it with another instance that is linked together fully before publishing it, which ensure safe publication. There might be alternatives involving volatiles and/or careful fencing, but I've not been able to measure any added startup cost from this anyway. Thanks! /Claes
Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData
Hi Peter, too late! :-) Good observation about us creating a new Function every time we look up an item. If we go with an Object as a reservation marker we could get away with a singleton static Function and still use computeIfAbsent (which I think is clearer and avoids creating a reservation Object on every lookup). I'd consider this a reasonable follow-up RFE: http://cr.openjdk.java.net/~redestad/scratch/ClassSpec_followup.00/ Its value as an optimization might be somewhat dubious, but it does help future-proof the mechanism (say if we wanted to add forms that are even more heavyweight or if we needed to side-effect something when calling newSpeciesData...). /Claes On 2018-04-25 18:45, Peter Levart wrote: Hi Claes, Your patch is OK from logical point of view, but something bothers me a little. For each species data that gets cached, at least 3 objects are created: - the Function passed to computeIfAbsent - the unresolved SpeciesData object that serves as a placeholder - the 2nd resolved SpeciesData object that replaces the 1st The 1st SpeciesData object serves just as placeholder, so it could be lighter-weight. An java.lang.Object perhaps? Creation of Function object could be eliminated by using putIfAbsent instead of computeIfAbsent then. For the price of some unsafe casts that are contained to a single method that is the sole user of CHM cache. For example, like this: http://cr.openjdk.java.net/~plevart/jdk-dev/8202184_ClassSpecializer/webrev.01/ I won't oppose to your version if you find it easier to understand. I just hat to try to do it without redundant creation of placeholder SpeciesData object. What do you think? Regards, Peter On 04/24/2018 06:57 PM, Claes Redestad wrote: Hi, the current implementation of ClassSpecializer.findSpecies may cause excessive blocking due to a potentially expensive computeIfAbsent, and we have reason to believe this might have been cause for a few very rare bootstrap issues in tests that attach debuggers to VM in the middle of this. Breaking this apart so that code generation and linking is done outside of the computeIfAbsent helps minimize the risk that a thread gets interrupted by a debugger at a critical point in time, while also removing a few limitations on what we can do from a findSpecies, e.g., look up other SpeciesData. Bug: https://bugs.openjdk.java.net/browse/JDK-8202184 Webrev: http://cr.openjdk.java.net/~redestad/8202184/open.00/ This implementation inserts a placeholder, unresolved SpeciesData, and then replaces it with another instance that is linked together fully before publishing it, which ensure safe publication. There might be alternatives involving volatiles and/or careful fencing, but I've not been able to measure any added startup cost from this anyway. Thanks! /Claes
