Library feedback & future improvements

[dmytrodanylyk]

Hi,

Great library, I had a brief look into the source code, here are some of my thoughts:

Dependencies

    implementation 'com.google.code.gson:gson:2.8.5'
    implementation 'com.squareup.moshi:moshi:1.6.0'
    implementation 'com.fasterxml.jackson.core:jackson-core:2.9.6'
    implementation 'com.fasterxml.jackson.core:jackson-databind:2.9.6'

Not everyone is happy by bringing unnecessary dependencies, better to put them as separate libraries. E.g. com.epam.coroutinecache:coroutines-cache-gson-mapper

API

1. CoroutinesCache has a CoroutineScope parameter. I think cache operations should be scoped, not enire cache. In most cases, cache is a singleton and we create it once per app lifetime since initialization of this object usually takes some time. Also, you default to GlobalScope which is represented by Dispatchers.Default, in your case you should be using Dispatchers.IO
2. Would be nice to be able to save some android data structures like Bitmap.

Performance

1. LRU implementation of memory and disk cache would be great.
2. Currently, every operation lock memory and disk cache. You can boost performance a lot if you perform lock on particular key, not entire store.

E.g.

// independent operations, different keys
store.put("key1")
store.get("key2")

// dependent operations, same keys
store.put("key1")
store.get("key1")

// dependent operations, global
store.put("key1")
store. deleteAll()

3. You expose coroutine API, internally use Mutex as a locking mechanism.
4. Instead of trying to figure out data type during serialization/deserialization I suggest to just ask user this infromation. E.g. why not to do @ProviderKey("TestKey", "Bitmap::class")? With this approach you don't need reflection and can just immediately retrieve correct serializer/deserializer.

Realiability For DiskCache it's better to use AtomicFile to have a backup.

[vladimir-ivanov-epam]

Currently, every operation lock memory and disk cache. You can boost performance a lot if you perform lock on particular key, not entire store.

This is already being addressed in #4 , expect an update in binaries soon!

[dmytrodanylyk]

@vladimir-ivanov-epam I was addressing a different issue. If you can, ping me in a slack channel of kotlinlang.

[dmytrodanylyk]

There is one more thing I wanted to pointed out, I don't see any locking mechanism for entire operations. It seems your operations are not atomic which may result in nasty issues.

suspend fun save(key: String, data: Any?, lifeTimeMillis: Long = 0, isExpirable: Boolean = true) = scope.async {
    val record = Record(data, isExpirable, lifeTimeMillis)

    memory.saveRecord(key, record) // 1

    if (diskCache.storedMB() >= maxMbCacheSize) {
        System.out.println("Record can not be persisted because it would exceed the max limit megabytes settled down")
    } else {
        diskCache.saveRecord(key, record) // 2
    }

    deleteExpirableRecordsAction.deleteExpirableRecords().await()
}

E.g. Consider the following scenario:

async { save("key1", "data1") } // coroutine1
async { save("key2", "data2") } // coroutine2

Because save operation doesn have a lock, following may happen:

• coroutine1 executes memory.saveRecord(key, record)
• coroutine2 executes memory.saveRecord(key, record)
• coroutine2 executes diskCache.saveRecord(key, record)
• coroutine1 executes diskCache.saveRecord(key, record)

As a result memory cache contains value data1 and disk cache contains value data2.