I've gone through the comments and questions. Please find my
responses below. Regarding the correctness error (4e), I think the
code is actually correct, but it could have been clearer.
The new buffer manager isn't enabled in the build yet. Should the
issues mentioned by Øystein be resolved first, or would it be OK to
enable it first and do the reorganization and improvements later?
Responses to Øystein's questions/comments:
a) Difference between evictEntry() and removeEntry().
I think you are right that the only significant difference is that
removeEntry() calls free() whereas evictEntry() doesn't. Calling
free() from evictEntry() wouldn't be correct in the current code,
though. free() is called to notify the replacement policy that the
Cacheable contained in the entry can be reused. When evictEntry() is
called, we're either in the process of reusing the Cacheable (in which
case we don't want to first mark it as reusable so that others can
grab it, and then try to reobtain it and possibly reuse it), or we're
shrinking the cache (in which case making it reusable would mean that
the cache doesn't shrink).
I'll see if I can find a clever way to merge the two methods, or at
least improve the comments and perhaps the names of the methods.
b) Organization of find and create methods.
I guess it is possible to split them into two or three basic methods
and build the public methods on top of them. However, I'm not sure
it's a good idea to remove the create flag. findOrCreateObject() is a
fairly large method, and the check of the create flag is a very small
part of it. If the create flag were removed, it would mean that we
have to duplicate a most of the logic in that method. But if we split
the logic into more basic methods, it will hopefully be easier to
understand how the create flag affects each of the basic methods.
c) findOrCreateObject(): create vs createParameter
create can be true even if createParameter is null, so I don't think
create can be skipped. The distinction between setIdentity() and
createIdentity() comes from the Cacheable interface and can't be
changed without modifying the existing buffer manager and the store as
d) findCached()/release(): get() vs getEntry()
You're right, it is not harmful to call getEntry() in terms of
correctness, but it's slightly more expensive. I will update the
comments to make this clearer.
a) Misleading name: lockWhenIdentityIsSet()
Your suggestion lockAndBlockUntilIdentityIsSet() does match more
closely the calls in the method (lock() + await()), but since await()
will release the lock until it wakes up, I think the original name
describes better how the locking and blocking is perceived from
outside the method. Perhaps waitUntilIdentityIsSetAndLock() is a
better name? Of course, better comments in the method would also help.
a) Return value from insertEntry() isn't used.
The return value isn't needed since the replacement policy will link
the CacheEntry and the Callback internally by calling
CacheEntry.setCallback(). Will change the method so that it's void.
a) Why not synchronize inside grow()?
One of the callers needs the synchronization for more than just the
call to grow() and therefore needs the synchronized block anyway, so I
felt it was more consistent if all (both) callers used an explicit
synchronization block. (It also avoided double synchronization for
that caller.) The other methods you mention are supposed to be atomic,
and are not intended to be called from within a larger block
synchronized on the same object.
b) Handling of small cache sizes in rotateClock().
I agree that this is a bit strange. The logic is copied from
Clock.rotateClock(), by the way. I believe you are correct that it's
not relevant for the (default) cache sizes currently used (none is
smaller than 32). Perhaps it would be better to simplify it and just
say itemsToCheck = Math.max(20, ...) or something (the number 20 is
c) Add more comments about why cleaned entries are not reused.
Will update the comments with more details from the JIRA discussion.
d) Incomplete @return tag for shrinkMe().
Thanks, fixed in revision 631225.
e) Off-by-one error in shrinkMe().
I don't think there is an off-by-one error, since we use the old value
of pos, not the one that has been increased by one. I guess it's a bit
confusing to use the postfix increment operator and store away the old
value on the same line in the code. Perhaps it would be clearer if the
code were reordered from
index = pos++;
h = clock.get(index);
h = clock.get(pos);
index = pos;
Or perhaps I misunderstood what you meant?
f) Factor out common code for evicting entries in rotateClock() and
Will do that.
g) Comment on the heuristics in trimMe().
As you quite correctly guessed, the heuristics have been taken from
the old clock implementation (Clock.trimToSize()), and I don't know
why they were chosen.
What strikes me with the heuristics, is that they seem to do their
best to prevent this method from ever doing anything (which can also
be seen in the code coverage reports). Also, I don't see very much
value in shrinking a cache that already is smaller than the max
size. If someone needs it, the correct way would be to reduce the max
size of the cache, I think. Perhaps it would be better just to remove
it? Seems like it's much complexity for little added value.
h) Naming constants used in heuristic
Probably a good idea. Will do that.
a) Misleading class javadoc
Will fix that comment.
b) Comment that serviceImmediately() isn't used by BasicDaemon
Could do that. Now, BackgroundCleaner doesn't know that it's being
serviced by a BasicDaemon, so I'm not sure whether or not it's
BackgroundCleaner's concern that the method is not being used by
BasicDaemon. If another implementation of DaemonService is used
(that's not controlled by the cache implementation), the returned
value may become relevant.