HashMap не работает / проблемы с производительностью

В настоящее время у меня реализован HashMap, который

private static Map<String, Item> cached = new HashMap<String, Item>();

а Item — это объект со свойствами Date expireTime и byte[] data

Эта карта используется, когда несколько потоков одновременно начинают использовать это. Проверка, которую я делаю,

1.

public static final byte[] getCachedData(HttpServletRequest request) throws ServletException
{
    String url = getFullURL(request);
    Map<String, Item> cache = getCache(request);  // this chec
    Item item = null;

    synchronized (cache)
    {
        item = cache.get(url);
        if (null == item)
            return null;

        // Make sure that it is not over an hour old.
        if (item.expirationTime.getTime() < System.currentTimeMillis())
        {
            cache.remove(url);
            item = null;
        }
    }

    if (null == item)
    {
        log.info("Expiring Item: " + url);
        return null;
    }

    return item.data;
}

2. Если данные возвращаются нулевыми, то мы создаем и данные и кэшируем их в hashMap

public static void cacheDataX(HttpServletRequest request, byte[] data, Integer minutes) throws ServletException
{
    Item item = new Item(data);
    String url = getFullURL(request);
    Map<String, Item> cache = getCache(request);

    log.info("Caching Item: " + url + " - Bytes: " + data.length);
    synchronized (cache)
    {
        Calendar cal = Calendar.getInstance();
        cal.add(Calendar.MINUTE, minutes);
        item.expirationTime = cal.getTime();
        cache.put(url, item);
    }
}

Похоже, что если несколько потоков обращаются к ключу say (в данном случае URL-адресу), то данные добавляются в кеш более одного раза в одном и том же месте ключа [поскольку getCacheData вернет null для нескольких потоков, поскольку hashmap не закончил запись данных для первого потока]

Любые предложения о том, как решить проблему?


person Sushant Shah    schedule 07.07.2011    source источник
comment
Посмотрите на java.util.concurrent.ConcurrentHashMap; метод putIfAbsent может быть очень полезным.   -  person toto2    schedule 07.07.2011
comment
Каландр - очень тяжелый предмет. Если вы посмотрите на источник GregorianCalender, вы поймете, почему. Вместо этого попробуйте использовать System.currentTimeMillis().   -  person Peter Lawrey    schedule 07.07.2011


Ответы (2)


В cacheDataX добавьте проверку существования элемента перед добавлением (внутри синхронизированного блока).

synchronized (cache)
    {
        if (cache.get(url) == null) {
            Calendar cal = Calendar.getInstance();
            cal.add(Calendar.MINUTE, minutes);
            item.expirationTime = cal.getTime();
            cache.put(url, item);
        }
    }

Это гарантирует, что несколько потоков, которые уже выполнили поиск и вернули null, не смогут добавить одни и те же данные в кеш. Один добавит его, а другие потоки будут молча игнорировать, поскольку кеш уже обновлен.

person Robin    schedule 07.07.2011

Вам нужен один блок синхронизации, чтобы охватить как получение чего-либо из кеша, так и вставку в кеш. В исходном коде у вас есть состояние гонки: несколько потоков могут выполнить шаг 1 до того, как кто-либо выполнит шаг 2.

person Nathan Hughes    schedule 07.07.2011
comment
@jtoberon - это не имеет ничего общего с двойной проверкой блокировки. Это очень специфическая идиома, связанная с созданием объектов и ленивой инициализацией. - person Robin; 07.07.2011
comment
Я удалю свой предыдущий комментарий, потому что он был запутанным. Я имел в виду, что обычно вы должны сделать следующее в одном синхронизированном блоке, если хотите, чтобы была создана ровно одна вещь: (1) проверить ссылку, (2) если ноль, создать, (3) установить ссылку. Ни двойная проверка, ни размещенный код не делают этого должным образом. - person jtoberon; 07.07.2011