db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Øystein Grøvlen (JIRA) <j...@apache.org>
Subject [jira] Commented: (DERBY-2911) Implement a buffer manager using java.util.concurrent classes
Date Fri, 22 Feb 2008 11:06:19 GMT

    [ 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.


Mime
View raw message