snippetjavaMinor
How to improve this session cache to search faster
Viewed 0 times
thissearchimprovefastercachesessionhow
Problem
I have this code which will store user session credentials:
```
import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
public class SessionCache
{
/**
* Default Constructor
*/
public SessionCache()
{
}
/**
* Map which which will store Connections Objects by connection_id
*/
private static final Map cache = new LinkedHashMap<>();
/**
* Object which will store connection credentials
*/
public class ActiveConnections
{
private int one;
private int two;
private int three;
public ActiveConnections(int one, int two, int three)
{
this.one = one;
this.two = two;
this.three = three;
}
public int getOne()
{
return one;
}
public void setOne(int one)
{
this.one = one;
}
public int getTwo()
{
return two;
}
public void setTwo(int two)
{
this.two = two;
}
public int getThree()
{
return three;
}
public void setThree(int three)
{
this.three = three;
}
}
/**
* Get Connection credentials by connection_id
*
* @param connection_id
* @return
*/
public Object getCache(String connection_id)
{
return cache.get(connection_id);
}
/**
* Insert new Connection Object into the cache
*
* @param connection_id
* @param one
* @param two
* @param three
*/
public void addCache(String connection_id, int one, int two, int three)
{
/**
* @ You may want to check if entry already exists for connection depends on logic in your application.
* @ Otherwise this will replace any previous entry for connection_id
*/
if (!cache.containsKey(connection_
```
import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
public class SessionCache
{
/**
* Default Constructor
*/
public SessionCache()
{
}
/**
* Map which which will store Connections Objects by connection_id
*/
private static final Map cache = new LinkedHashMap<>();
/**
* Object which will store connection credentials
*/
public class ActiveConnections
{
private int one;
private int two;
private int three;
public ActiveConnections(int one, int two, int three)
{
this.one = one;
this.two = two;
this.three = three;
}
public int getOne()
{
return one;
}
public void setOne(int one)
{
this.one = one;
}
public int getTwo()
{
return two;
}
public void setTwo(int two)
{
this.two = two;
}
public int getThree()
{
return three;
}
public void setThree(int three)
{
this.three = three;
}
}
/**
* Get Connection credentials by connection_id
*
* @param connection_id
* @return
*/
public Object getCache(String connection_id)
{
return cache.get(connection_id);
}
/**
* Insert new Connection Object into the cache
*
* @param connection_id
* @param one
* @param two
* @param three
*/
public void addCache(String connection_id, int one, int two, int three)
{
/**
* @ You may want to check if entry already exists for connection depends on logic in your application.
* @ Otherwise this will replace any previous entry for connection_id
*/
if (!cache.containsKey(connection_
Solution
- First you should rename your instance members in ActiveConnections class - having methods and variables named getOne(), getTwo(), etc, are horrible because at first you'd expect them to return the int value of one, two, three (e.g., returning 1, 2, 3) but in reality they most likely won't.
- You should either put a lock on your cache or use a
ConcurrentMapor you will sooner or later experience race condition errors.
- Your
sendmethods should be renamed togetbecause you are not sending them anywhere, you are returning them.
- You should consider setting a memory/element limit on your cache; ex. it should only hold 101 cached items and when this limit is reached you apply a Least Recently Used policy and remove the X amount of LRU cache items.
- Your
getCache(..)andaddCache(..)should be renamed because you are not getting nor adding a cache, but rather a cache item.
- Your
getCache(..)should have ActiveConnection as the return type
If you want a kind of Sliding Expiration policy where you remove items that are older than 5 seconds there are a few things to keep in mind:
- 5 seconds are a very short period of time; and
- You need a way to map a timestamp to a cache item. A very simple way of doing this is making a nested class, e.g., CacheItem, which only holds a timestamp and a value and your map will consist of a ``
edit
Here is an example of a very simple cache class I wrote in C# (just for reference). For the test cases I've ran it through it works fine, but you might want to check into existing cache libraries.
public class SimpleCache
{
public const int MAX_CACHE_SIZE = 101;
public long CacheExpirationTicks { get; set; }
private ConcurrentDictionary _cache = new ConcurrentDictionary();
public SimpleCache() { CacheExpirationTicks = 30000; /*default*/ }
public void Add(string key, object value)
{
//if the cache limit is reached we apply a LRU policy on the cache and remove half of the cache ( MAX_CACHE_SIZE / 2 ) least recently used
if (_cache.Count == MAX_CACHE_SIZE)
{
foreach (var cacheItem in _cache
.OrderByDescending(x => x.Value.LastAccessed)
.Skip(MAX_CACHE_SIZE / 2))
{
CacheItem ignored;
_cache.TryRemove(cacheItem.Key, out ignored);
}
}
_cache.TryAdd(key, new CacheItem() { Value = value, LastAccessed = DateTime.Now.Ticks });
}
public object this[string key]
{
get
{
if (_cache.ContainsKey(key))
{
if ((_cache[key].LastAccessed + CacheExpirationTicks) > DateTime.Now.Ticks)
{
return _cache[key].Value;
}
CacheItem ignored;
_cache.TryRemove(key, out ignored);
}
return null;
}
}
internal sealed class CacheItem
{
//despite of it's name, LastAccess will *AND SHOULD* only be set once. The cache will validate its item in the get and add function and remove them by itself
public long LastAccessed { get; set; }
public object Value { get; set; }
}
}And you would implement it like this
public class Program
{
public static SimpleCache simpleCache = new SimpleCache();
static void Main(string[] args)
{
DataTable dt = GetDataTableFromCSVFile("myCSV.csv");
}
private static DataTable GetDataTableFromCSVFile(string csv_file_path)
{
object obj;
if ((obj = simpleCache[csv_file_path]) != null)
{
return (DataTable)obj;
}
else
{
var csvData = ExpensiveOperation();
var key = csv_file_path;
simpleCache.Add(key, csvData);
return csvData;
}
}
}Code Snippets
public class SimpleCache
{
public const int MAX_CACHE_SIZE = 101;
public long CacheExpirationTicks { get; set; }
private ConcurrentDictionary<string, CacheItem> _cache = new ConcurrentDictionary<string, CacheItem>();
public SimpleCache() { CacheExpirationTicks = 30000; /*default*/ }
public void Add(string key, object value)
{
//if the cache limit is reached we apply a LRU policy on the cache and remove half of the cache ( MAX_CACHE_SIZE / 2 ) least recently used
if (_cache.Count == MAX_CACHE_SIZE)
{
foreach (var cacheItem in _cache
.OrderByDescending(x => x.Value.LastAccessed)
.Skip(MAX_CACHE_SIZE / 2))
{
CacheItem ignored;
_cache.TryRemove(cacheItem.Key, out ignored);
}
}
_cache.TryAdd(key, new CacheItem() { Value = value, LastAccessed = DateTime.Now.Ticks });
}
public object this[string key]
{
get
{
if (_cache.ContainsKey(key))
{
if ((_cache[key].LastAccessed + CacheExpirationTicks) > DateTime.Now.Ticks)
{
return _cache[key].Value;
}
CacheItem ignored;
_cache.TryRemove(key, out ignored);
}
return null;
}
}
internal sealed class CacheItem
{
//despite of it's name, LastAccess will *AND SHOULD* only be set once. The cache will validate its item in the get and add function and remove them by itself
public long LastAccessed { get; set; }
public object Value { get; set; }
}
}public class Program
{
public static SimpleCache simpleCache = new SimpleCache();
static void Main(string[] args)
{
DataTable dt = GetDataTableFromCSVFile("myCSV.csv");
}
private static DataTable GetDataTableFromCSVFile(string csv_file_path)
{
object obj;
if ((obj = simpleCache[csv_file_path]) != null)
{
return (DataTable)obj;
}
else
{
var csvData = ExpensiveOperation();
var key = csv_file_path;
simpleCache.Add(key, csvData);
return csvData;
}
}
}Context
StackExchange Code Review Q#36200, answer score: 8
Revisions (0)
No revisions yet.