ICU Bounded Cache, r2

ICU Bounded Cache, r2

[andy.heninger, 2015-07-29 21:41:26]

https://codereview.appspot.com/253570043/diff/1/source/common/sharedobject.cpp
File source/common/sharedobject.cpp (right):

https://codereview.appspot.com/253570043/diff/1/source/common/sharedobject.cp...
source/common/sharedobject.cpp:52: umtx_atomic_inc(&totalRefCount);
Related to the comment in the .h file, this is the kind of update that makes me
nervous with threads racing through, getting one upddate done but not the other.

https://codereview.appspot.com/253570043/diff/1/source/common/sharedobject.h
File source/common/sharedobject.h (right):

https://codereview.appspot.com/253570043/diff/1/source/common/sharedobject.h#...
source/common/sharedobject.h:213: mutable u_atomic_int32_t hardRefCount;
I'm really wary of these three reference counts. There is no synchronization
between them, their values can be inconsistent when there are races through the
updating functions, and I can't see clearly whether that's a problem or not.

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.cpp
File source/common/unifiedcache.cpp (right):

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.cp...
source/common/unifiedcache.cpp:124: int32_t count, int32_t
percentageOfInUseItems) {
Should check that values are sane - not negative, not so large that calculations
using them might overflow. Which means the function needs an error parameter.

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.cp...
source/common/unifiedcache.cpp:211: // Returns the next element in the cache
round robin style.
Since this is relying on currently undocumented behavior of uhash, we should add
something there (to uhash) documenting what happens if an iteration is continued
after the hash table has been modified. (The iteration is guaranteed to
terminate reasonably; no guarantees that every element will be returned, or that
some won't be returned more than once.)

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.cp...
source/common/unifiedcache.cpp:456: UHashElement *ptr = const_cast<UHashElement
*>(element);
Should the parameter have been non-const, avoiding the cast?
It's being modified, so const seems questionable.

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.h
File source/common/unifiedcache.h (right):

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.h#...
source/common/unifiedcache.h:309: * settings.
Or, alternatively, we could have this function immediately run evictions until
the new constraints are met. Might be useful for testing, but probably doesn't
otherwise really matter, since we don't anticipate changing the settings on the
fly. I guess I'm not suggesting changing it.

https://codereview.appspot.com/253570043/diff/1/source/test/intltest/unifiedc...
File source/test/intltest/unifiedcachetest.cpp (right):

https://codereview.appspot.com/253570043/diff/1/source/test/intltest/unifiedc...
source/test/intltest/unifiedcachetest.cpp:154: // cache API incorrectly by
creating their own cache instances.
This should probably be mentioned on the constructor declaration itself.

https://codereview.appspot.com/253570043/diff/1/source/test/intltest/unifiedc...
source/test/intltest/unifiedcachetest.cpp:267: }
Since setEvictionPolicy() is advertised to work at any time, maybe change the
settings of an already populated test cache, to verify that nothing bad happens.

[tkeep, 2015-07-31 17:41:17]

Andy, I have responded to your 2nd round of comments.

https://codereview.appspot.com/253570043/diff/1/source/common/sharedobject.cpp
File source/common/sharedobject.cpp (right):

https://codereview.appspot.com/253570043/diff/1/source/common/sharedobject.cp...
source/common/sharedobject.cpp:52: umtx_atomic_inc(&totalRefCount);
On 2015/07/29 21:41:26, andy.heninger wrote:
> Related to the comment in the .h file, this is the kind of update that makes
me
> nervous with threads racing through, getting one upddate done but not the
other.

Done.

https://codereview.appspot.com/253570043/diff/1/source/common/sharedobject.h
File source/common/sharedobject.h (right):

https://codereview.appspot.com/253570043/diff/1/source/common/sharedobject.h#...
source/common/sharedobject.h:213: mutable u_atomic_int32_t hardRefCount;
On 2015/07/29 21:41:26, andy.heninger wrote:
> I'm really wary of these three reference counts. There is no synchronization
> between them, their values can be inconsistent when there are races through
the
> updating functions, and I can't see clearly whether that's a problem or not.

Done.

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.cpp
File source/common/unifiedcache.cpp (right):

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.cp...
source/common/unifiedcache.cpp:124: int32_t count, int32_t
percentageOfInUseItems) {
On 2015/07/29 21:41:26, andy.heninger wrote:
> Should check that values are sane - not negative, not so large that
calculations
> using them might overflow. Which means the function needs an error parameter.

Added check to ensure parameters are non-negative. Guaranteeing that overflow's
won't happen is a harder problem. Would it make sense to cast the in-use size
and percentage to int64s before multiplying? Then I could detect overflow if the
product > 2^32 - 1.

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.cp...
source/common/unifiedcache.cpp:211: // Returns the next element in the cache
round robin style.
On 2015/07/29 21:41:26, andy.heninger wrote:
> Since this is relying on currently undocumented behavior of uhash, we should
add
> something there (to uhash) documenting what happens if an iteration is
continued
> after the hash table has been modified. (The iteration is guaranteed to
> terminate reasonably; no guarantees that every element will be returned, or
that
> some won't be returned more than once.)

Done.

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.cp...
source/common/unifiedcache.cpp:456: UHashElement *ptr = const_cast<UHashElement
*>(element);
On 2015/07/29 21:41:26, andy.heninger wrote:
> Should the parameter have been non-const, avoiding the cast?
> It's being modified, so const seems questionable.

UHashElements are always const. In fact the documentation in uhash states that
under no circumstances should a caller modify a returned
UHashElement. 

Nevertheless, we have been modifying UHashElements in this code since
UnifiedCache was released. Maybe we should consider revisiting the uhash
documentation to state that only the key of a UHashElement should never be
changed?

Because UHashElement is always const, if I were to use UHashElement * as you
suggest in the parameter list, it would only force me to move the const_cast
into the caller.

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.h
File source/common/unifiedcache.h (right):

https://codereview.appspot.com/253570043/diff/1/source/common/unifiedcache.h#...
source/common/unifiedcache.h:309: * settings.
On 2015/07/29 21:41:26, andy.heninger wrote:
> Or, alternatively, we could have this function immediately run evictions until
> the new constraints are met. Might be useful for testing, but probably doesn't
> otherwise really matter, since we don't anticipate changing the settings on
the
> fly. I guess I'm not suggesting changing it.

Acknowledged.

https://codereview.appspot.com/253570043/diff/1/source/test/intltest/unifiedc...
File source/test/intltest/unifiedcachetest.cpp (right):

https://codereview.appspot.com/253570043/diff/1/source/test/intltest/unifiedc...
source/test/intltest/unifiedcachetest.cpp:154: // cache API incorrectly by
creating their own cache instances.
On 2015/07/29 21:41:26, andy.heninger wrote:
> This should probably be mentioned on the constructor declaration itself.

Done.

https://codereview.appspot.com/253570043/diff/1/source/test/intltest/unifiedc...
source/test/intltest/unifiedcachetest.cpp:267: }
On 2015/07/29 21:41:26, andy.heninger wrote:
> Since setEvictionPolicy() is advertised to work at any time, maybe change the
> settings of an already populated test cache, to verify that nothing bad
happens.

Done.