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
\$\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\$
  • \$\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\$ – gstackoverflow Jan 26 '17 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 Jan 26 '17 at 7:07
  • \$\begingroup\$ please provide proov about twice computation. computeIfAbsent uses synchronize inside \$\endgroup\$ – gstackoverflow Jan 26 '17 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 Jan 26 '17 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\$ – gstackoverflow Jan 26 '17 at 7:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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