Properly setting up a simple server-side cache

Question

I'm trying to set up a server-side cache properly and I'm looking for constructive criticism on the setup I have currently. The cache is loaded when the Servlet starts and never changed again, so in effect it's a read-only cache. It obviously needs to stay in memory for the lifetime of the Servlet. Here's how I have it set-up

private static List<ProductData> _cache;
private static ProductManager productManager;

private ProductManager() {
    try {
        lookup();
    } catch (Exception ex) {
        _cache = null;
    }
}

public synchronized static ProductManager getInstance() {
    if (productManager== null) {
        productManager= new ProductManager();
    }
    return productManager;
}

The cache is setup by the Servlet as below:

private ProductManager productManager;

public void init(ServletConfig config) throws ServletException {
    productManager = ProductManager.getInstance();
}

And finally, this is how I access it:

public static ProductData lookup(long id) throws Exception {
    if (_cache != null) {
        for (int i = 0; i < _cache.size(); i++) {
            if (_cache.get(i).id == id) {
                return _cache.get(i);
            }
        }
    }

    // Look it up in the DB.
}

public static List<ProductData> lookup() throws Exception {
    if (_cache != null) {
        return _cache;
    }

    // Read it from the DB.

    _cache = list;
    return list;
}

Answer 1

I would recommend you to make the cache as a hashmap :

private static HashMap<Long,ProductData> _cache = new HashMap<Long,ProductData>();


// lookup by id becomes
return _cache.get(id);

// lookup of the complete collection of ProductData :
return _cache.values();

You can make the cache a static field in the ProductData class so there is less coupling and moving parts.

With a hashmap looking up by id will be essentially constant time while the search with the for loop will increase when the number of ProductData increase over time.

Answer 2

A few things that spring to mind:

• Store your cached ProductData instances in a map so that you can look up a cached instance in constant time rather than having to search through a list.
• The lookup methods aren't thread-safe.
• Only lookup() will actually load the values from the database: don't you want the other lookup method to also load the cache (if not already loaded) to speed up retrieve of single ProductData instances?

Answer 3

Please look at Guava's Suppliers.memoizeWithExpiration() and MapMaker. Both of these (in your case I think MapMaker is more relevant) will let you create a caching function in a matter of minutes.

Save yourself some headache, use an API :)

Answer 4

You're doing it the hard way. A singleton-flavored pattern is completely unnecessary. Just implement a ServletContextListener to have a hook on the webapp startup (and shutdown), so that you can just load and store the data in the application scope during the webapp startup.

public class Config implements ServletContextListener {

    private static final String ATTRIBUTE_NAME = "com.example.Config";
    private Map<Long, Product> products;

    @Override
    public void contextInitialized(ServletContextEvent event) {
        ServletContext context = event.getServletContext();
        context.setAttribute(ATTRIBUTE_NAME, this);
        String dbname = context.getInitParameter("dbname");
        products = Database.getInstance(dbname).getProductDAO().map();
    }

    @Override
    public void contextDestroyed(ServletContextEvent event) {
        // NOOP.
    }

    public static Config getInstance(ServletContext context) {
        return (Config) context.getAttribute(ATTRIBUTE_NAME);
    }

    public Map<Long, Product> getProducts() {
        return products;
    }

}

Which you register in web.xml as follows:

<listener>
    <listener-class>com.example.Config</listener-class>
</listener>

This way you can get it in any servlet as follows:

Config config = Config.getInstance(getServletContext());
Map<Long, Product> products = config.getProducts();
// ...