bailuk / AAT

Another Activity Tracker for Android
https://bailu.ch/aat
GNU General Public License v3.0
150 stars 41 forks source link

Redundant code #135

Closed rparkins999 closed 1 year ago

rparkins999 commented 1 year ago

Class TileProvider has

    public synchronized void preload(List<TilePosition> tilePositions) {
        cache.setCapacity(tilePositions.size());

        for (TilePosition p : tilePositions) {
            cache.get(p.tile);
        }

        for (TilePosition p : tilePositions) {
            getHandle(p.tile);
        }
    }

What is the purpose of the first for loop? In the second loop, getHandle calls getTileHandle which calls cache.get(), so the first set of calls to cache.get() don't do anything useful.

bailuk commented 1 year ago

The first loop is to update the access time inside the 1. Level cache. (if the tile is already loaded). The second loop is to load missing tiles.

I've added some comments to clarify: https://github.com/bailuk/AAT/commit/222a07aac1b68df42e31f546a2e7126acf28b6b4

rparkins999 commented 1 year ago

As my comment says, getHandle() always calls through to cache.get() anyway, so you just update the access time twice. I couldn't find any path through getHandle() that didn't call cache.get() before doing anything else.

bailuk commented 1 year ago

The first loop only handles cached tiles. It kind of "reserves" them by setting the access time.

Without this loop it might be possible that, in the second loop, a needed tile gets replaced inside the cache before the call to cache.get(). Because the second loop can put (replace) tiles into the cache.

You are right that cache.get() gets called twice, but that is actually intended here.

rparkins999 commented 1 year ago

Without this loop it might be possible that, in the second loop, a needed tile gets replaced inside the cache before the call to cache.get()

I don't see any way in which this is possible. The first thing that is done in the second loop is to call getHandle() on an object of class Tile. getHandle() is private, so it must be the version in TileProvider which is called.

The first thing that getHandle() does is to call getTileHandle() on the same object of class Tile which was passed to it. getTileHandle() is also private, so it must be the version in TileProvider which is called.

The first thing that getTileHandle() does is to call cache.get() on the same object of class Tile which was passed to it. So there is no way that anything else can happen before cache.get() is called.

If you're suggesting that another thread can change tilePositions[i].tile to point to a different object of class Tile, in that case there was no point in calling cache.get() in the first loop to set the access time, because you aren't going to access the object of class Tile that you called it on.