From f8cb33fb8c18334fe94b215463afb9ecaa09fc92 Mon Sep 17 00:00:00 2001 From: Ahmet Mircik Date: Mon, 3 May 2021 23:31:18 +0300 Subject: [PATCH] Fix lru eviction algorithm --- .../LRUEvictionPolicyComparator.java | 7 +- .../impl/recordstore/AbstractRecordStore.java | 1 + .../eviction/EvictionPolicyEvaluatorTest.java | 15 +- .../LRUEvictionPolicyComparatorTest.java | 149 ++++++++++++++++++ 4 files changed, 164 insertions(+), 8 deletions(-) create mode 100644 hazelcast/src/test/java/com/hazelcast/internal/eviction/impl/comparator/LRUEvictionPolicyComparatorTest.java diff --git a/hazelcast/src/main/java/com/hazelcast/internal/eviction/impl/comparator/LRUEvictionPolicyComparator.java b/hazelcast/src/main/java/com/hazelcast/internal/eviction/impl/comparator/LRUEvictionPolicyComparator.java index 49e6e543a6e21..0d7a2b26aba49 100644 --- a/hazelcast/src/main/java/com/hazelcast/internal/eviction/impl/comparator/LRUEvictionPolicyComparator.java +++ b/hazelcast/src/main/java/com/hazelcast/internal/eviction/impl/comparator/LRUEvictionPolicyComparator.java @@ -33,9 +33,10 @@ public class LRUEvictionPolicyComparator @Override public int compare(EvictableEntryView e1, EvictableEntryView e2) { - int result = Long.compare(e1.getLastAccessTime(), e2.getLastAccessTime()); - // if access times are same, we try to select oldest entry to evict - return result == 0 ? Long.compare(e1.getCreationTime(), e2.getCreationTime()) : result; + long time1 = Math.max(e1.getCreationTime(), e1.getLastAccessTime()); + long time2 = Math.max(e2.getCreationTime(), e2.getLastAccessTime()); + + return Long.compare(time1, time2); } @Override diff --git a/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/AbstractRecordStore.java b/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/AbstractRecordStore.java index 2b7b8301cb58f..804d2198c0f5c 100644 --- a/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/AbstractRecordStore.java +++ b/hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/AbstractRecordStore.java @@ -142,6 +142,7 @@ public Record createRecord(Object value, long ttlMillis, long maxIdle, long now) Record record = recordFactory.newRecord(value); record.setCreationTime(now); record.setLastUpdateTime(now); + record.setLastAccessTime(now); updateStatsOnPut(false, now); return record; diff --git a/hazelcast/src/test/java/com/hazelcast/internal/eviction/EvictionPolicyEvaluatorTest.java b/hazelcast/src/test/java/com/hazelcast/internal/eviction/EvictionPolicyEvaluatorTest.java index d52e743fa9c45..299d1dd51466a 100644 --- a/hazelcast/src/test/java/com/hazelcast/internal/eviction/EvictionPolicyEvaluatorTest.java +++ b/hazelcast/src/test/java/com/hazelcast/internal/eviction/EvictionPolicyEvaluatorTest.java @@ -125,20 +125,25 @@ public EvictionPolicyComparator getComparator() { = new ArrayList>(); long baseTime = System.currentTimeMillis(); - + long minCreationTime = -1; for (int i = 0; i < recordCount; i++) { long creationTime = baseTime + (i * 100); + minCreationTime = minCreationTime == -1 ? creationTime : Math.min(creationTime, minCreationTime); + CacheObjectRecord record = new CacheObjectRecord(i, creationTime, Long.MAX_VALUE); if (i == expectedEvictedRecordValue) { - // The record in the middle will be minimum access time. - // So, it will be selected for eviction - record.setLastAccessTime(baseTime - 1000); + // The record in the middle will be minimum access + // time. So, it will be selected for eviction + // (set creation-time also to keep this condition + // true --> creation-time <= last-access-time) + record.setCreationTime(minCreationTime); + record.setLastAccessTime(minCreationTime + 1); } else if (i == expectedExpiredRecordValue) { record.setExpirationTime(System.currentTimeMillis()); } else { record.setLastAccessTime(creationTime + 1000); } - records.add(new SimpleEvictionCandidate(i, record)); + records.add(new SimpleEvictionCandidate<>(i, record)); } sleepAtLeastMillis(1); diff --git a/hazelcast/src/test/java/com/hazelcast/internal/eviction/impl/comparator/LRUEvictionPolicyComparatorTest.java b/hazelcast/src/test/java/com/hazelcast/internal/eviction/impl/comparator/LRUEvictionPolicyComparatorTest.java new file mode 100644 index 0000000000000..133b21eb8ea23 --- /dev/null +++ b/hazelcast/src/test/java/com/hazelcast/internal/eviction/impl/comparator/LRUEvictionPolicyComparatorTest.java @@ -0,0 +1,149 @@ +/* + * Copyright (c) 2008-2021, Hazelcast, Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.hazelcast.internal.eviction.impl.comparator; + +import com.hazelcast.spi.eviction.EvictableEntryView; +import com.hazelcast.test.HazelcastSerialClassRunner; +import com.hazelcast.test.annotation.QuickTest; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.runner.RunWith; + +import java.util.Collections; +import java.util.LinkedList; +import java.util.List; + +import static org.junit.Assert.assertEquals; + +@RunWith(HazelcastSerialClassRunner.class) +@Category(QuickTest.class) +public class LRUEvictionPolicyComparatorTest { + + private static final long NOW = 20; + + @Test + public void lru_comparator_does_not_prematurely_select_newly_created_entries() { + // 0. Entries to sort + List givenEntries = new LinkedList<>(); + givenEntries.add(new TestEntryView(1, 1, 0)); + givenEntries.add(new TestEntryView(2, 2, 3)); + givenEntries.add(new TestEntryView(3, 2, 0)); + givenEntries.add(new TestEntryView(4, 4, 4)); + givenEntries.add(new TestEntryView(5, 5, 20)); + givenEntries.add(new TestEntryView(6, 6, 6)); + givenEntries.add(new TestEntryView(7, 7, 0)); + givenEntries.add(new TestEntryView(8, 9, 15)); + givenEntries.add(new TestEntryView(9, 10, 10)); + givenEntries.add(new TestEntryView(10, 10, 0)); + + // 1. Create expected list of ordered elements by + // sorting entries based on their idle-times. Longest + // idle time must be the first element of the list. + List descOrderByIdleTimes = new LinkedList<>(givenEntries); + Collections.sort(descOrderByIdleTimes, (o1, o2) -> -Long.compare(idleTime(o1), idleTime(o2))); + + // 2. Then sort given entries by using LRU eviction comparator. + Collections.sort(givenEntries, (o1, o2) -> LRUEvictionPolicyComparator.INSTANCE.compare(o1, o2)); + + // 3. Check both lists are equal + assertEquals(descOrderByIdleTimes, givenEntries); + } + + private static long idleTime(EvictableEntryView entryView) { + return NOW - Math.max(entryView.getCreationTime(), entryView.getLastAccessTime()); + } + + private static class TestEntryView implements EvictableEntryView { + private long id; + private long creationTime; + private long lastAccessTime; + + TestEntryView(long id, long creationTime, long lastAccessTime) { + this.id = id; + this.creationTime = creationTime; + this.lastAccessTime = lastAccessTime; + } + + @Override + public long getCreationTime() { + return creationTime; + } + + @Override + public long getLastAccessTime() { + return lastAccessTime; + } + + public long getId() { + return id; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + + if (o == null || getClass() != o.getClass()) { + return false; + } + + TestEntryView that = (TestEntryView) o; + + if (id != that.id) { + return false; + } + if (creationTime != that.creationTime) { + return false; + } + return lastAccessTime == that.lastAccessTime; + } + + @Override + public int hashCode() { + int result = (int) (id ^ (id >>> 32)); + result = 31 * result + (int) (creationTime ^ (creationTime >>> 32)); + result = 31 * result + (int) (lastAccessTime ^ (lastAccessTime >>> 32)); + return result; + } + + @Override + public String toString() { + return "TestEntryView{" + + "id=" + id + + ", now=" + NOW + + ", creationTime=" + creationTime + + ", lastAccessTime=" + lastAccessTime + + ", idleTime=" + idleTime(this) + '}'; + } + + @Override + public Object getKey() { + throw new UnsupportedOperationException(); + } + + @Override + public Object getValue() { + throw new UnsupportedOperationException(); + } + + @Override + public long getHits() { + throw new UnsupportedOperationException(); + } + } +}