Ready for Review: Weigher Impl #156

digitalbuddha · 2020-04-19T23:45:33Z

Resolve #153

I was able to implement as extension function. Doing as a subclass of Builder did not work out due to build function taking a parameterized type. I could not figure out how to make that one the same as the class parameters on WeighingCacheBuilder

I can't get tests to run locally, will run and add new ones tomorrow.

My biggest question is whether expireEntries needs to reduce the total weight. Initially I had it reducing total weight but previous impl (copied from guava) did not do it.

https://github.com/nytimes/Store/blob/e4c21b23c80ba1c6a2f5c451066ed7adff3592f0/cache/src/main/java/com/nytimes/android/external/cache3/LocalCache.java#L2657

Bug or feature?

TODO Tests tomorrow

digitalbuddha · 2020-04-19T23:46:24Z

cache/src/main/kotlin/com/dropbox/android/external/cache4/Cache.kt

@@ -54,6 +54,14 @@ interface Cache<in Key : Any, Value : Any> {
     */
    interface Builder {

+        @ExperimentalTime
+        var expireAfterWriteDuration: Duration


had to pull these into interface for extension build function. My question about these (for follow up) - do we need the setter functions or can we have a builder with only vars and set()

Setter functions return the instance whereas simple vars do not. So that allows chain of the method calls.

ychescale9 · 2020-04-20T11:19:26Z

cache/src/main/kotlin/com/dropbox/android/external/cache4/RealCache.kt

+    val weigher: Weigher<Key, Value> = OneWeigher(),
+    val maxWeight: Long = UNSET_LONG


Do we need the default parameters? this class is internal anyway so we can just choose the defaults in builder?

Since the default builder is weigher agnostic it felt cleaner here.

ychescale9 · 2020-04-20T11:22:05Z

cache/src/main/kotlin/com/dropbox/android/external/cache4/Weigher.kt

+    fun weigh(key: K, value: V): Int
+}
+
+class OneWeigher<K : Any, V : Any> : Weigher<K, V> {


internal?

cache/src/main/kotlin/com/dropbox/android/external/cache4/RealCache.kt

tasomaniac

Looks good to me. If Store3 had tests around this, it would be good to bring those.

digitalbuddha · 2020-04-26T17:41:05Z

Great let me finish up tonight/tomorrow. We did not have tests since it was a straight fork of Guava. Let me add tests

…

On Sun, Apr 26, 2020, 1:38 PM Said Tahsin Dane ***@***.***> wrote: ***@***.**** approved this pull request. Looks good to me. If Store3 had tests around this, it would be good to bring those. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#156 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCRPXH3RTNS4GEKJVVWUOLRORWSPANCNFSM4MMAC7UA> .

tasomaniac · 2020-04-26T18:06:00Z

We have some tests in Apollo testing our weigher implementation and it's Integration with the cache. Does Store have snapshots? I can quickly check and run those.

tasomaniac · 2020-07-06T09:36:55Z

Any plans to finalize this? We need this in order to migrate from old version. Thanks.

digitalbuddha · 2020-07-08T12:03:26Z

Yes this week. Sorry got pulled off and time flies

tasomaniac · 2020-07-08T14:03:25Z

Thanks ❤️

digitalbuddha · 2020-07-14T13:05:37Z

Ran into a race condition while trying to land. Will continue tonight (almost there)

…Cache.kt Co-Authored-By: Yang <ychescale9@gmail.com>

…g tests)

digitalbuddha · 2020-07-18T15:41:35Z

@tasomaniac should be good to go, mind reviewing?

codecov · 2020-07-18T16:15:13Z

Codecov Report

Merging #156 into master will decrease coverage by 0.38%.
The diff coverage is 94.87%.

@@             Coverage Diff              @@
##             master     #156      +/-   ##
============================================
- Coverage     86.01%   85.62%   -0.39%     
+ Complexity      230      229       -1     
============================================
  Files            54       56       +2     
  Lines           951      981      +30     
  Branches        149      156       +7     
============================================
+ Hits            818      840      +22     
- Misses           76       77       +1     
- Partials         57       64       +7

Impacted Files	Coverage Δ	Complexity Δ
...dropbox/android/external/cache4/WeighingBuilder.kt	`90.00% <90.00%> (ø)`	`0.00 <0.00> (?)`
...n/com/dropbox/android/external/cache4/RealCache.kt	`82.25% <95.45%> (-3.73%)`	`40.00 <6.00> (-1.00)`
...otlin/com/dropbox/android/external/cache4/Cache.kt	`100.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...lin/com/dropbox/android/external/cache4/Weigher.kt	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a954e4...24debc5. Read the comment docs.

ychescale9 · 2020-07-20T13:02:53Z

cache/src/main/kotlin/com/dropbox/android/external/cache4/RealCache.kt

@@ -78,6 +81,8 @@ internal class RealCache<Key : Any, Value : Any>(
     */
    private val loadersSynchronizer = KeyedSynchronizer<Key>()

+    private var totalWeight = 0L


was this reverted?

Oops good catch. I'll add tests for the new builder as well. BTW your tests were great and caught a logic error of mine

ychescale9

Some tests would be great 😃

digitalbuddha · 2020-08-06T16:31:46Z

@ychescale9 ready to review, I added tests. The most interesting one was test cache and evict nothing when caching something heavier than max weight which lead to a behavior change in our cache code. Guava cache's will not cache entries that are larger than max weight. Made a minor adjustment to short circuit the put logic. Could you kindly double check that this looks reasonable?

eyalgu · 2020-08-07T17:16:56Z

cache/src/main/kotlin/com/dropbox/android/external/cache4/RealCache.kt

@@ -139,12 +148,22 @@ internal class RealCache<Key : Any, Value : Any>(
        val existingEntry = cacheEntries[key]
        if (existingEntry != null) {
            // cache entry found
-            recordWrite(existingEntry, nowNanos)
+            // remove the weight of the current entry
+            totalWeight -= existingEntry.weight


The cache is multi-threaded and this is not thread safe.

This is actually not easy to fix without adding substantial synchronization to the cache (or adding other optimizations beyond the scope of this cache). The reason is that you need to make sure the state of cachedEntries and totalWeight is updated atomically.

eyalgu · 2020-08-07T17:21:21Z

cache/src/main/kotlin/com/dropbox/android/external/cache4/RealCache.kt

            existingEntry.value = value
+            existingEntry.weight = weight


value and weight need to be updated atomically (e.g put them in pair or data class)

digitalbuddha · 2020-08-09T21:38:33Z

#200 is the fix for this

digitalbuddha commented Apr 19, 2020

View reviewed changes

ychescale9 reviewed Apr 20, 2020

View reviewed changes

tasomaniac previously approved these changes Apr 26, 2020

View reviewed changes

ychescale9 mentioned this pull request Jul 6, 2020

MegaIssue: Cache improvements apollographql/apollo-kotlin#2331

Open

40 tasks

digitalbuddha dismissed tasomaniac’s stale review via 492cce7 July 18, 2020 15:23

miken and others added 5 commits July 18, 2020 11:40

weigher impl

e328bf2

close class

173fae0

Update cache/src/main/kotlin/com/dropbox/android/external/cache4/Real…

f46dc42

…Cache.kt Co-Authored-By: Yang <ychescale9@gmail.com>

fixer weigher impl. Still need to fix lru ordering in cache (2 failin…

864dec6

…g tests)

passing tests after adding weigher

46e583f

digitalbuddha force-pushed the miken/weigher branch from 7e65133 to 46e583f Compare July 18, 2020 15:41

miken added 2 commits July 18, 2020 11:54

lint

4d4ac91

ktlint

33bbf66

ychescale9 reviewed Jul 20, 2020

View reviewed changes

add tests for weigher

5e402ad

digitalbuddha changed the title ~~WIP Weigher Impl~~ Ready for Review: Weigher Impl Aug 6, 2020

ktlint

24debc5

digitalbuddha requested review from eyalgu and chris-mitchell August 6, 2020 16:45

eyalgu suggested changes Aug 7, 2020

View reviewed changes

digitalbuddha closed this Aug 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ready for Review: Weigher Impl #156

Ready for Review: Weigher Impl #156

digitalbuddha commented Apr 19, 2020

digitalbuddha Apr 19, 2020

tasomaniac Apr 20, 2020

ychescale9 Apr 20, 2020

digitalbuddha May 14, 2020

ychescale9 Apr 20, 2020

tasomaniac left a comment

digitalbuddha commented Apr 26, 2020 via email

tasomaniac commented Apr 26, 2020

tasomaniac commented Jul 6, 2020

digitalbuddha commented Jul 8, 2020

tasomaniac commented Jul 8, 2020

digitalbuddha commented Jul 14, 2020

digitalbuddha commented Jul 18, 2020

codecov bot commented Jul 18, 2020 •

edited

Loading

ychescale9 Jul 20, 2020

digitalbuddha Jul 20, 2020

ychescale9 left a comment

digitalbuddha commented Aug 6, 2020

eyalgu Aug 7, 2020

eyalgu Aug 7, 2020

digitalbuddha commented Aug 9, 2020

		val weigher: Weigher<Key, Value> = OneWeigher(),
		val maxWeight: Long = UNSET_LONG

Ready for Review: Weigher Impl #156

Ready for Review: Weigher Impl #156

Conversation

digitalbuddha commented Apr 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tasomaniac left a comment

Choose a reason for hiding this comment

digitalbuddha commented Apr 26, 2020 via email

tasomaniac commented Apr 26, 2020

tasomaniac commented Jul 6, 2020

digitalbuddha commented Jul 8, 2020

tasomaniac commented Jul 8, 2020

digitalbuddha commented Jul 14, 2020

digitalbuddha commented Jul 18, 2020

codecov bot commented Jul 18, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ychescale9 left a comment

Choose a reason for hiding this comment

digitalbuddha commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalbuddha commented Aug 9, 2020

codecov bot commented Jul 18, 2020 •

edited

Loading