TilesetCache interface instead of java.util.Map<String, TileSet>

[sergsavchuk]

Hello again!
I submitted a pull request a while ago. And you left a nice suggestion in the comment. I soon implemented this, but forgot to submit a pull request, so here I am :)

Here is an example of interface implementation:

public class FutureTilesetCache implements TilesetCache
{
    private final Logger log = LoggerFactory.getLogger(FutureTilesetCache.class);

    private final Map<String, CompletableFuture<TileSet>> cachedTilesets = new ConcurrentHashMap<>();

    @Override
    public synchronized boolean needToLoadTileset(String tilesetName)
    {
        if (!cachedTilesets.containsKey(tilesetName))
        {
            log.info("Need to load " + tilesetName);
            cachedTilesets.put(tilesetName, new CompletableFuture<>());
            return true;
        }

        return false;
    }

    @Override
    public void tilesetLoadingFinished(TileSet loadedTileset)
    {
        if (cachedTilesets.get(loadedTileset.getName()) == null)
        {
            throw new RuntimeException("Use needToLoadTileset method before loading tileset");
        }
        else if (cachedTilesets.get(loadedTileset.getName()).isDone())
        {
            throw new RuntimeException("Shouldn't be called more than once");
        }

        log.info("Loaded " + loadedTileset.getName());
        cachedTilesets.get(loadedTileset.getName()).complete(loadedTileset);
    }

    @Override
    public TileSet getTileset(String tilesetName)
    {
        try
        {
            return cachedTilesets.get(tilesetName).get();
        }
        catch (InterruptedException | ExecutionException e)
        {
            throw new RuntimeException("Something went wrong", e);
        }
        catch (NullPointerException nullPointerException)
        {
            return null;
        }
    }
}

[bjorn]

Thanks a lot for making the pull request. Better late than never! :-)

I left one inline comment regarding a thought I had about the suggested caching API.

[bjorn]

Would it be reasonable to simplify this API so that the cache API is just getTileset(name, lambda), so that it would call the lambda function to load the tileset when it does not exist in the cache yet? I'm not sure what lambda functions would look like in Java, maybe:

TileSet tileSet = tilesetCache.getTileset(name, (Node t, String name) -> processTileset(t, name))

(and the lambda doesn't really need to capture the name, since it could be read again from the Node)

Or are there some downsides to this?

[bjorn]

Or, I guess this should actually look more like:

TileSet tileSet = tilesetCache.getTileset(name, () -> processTileset(t, name))

Since the t and name are local and can't be passed in again as parameters to the lambda. But I don't know how capturing of local values works in Java.

[sergsavchuk]

Now interface can be implemented something like this:

public class FutureTilesetCache implements TilesetCache
{
    private final Logger log = LoggerFactory.getLogger(FutureTilesetCache.class);

    private final Map<String, CompletableFuture<TileSet>> cachedTilesets = new ConcurrentHashMap<>();

    @Override
    public TileSet getTileset(String tilesetName, TilesetLoader tilesetLoader)
    {
        try
        {
            if (!cachedTilesets.containsKey(tilesetName))
            {
                if (tilesetLoader == null)
                {
                    return null;
                }

                cachedTilesets.put(tilesetName, new CompletableFuture<>());
                cachedTilesets.get(tilesetName).complete(tilesetLoader.load());
            }

            return
        catch (Exception e)
        {
            throw new RuntimeException("Something went wrong", e);
        }
    }
}

[bjorn]

Are you sure the implementation of the interface works like that? Since the getTileset function is not synchronized, there could be multiple threads determining that the cache does not contain a tileset with the given name that will then call tilesetLoader.load(), or not?

I've left two small inline comments.

[bjorn]

Now this name variable can be moved into the tilesetCache != null block, since it's not used otherwise.

[sergsavchuk]

👍

[bjorn]

How does this bare lambda function turn into a class that implements a load function? Is that just Java magic?

[sergsavchuk]

Yes, it's Java magic 🪄 .
In Java lambdas are simply implementations of single function interfaces.

[sergsavchuk]

Yes, you are right, the function itself should be synchronized to avoid synchro problems:

public synchronized TileSet getTileset(String tilesetName, TilesetLoader tilesetLoader)

[bjorn]

Yes, you are right, the function itself should be synchronized to avoid synchro problems:

Except then the whole loading of the tileset is synchronized, so any one tileset getting loaded would block all threads requesting a tileset, right?

[sergsavchuk]

Yes, you are right, the function itself should be synchronized to avoid synchro problems:

Except then the whole loading of the tileset is synchronized, so any one tileset getting loaded would block all threads requesting a tileset, right?

Yes, this can be solved as in the previous version of the cache:

public class FutureTilesetCache implements TilesetCache
{
    private final Logger log = LoggerFactory.getLogger(FutureTilesetCache.class);

    private final Map<String, CompletableFuture<TileSet>> cachedTilesets = new ConcurrentHashMap<>();

    public synchronized boolean needToLoadTileset(String tilesetName)
    {
        if (!cachedTilesets.containsKey(tilesetName))
        {
            log.info("Need to load " + tilesetName);
            cachedTilesets.put(tilesetName, new CompletableFuture<>());
            return true;
        }
  
        return false;
    }

    @Override
    public TileSet getTileset(String tilesetName, TilesetLoader tilesetLoader)
    {
        try
        {
            if (tilesetLoader != null && needToLoadTileset(tilesetName))
            {
                cachedTilesets.get(tilesetName).complete(tilesetLoader.load());
            }

            return cachedTilesets.get(tilesetName).get();
        }
        catch (Exception e)
        {
            throw new RuntimeException("Something went wrong", e);
        }
    }
}

[bjorn]

Yes, this can be solved as in the previous version of the cache:

Alright, then the interface is fine as it is! Thanks a lot for making this nice improvement! :-)