Closed paulushub closed 5 years ago
Hey @paulushub thx for pointing this out, it is definitely a typo. Indeed they would be interpreted as common methods and not as constructors in this case:
val cache = CRSCache() // default one would be called, none of implemented
cache.CRSCache() // valid call
cache.CRSCache(map1, map2) // valid call
@pomadchin Thanks for the quick response. The class needs some review.
crsCache
and epsgCache
are created when defined, so should not be created again in the first constructor.
private ConcurrentHashMap<String, CoordinateReferenceSystem> crsCache = new ConcurrentHashMap<>();
private ConcurrentHashMap<String, String> epsgCache = new ConcurrentHashMap<>();
computeIfAbsent
method seem redundant, since the codes before calling that method already checks for the absence of the key.
public CoordinateReferenceSystem createFromName(String name)
throws UnsupportedParameterException, InvalidValueException, UnknownAuthorityCodeException {
CoordinateReferenceSystem res = crsCache.get(name);
if(res != null) return res;
return crsCache.computeIfAbsent(name, k -> crsFactory.createFromName(name));
}
@paulushub
computeIfAbsent
looks like this (Java 8 version, and we're aiming Java 8): public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
if (key == null || mappingFunction == null)
throw new NullPointerException();
int h = spread(key.hashCode());
V val = null;
int binCount = 0;
for (Node<K,V>[] tab = table;;) {
Node<K,V> f; int n, i, fh;
if (tab == null || (n = tab.length) == 0)
tab = initTable();
else if ((f = tabAt(tab, i = (n - 1) & h)) == null) {
Node<K,V> r = new ReservationNode<K,V>();
synchronized (r) {
if (casTabAt(tab, i, null, r)) {
binCount = 1;
Node<K,V> node = null;
try {
if ((val = mappingFunction.apply(key)) != null)
node = new Node<K,V>(h, key, val, null);
} finally {
setTabAt(tab, i, node);
}
}
}
if (binCount != 0)
break;
}
else if ((fh = f.hash) == MOVED)
tab = helpTransfer(tab, f);
else {
boolean added = false;
synchronized (f) {
if (tabAt(tab, i) == f) {
if (fh >= 0) {
binCount = 1;
for (Node<K,V> e = f;; ++binCount) {
K ek; V ev;
if (e.hash == h &&
((ek = e.key) == key ||
(ek != null && key.equals(ek)))) {
val = e.val;
break;
}
Node<K,V> pred = e;
if ((e = e.next) == null) {
if ((val = mappingFunction.apply(key)) != null) {
added = true;
pred.next = new Node<K,V>(h, key, val, null);
}
break;
}
}
}
else if (f instanceof TreeBin) {
binCount = 2;
TreeBin<K,V> t = (TreeBin<K,V>)f;
TreeNode<K,V> r, p;
if ((r = t.root) != null &&
(p = r.findTreeNode(h, key, null)) != null)
val = p.val;
else if ((val = mappingFunction.apply(key)) != null) {
added = true;
t.putTreeVal(h, key, val);
}
}
}
}
if (binCount != 0) {
if (binCount >= TREEIFY_THRESHOLD)
treeifyBin(tab, i);
if (!added)
return val;
break;
}
}
}
if (val != null)
addCount(1L, binCount);
return val;
}
@paulushub I benchmarked without crsCache.get(name)
and it is a bit slower:
[info] Benchmark Mode Cnt Score Error Units
[info] CRSBench.getEpsgCodeTest avgt 3 0.099 ± 0.276 us/op
[info] CRSBench.logicalEqualsFalse avgt 3 0.043 ± 0.030 us/op
[info] CRSBench.logicalEqualsTrue avgt 3 0.038 ± 0.040 us/op
[info] CRSBench.logicalVarEqualsFalse avgt 3 0.043 ± 0.030 us/op
[info] CRSBench.logicalVarEqualsTrue avgt 3 0.038 ± 0.005 us/op
[info] CRSBench.resolveCRS avgt 3 0.064 ± 0.251 us/op
Having used crsCache.get(name), do you still need computeIfAbsent and why?
@paulushub it probably could be putIfAbsent
or just put
; I used computeIfAbsent
to simplify signatures (Java8 has a poor syntax, and it is the only way I could use lambdas to insert values), and there would be no performance overhead in it due to the get
check.
Also computeIfAbsent
returns the current value associated with key, and put
functions return the previous value; in other words:
ConcurrentHashMap<String, String> cache = new ConcurrentHashMap<>();
cache.putIfAbsent("str", "value"); //> returns null
cache.computeIfAbsent("str1", k -> "value"); //> returns "value"
The following methods in CRSCache are having the same name as the class, are these constructors returning a result? I do not know how Java pulls this, but this will not work in many languages!