6

I have the below code: 

import java.math.BigInteger;
import java.util.HashMap;
import java.util.Map;

public class DynamicFib
{
    private static Map<Integer, BigInteger> myMap = new HashMap<>();

    static {
        myMap.put(0, BigInteger.ZERO); //fibonacci(0)
        myMap.put(1, BigInteger.ONE); //fibonacci(1)
    }

    public static BigInteger fibonacci(int x)
    {
//        System.out.println("x = [" + x + "]");
        return myMap.computeIfAbsent(x, n -> fibonacci(n - 2).add(fibonacci(n - 1)));
    }

    public static void main(String[] args)
    {

        System.out.println("l = " + fibonacci(25));
        System.out.println("myMap = " + myMap);
        System.out.println("myMap = " + myMap.keySet().size());

    }

}

console output:

l = 75025

myMap = {0=0, 1=1, 2=1, 3=2, 4=3, 5=5, 6=8, 7=13, 8=21, 9=34, 10=55, 11=89, 12=144, 13=233, 14=377, 15=610, 16=987, 17=1597, 18=2584, 19=4181, 20=6765, 21=10946, 22=17711, 23=28657, 24=46368}

myMap = 31

memo has just 25 elements but size returns 31. How? is it a bug in hash map implementation?

I changed hashmap to ConcurrentHashMap, it simply hangs if I ask for 9th or more fibonacci number.

But this works and returns fibanocci number correctly even for 1000!

Employee
  • 3,109
  • 5
  • 31
  • 50
Vijayakumar Chava
  • 69
  • 1
  • 5
  • I am getting a `java.util.ConcurrentModificationException` upon running this code. what JDK version are you running on? – Ousmane D. Jan 01 '19 at 20:57
  • I'm getting the same on JDK 11. – Joe C Jan 01 '19 at 20:58
  • 1
    I also tried with a `ConcurrentHashMap`, and it threw a specific error (`java.lang.IllegalStateException: Recursive update`) which suggests you really shouldn't do this. – Joe C Jan 01 '19 at 21:00
  • whether you're doing this for fun or not it's overcomplicating things, you can create a method to generate Fibonacci numbers recursively with better readability and efficiency. – Ousmane D. Jan 01 '19 at 21:02
  • 11
    The javadoc for [HashMap#computeIfAbsent](https://docs.oracle.com/javase/9/docs/api/java/util/HashMap.html#computeIfAbsent-K-java.util.function.Function-) has a special warning about this: **The mapping function should not modify this map during computation.** – Thomas Kläger Jan 01 '19 at 21:07
  • I am using jdk 8 and it looks like the issue is addressed in jdk 9. – Vijayakumar Chava Jan 01 '19 at 21:07
  • 2
    @Aomine you can do it trivially with a loop. No need even for recursion. – Andy Turner Jan 01 '19 at 21:16
  • @AndyTurner yes definitely. I just thought the OP wanted a recursive approach. – Ousmane D. Jan 01 '19 at 21:18
  • 4
    @Aomine OP *is* using a recursive approach (method `fibonacci` is calling itself twice), but attempted to add [memoization](https://en.wikipedia.org/wiki/Memoization) for improved performance. – Andreas Jan 01 '19 at 21:19

1 Answers1

4

Yes, the HashMap.computeIfAbsent(K, Function) Javadoc notes that you should not modify the map during computation. If you modify the method to first check if the map contains x as a key and then return like

public static BigInteger fibonacci(int x) {
    if (!myMap.containsKey(x)) {
        myMap.put(x, fibonacci(x - 2).add(fibonacci(x - 1)));
    }
    return myMap.get(x);
}

Then you would see (as I think you expected)

l = 75025
myMap = {0=0, 1=1, 2=1, 3=2, 4=3, 5=5, 6=8, 7=13, 8=21, 9=34, 10=55, 11=89, 12=144, 13=233, 14=377, 15=610, 16=987, 17=1597, 18=2584, 19=4181, 20=6765, 21=10946, 22=17711, 23=28657, 24=46368, 25=75025}
myMap = 26
Elliott Frisch
  • 198,278
  • 20
  • 158
  • 249