Комментарии 24
Как-раз пару дней назад похожую задачу на собеседовании решал.
Это всё хорошо, но зачем очередной велосипед при наличии класса Cache в Google Guava, который делает ровно всё то же самое?
1. именование у вас не единообразно — где-то «currentTimeMillis», где-то «default_timeout».
2. ну и оно у вас вообще компилируется?? В имени класса выпишете, через «с», а в конструкторе через «s»:
public class Caсhe<K, V> {
…
public Cashe(long default_timeout) throws Exception {
3. Странный у вас hashCode(). В чем смысл того, что вы прибавляете «43*7» к hashCode() ключа?
public int hashCode() {
int hash = 7;
hash = 43 * hash + (this.key != null? this.key.hashCode(): 0);
return hash;
}
2. ну и оно у вас вообще компилируется?? В имени класса выпишете, через «с», а в конструкторе через «s»:
public class Caсhe<K, V> {
…
public Cashe(long default_timeout) throws Exception {
3. Странный у вас hashCode(). В чем смысл того, что вы прибавляете «43*7» к hashCode() ключа?
public int hashCode() {
int hash = 7;
hash = 43 * hash + (this.key != null? this.key.hashCode(): 0);
return hash;
}
НЛО прилетело и опубликовало эту надпись здесь
Свежедобавленный элемент может быть тут же удален вследствие race condition между добавляющим и чистящим потоками, плохо!
Дополню другими замечаниями.
Зачем здесь параметр default_timeout передавать в конструктор? Создайте для Key конструктор с одним аргументом key и используйте его. Всё равно же сравнения не по таймауту делаются. И вообще как-то на каждый get по объекту создавать — не самая лучшая идея…
Дублирование кода в методах put, setAll и addAll.
Для методов
не нужно писать один и тот же код в реализации. Просто из первого метода вызывайте второй — иначе зачем вам жуткий копипаст? Для put и addAll аналогично.
public V get(K key)
{
return globalMap.get(new Key(key, default_timeout));
}
Зачем здесь параметр default_timeout передавать в конструктор? Создайте для Key конструктор с одним аргументом key и используйте его. Всё равно же сравнения не по таймауту делаются. И вообще как-то на каждый get по объекту создавать — не самая лучшая идея…
Дублирование кода в методах put, setAll и addAll.
Для методов
public void setAll(Map<K, V> map) ...
public void setAll(Map<K, V> map, long timeout) ...
не нужно писать один и тот же код в реализации. Просто из первого метода вызывайте второй — иначе зачем вам жуткий копипаст? Для put и addAll аналогично.
Вообще сама идея сделать кеш с указываемым временем жизни для каждого объекта кеша малоудачна.
Пример:
Делаем default_timeout 3600000 (1 час)
Добавляем put(«key», 60000); // 1 минута
Но даже через 10 минут скорее всего данные будут жить, так как проверка проводится 1 раз в 20 минут.
Выход: добавить параметр shrinkerWorkInterval (как в JCS) или сделать время жизни всех ключей одинаковым (как в Guava)
Кроме того не радуют вызов
Создание нового объекта при каждом получении объекта из кеша? Выше уже посоветовали хранить данные в ConcurrentHashMap и отдельно данные о времени жизни в другой коллекции.
Пример:
Делаем default_timeout 3600000 (1 час)
Добавляем put(«key», 60000); // 1 минута
Но даже через 10 минут скорее всего данные будут жить, так как проверка проводится 1 раз в 20 минут.
Выход: добавить параметр shrinkerWorkInterval (как в JCS) или сделать время жизни всех ключей одинаковым (как в Guava)
Кроме того не радуют вызов
return globalMap.get(new Key(key, default_timeout)); Создание нового объекта при каждом получении объекта из кеша? Выше уже посоветовали хранить данные в ConcurrentHashMap и отдельно данные о времени жизни в другой коллекции.
Как по мне — поток проверки совершенно лишний.
Можно просто переопределить get и в момент запроса просто сравнивать время в ключе с текущим. Даже удалять не надо — просто возвращаете null, если time-to-live уже прошел.
Можно просто переопределить get и в момент запроса просто сравнивать время в ключе с текущим. Даже удалять не надо — просто возвращаете null, если time-to-live уже прошел.
И не запрашиваемые данные будут висеть в кеше пока уборщица не выдернет кабель питания?
Ну, удаляйте. Это уже ваше дело как себя вести с этими объектами. Может у вас будет обновление кеша по тем же ключам. Тогда удалять смысла нет — только лишняя операция.
Суть моего коммента была в том, что демон немножко бесполезен)
Суть моего коммента была в том, что демон немножко бесполезен)
На это каждый смотрит со своей колокольни. Если в кеш будет вставка во много потоков больших объектов и обращение к этим объектам будет идти раз в пятилетку, то очень скоро мы получим OutOfMemory. А с демоном, если обращений совсем не будет то через некоторое время CHM станет пустой.
Согласен, что надо смотреть под конкретную ситуацию, чтобы соблюсти баланс между потребляемой памятью и операциями над этим всем.
Не согласен про раз в пятилетку. На то он и кеш, что чтение многократно преобладает над изменением. Иначе — зачем он тогда вообще нужен.
Не согласен про раз в пятилетку. На то он и кеш, что чтение многократно преобладает над изменением. Иначе — зачем он тогда вообще нужен.
Так этож кэш. Тут можно и SoftReference заюзать, что бы oome не получть*
Можно, но его уже нельзя будет назвать с определенным временем хранения. Не будет никакой гарантии, что объект пролежит в кеше те же 10 мин. т.к. при нехватки памяти GC их подметет.
Я считаю, что плох тот кэш, благодаря которому, можно OOME словить. Хотя, думаю, в любом случае, до OOME далеко. До него еще будут несколько сборок мусора и тормоза такие (если хип у нас достаточно большой), что польза от данного кэша быстро убежит в минус бесконечности.
Одним словом я — за стабильность. Поэтому лучше кэш весь слить, нежели вообще сервак положить. Если слив кэша равносилен тому, что через несколько мгновений все ляжет и так — то надо думать как исправить эту ситуацию.
Одним словом я — за стабильность. Поэтому лучше кэш весь слить, нежели вообще сервак положить. Если слив кэша равносилен тому, что через несколько мгновений все ляжет и так — то надо думать как исправить эту ситуацию.
Лучше вместо того чтобы поток ходил по всем элементам в фоне, сделать кучу по времени жизни, и засыпать до момента когда нужно будет изъять ближайший.
При вставке элемента посылать потоку-уборщику interrupt(чтобы узнать что в вершине кучи мог появится элемент который нужно удалить раньше).
При вставке элемента посылать потоку-уборщику interrupt(чтобы узнать что в вершине кучи мог появится элемент который нужно удалить раньше).
Зарегистрируйтесь на Хабре, чтобы оставить комментарий
Пишем кеш с определенным временем хранения объектов с использованием java.util.concurrent