[
https://issues.apache.org/jira/browse/DERBY-2911?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12571358#action_12571358
]
Øystein Grøvlen commented on DERBY-2911:
----------------------------------------
I have looked at the code introduced in this JIRA for a new cache
manager, and think it looks very good. I have only found one small issue
related to correctness (4e), but I have some minor comments/questions on
the organization of the code:
1. ConcurrentCache:
a. What is the significant difference between removeEntry and
evictEntry? At first, I thought it was that evictEntry set the
Cacheable to null, but then I discovered that removeEntry does
the same through its call to CacheEntry#free. So I guess the
only difference is that removeEntry will include a call to
ReplacementPolicy#Callback#free. I am not sure I understand why
this should be avoided for evictEntry. If both methods are
needed, the first sentences of their javadoc should be different
in order to be able to make a distinction when reading the
javadoc summary.
b. To me, the organization of find and create methods for entries
and cacheables are a bit confusing. It seems to me that it would
be easier to understand if it was based on basic methods for
get/find and create, that just did what it said it did, and then
created the more advanced operations like
findAndCreateIfNotFound on top of that. Is this done for
efficiency reasons? Especially, the findOrCreateObject method
is a bit confusing since it uses a parameter to change behavior
from findAndCreateIfNotFound to createAndFlagErrorIfFound.
Would it be possible to create two separate methods here? At
least, I think the javadoc for the create parameter needs to
describe exactly what will occur.
c. findOrCreateObject: The create parameter will determine whether
setIdentify and not createIdentify will be called when an object
is not found in the cache. Is this difference just an issue of
whether createParameter has been provided, or is there something
more that distinguishes these to cases?
d. findCached/release: Comments states that one "don't need to call
getEntry()", but I guess the points is that it would be harmful
to call getEntry?
2. CacheEntry:
a. lockWhenIdentityIsSet: I do not feel the name really describes
what the method is doing. lockAndBlockUntilIdentityIsSet would
be more descriptive. Maybe you can come up with an even better
(and shorter) name.
3. ReplacementPolicy:
a. The Callback returned by insertEntry seems currently not to be
in use. In what situations may it be used?
4. ClockPolicy:
a. grow: Instead of requiring that the caller synchronize, why not
synchronize within the method? (Like is done for several other
methods, eg., moveHand)
b. rotateClock: The concept of a small cache (<20 entries) is that
relevant for any currently used caches in Derby? It seems a bit
strange to check 38 items when there are 19 entries, but only 4
items if there are 20 entries. But maybe it is not that
relevant with respect to the cache sizes used in Derby?
c. rotateClock: I think it would be good if some of the discussion
in the JIRA about why it is not using the entries it has
cleaned, would be reflected in comments.
d. shrinkMe: javadoc for return is incomplete
e. shrinkMe: I think there is an off-by-one error for the call on
clock.get. If pos==size-1 when entering the synchronized block,
one will do get(size) which I guess would give
ArrayIndexOutOfBoundsException. Also, the first element of clock
will never be inspected since index will never be 0.
f. shrinkMe: The code to determine whether an item could be evicted
is the same as for rotateClock. Could this code be refactored
into a common method? (isEvictable() or something like that)
g. trimMe: This method contains a lot of heuristics that I guess
you have inherited from the old clock implementation. If you
have got any insight into why the particular values are chosen,
if would be good if you could comment on that. (I notice that
the criteria for being a small cache is not the same here as in
rotateClock.)
h. Maybe some of the numeric constants used for heuristics in this
class should be defined as named constants?
5. BackgroundCleaner:
a. I am not sure the statement "used by ConcurrentCache so that it
doesn't have to wait for clean operations to finish" covers the
purpose. Clean operations on ConcurrentCache does not use the
background cleaner. It is find operations on ConcurrentCache
that will invoke the cleaner. In addition, the background
cleaner does not help a given find operation, it just makes
future operation more likely to find an item that can be reused.
b. serviceImmediately: As far as I can tell this method is not
relevant since it is not used by BasicDaemon. Maybe that should
be indicated?
> Implement a buffer manager using java.util.concurrent classes
> -------------------------------------------------------------
>
> Key: DERBY-2911
> URL: https://issues.apache.org/jira/browse/DERBY-2911
> Project: Derby
> Issue Type: Improvement
> Components: Performance, Services
> Affects Versions: 10.4.0.0
> Reporter: Knut Anders Hatlen
> Assignee: Knut Anders Hatlen
> Priority: Minor
> Attachments: cleaner.diff, cleaner.tar, d2911-1.diff, d2911-1.stat,
> d2911-10.diff, d2911-10.stat, d2911-11.diff, d2911-2.diff, d2911-3.diff,
> d2911-4.diff, d2911-5.diff, d2911-6.diff, d2911-6.stat, d2911-7.diff,
> d2911-7a.diff, d2911-9.diff, d2911-9.stat, d2911-entry-javadoc.diff,
> d2911-unused.diff, d2911-unused.stat, d2911perf.java, derby-2911-8.diff,
> derby-2911-8.stat, perftest.diff, perftest.pdf, perftest.stat,
> perftest2.diff, perftest6.pdf, poisson_patch8.tar
>
>
> There are indications that the buffer manager is a bottleneck for some types
> of multi-user load. For instance, Anders Morken wrote this in a comment on
> DERBY-1704: "With a separate table and index for each thread (to remove latch
> contention and lock waits from the equation) we (...) found that
> org.apache.derby.impl.services.cache.Clock.find()/release() caused about 5
> times more contention than the synchronization in LockSet.lockObject() and
> LockSet.unlock(). That might be an indicator of where to apply the next push".
> It would be interesting to see the scalability and performance of a buffer
> manager which exploits the concurrency utilities added in Java SE 5.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
