Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Cache.getAllPresent() #609

Merged

Conversation

BoD
Copy link
Contributor

@BoD BoD commented Feb 28, 2024

Description

Adds getAllPresent() with no argument to Cache, in addition to existing getAllPresent(keys: List<*>).
In the original Guava library there was an asMap() method which could be used to get the contents of the cache.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Test Plan

Removed @Ignore from the getAllPresent() test.

Checklist:

Before submitting your PR, please review and check all of the following:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (N/A)
  • My changes generate no new warnings
  • I have added tests that prove my change is effective
  • New and existing unit tests pass locally with my changes

Additional Notes:

Used clear() as an inspiration for the implementation.

@BoD BoD force-pushed the add-get-all-present-to-cache branch from eea5f7a to c2f5141 Compare February 28, 2024 15:44
/**
* @return Map of the [Value] associated with each [Key] in the cache.
*/
fun getAllPresent(): Map<Key, Value> = throw NotImplementedError()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this for source compatibility of potential implementers of Cache. Let me know if you think this is unnecessary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is necessary, though appreciate you thinking about this. @digitalbuddha @yigit - thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't hurt

@@ -21,6 +21,11 @@ interface Cache<Key : Any, Value : Any> {
*/
fun getAllPresent(keys: List<*>): Map<Key, Value>

/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is reasonable. Can you please include an explanation:

/**
 * The default implementation provided here throws a [NotImplementedError] to maintain backward compatibility for existing implementations.
 * /

Copy link
Collaborator

@matt-ramotar matt-ramotar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document the default method, otherwise LGTM and I will be around to stamp. I see the other tickets - when do you need a release?

@@ -1379,6 +1379,29 @@ internal class LocalCache<K : Any, V : Any>(builder: CacheBuilder<K, V>) {
}*/
}

fun activeEntries(): Map<K, V> {
if (count.value != 0) { // read-volatile

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can invert if condition and return eagerly to avoid a nested indentation level 🙏

@BoD
Copy link
Contributor Author

BoD commented Mar 18, 2024

Please document the default method, otherwise LGTM and I will be around to stamp. I see the other tickets - when do you need a release?

🙏 I've addressed the comments 👍

A release is not urgent, for now we've bundled the classes in the project - whenever you make a release, we'll use the dependency instead.

@@ -1379,6 +1379,28 @@ internal class LocalCache<K : Any, V : Any>(builder: CacheBuilder<K, V>) {
}*/
}

fun activeEntries(): Map<K, V> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:
To make the function more efficient, we can consider a few optimizations:

Avoid creating a new map if there are no active entries: Instead of always creating a new map using buildMap, we can check if any active entries were found during the iteration. If no active entries are found, we can return an empty map directly.

Use a mutable map to avoid extra memory allocation:
When you use buildMap, it creates a new read-only map and copies the elements into it. This means that there is an additional step of creating a new map and copying the elements from the builder to the final map. This extra step introduces some overhead in terms of memory allocation and copying.

Instead of using buildMap, which creates a new map, we can use a mutable map like mutableMapOf() and populate it directly. This avoids the extra memory allocation and copying overhead.

fun activeEntries(): Map<K, V> {
    // read-volatile
    if (count.value == 0) return emptyMap()

    reentrantLock.lock()
    return try {
        val activeMap = mutableMapOf<K, V>()
        val table = table.value
        for (i in 0 until table.size) {
            var e = table[i]
            while (e != null) {
                if (e.valueReference?.isActive == true) {
                    activeMap[e.key] = e.valueReference.get()!!
                }
                e = e.next
            }
        }
        activeMap.ifEmpty { emptyMap() }
    } finally {
        reentrantLock.unlock()
    }
}

BoD added 3 commits April 4, 2024 18:37
Signed-off-by: BoD <BoD@JRAF.org>
Signed-off-by: BoD <BoD@JRAF.org>
Signed-off-by: BoD <BoD@JRAF.org>
@BoD BoD force-pushed the add-get-all-present-to-cache branch from de77e99 to 9b6a7b5 Compare April 4, 2024 16:38
@digitalbuddha digitalbuddha merged commit 63928aa into MobileNativeFoundation:main Apr 5, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants