0
\$\begingroup\$

I need to review this code:

import java.util.HashMap;
import java.util.Map;

// REVIEW
public abstract class Digest {
    private Map<String, byte[]> cache = new HashMap<>();

    public byte[] digest(String input) {
        byte[] result = cache.get(input);
        if (result == null) {
            synchronized (cache) {
                result = cache.get(input);
                if (result == null) {
                    result = doDigest(input);
                    cache.put(input, result);
                }
            }
        }
        return result;
    }

    protected abstract byte[] doDigest(String input);
}
  1. Should I mark cache as volatile ?
    I have doubts because we use cashe.get in method body instead of direct cashe

  2. Would it make sense to replace the map with ConcurrentHashMap?

  3. Is there way to fix code without replacing map implementation?
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Volatile won't do anything here, as this basically only affects the reference to the map, not the contents of the map.

As for ConcurrentHashMap (as opposed to explicit synchronizing I suppose): this depends. If you want to make absolutely sure that no digest is ever computed twice, stick to your approach. If you just want general concurrency, but can accept that eventually in race-conditions, digest is called twice for the same input (sidenote: I suppose digest is deterministic, so that two calls with the same input yield the same result), you may simply repace the map with a ConcurrentHashMap and the whole method with:

return cache.computeIfAbsent(input, this::doDigest);
\$\endgroup\$
11
  • \$\begingroup\$ 1.I believe that this computeIfAbsent will execute doDigest only once. 2. Is there way to fix this code without change map implementation? \$\endgroup\$ Commented Jan 26, 2017 at 7:00
  • \$\begingroup\$ If you are in a multithreaded environment, computeIfAbsent may call the computation twice, if a second thread requests the same data before the first thread has completed the computation. Also, for multithreaded caching, you'll have to synchronize / block somewhere. Generally, you are mostly better off when you leave this to the standard library than implementing this yourself. \$\endgroup\$
    – mtj
    Commented Jan 26, 2017 at 7:07
  • \$\begingroup\$ please provide proov about twice computation. computeIfAbsent uses synchronize inside \$\endgroup\$ Commented Jan 26, 2017 at 7:10
  • \$\begingroup\$ The synchronization is on the new node which in turn can be created by the unsafe class. This is really hard to read and I may be mistaken here, but I think that this may be different node objects for concurrent calls with the same keys. However, as this is your problem, I suggest you simply write some test code to verify or proof me wrong. And if I really misunderstood the code, this is even better as it solves your problem completly with a single line of code. :) \$\endgroup\$
    – mtj
    Commented Jan 26, 2017 at 7:14
  • \$\begingroup\$ Ok, but I really want to know how to solve problem without CHM. I just want to know it in educational purposes. \$\endgroup\$ Commented Jan 26, 2017 at 7:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.