LocalStore uses a SparseArray to track target ids, not a set #65
Conversation
|
Eh, hang on. I apparently only ran a subset of tests locally :( |
|
Ok, fixed it. It was android stuff. |
There was a problem hiding this comment.
We have customers doing thousands of listens (essentially all document lookups) and the SparseArray docs indicate performance is only 50% worse with "hundreds" of entries and that it shouldn't be used for large collections.
I'm not morally opposed to this, but how much worse is it for these worse-case customers?
|
I'm not adding the |
|
/test connected-check |
|
/test smoke-tests-release |
|
/test connected-check |
| @@ -115,13 +115,13 @@ public void removeQueryData(QueryData queryData) { | |||
| * | |||
| * @return the number of targets removed | |||
| */ | |||
| int removeQueries(long upperBound, Set<Integer> activeTargetIds) { | |||
| int removeQueries(long upperBound, SparseArray<?> activeTargetIds) { | |||
There was a problem hiding this comment.
The code in this function indicates to me that we always need to accept a SparseArray. For any other sparse array, activeTargetIds.get(targetId) would return null. Can you explain to me why you chose the wildcard in this PR?
There was a problem hiding this comment.
Er, why would any other SparseArray return null? The target ids are the keys, and SparseArray is required to use integer keys. It doesn't matter what the value is, we are only ever using the keys. Because we don't care about the value, the ? is appropriate. It indicates we will not make use of the value, which we don't.
There was a problem hiding this comment.
As per our offline discussion, this is ok. It's just not the most obvious use of an array-like data structure to me. I wish it was possible to make the key-only nature more obvious (such as with a Map's keySet), but since that's not possible without a copy, this LGTM
No functional changes, just switches
Set->SparseArrayfor LRU stuff sinceLocalStorealready uses aSparseArrayto track active targets.