|
[
Permlink
| « Hide
]
Knut Anders Hatlen added a comment - 13/Feb/07 09:50 AM
SinglePool's monitor is only locked when one needs to fetch the LockSpace object associated with a compatibility space from SinglePool's hash table. A hash table is used because the lock manager API says that any object can be a compatibility space, and that two objects, a and b, refer to the same compatibility space if a.equals(b) returns true. In the current Derby code, there are no compatibility spaces that are considered equal by equals() that are not considered equal by the == operator. Therefore, we could make the API have stricter requirements to the compatibility space objects (for instance that they must be == and created by a createCompatibilitySpace method) and remove the hash table. Each compatibility space object would have a direct reference to its locks. Since there would be no hash table to synchronize on, the problem with monitor contention would be solved. Also, there would be less indirection which could also be beneficial in the single-user case.
I think you are proposing that the LockFactory has a createCompatibilitySpace method that returns some interface, say CompatibilitySpace. Then all methods in the lock api that take 'Object compatibilitySpace' would change to 'CompatibilitySpace compatibilitySpace'. Without the specific interface the api could be become subject for bugs if a object not created by createCompatibilitySpace is passed in.
With this approach I don't see any need to specifiy/require anything about equals for CompatibilitySpace, but maybe I'm missing something. I think this is a good approach, the existing code valued footprint over multi-cpu performance (and maybe the existing approach did not save footprint due to the memory used by the hashtable). Seems this approach only needs one new class, the interface CompatibilitySpace, and then the existing LockSpace would implement it. > but maybe I'm missing something.
Nope, you're not missing anything! :) I think your suggestions sound great. LanguageConnectionContext has a method called getLockSpace(int) which creates a compatibility space, but the method is not used. unused.diff removes the method so that it doesn't complicate things when the use of compatibility space objects changes. All the tests passed.
getLockSpace(int) in the previous comment should have been getLockObject(int). Committed unused.diff with revision 507500.
derby-2328.diff removes the Hashtable in SinglePool. Derbyall and the JUnit tests passed on Solaris 10/JDK6.
Description of the changes: - New method in LockFactory: createCompatibilitySpace() - New (almost empty) interface CompatibilitySpace to be returned from createCompatibilitySpace() - LockSpace implements CompatibilitySpace and is returned by SinglePool.createCompatibilitySpace() - SinglePool no longer extends Hashtable. Now it casts the supplied compatibility space object to LockSpace and uses it directly. - Modified signatures of all methods/variables that used compatibility spaces (Object -> CompatibilitySpace) - Modified code that assumed compatibility space objects were transaction objects (virtual lock table and code that locked an object with the transaction as lock group) - Code that tested space1.equals(space2) was changed to (space1 == space2) Because of the signature changes the patch grew quite big. Reviews would be greatly appreciated. Thanks. Adding comments sent earlier today to derby-dev when JIRA was unavailable:
===================================================== I have downloaded and reviewed the patch. I like the introduction of the CompatibilitySpace interface and having to explicitly to create compatibility spaces. It mades the code more readable. Also removing the hash table from SinglePool is good both for simplifying the code, readability and performance (CPU and monitor contention). The patch looks very good. I have only some very minor nits: * java/engine/org/apache/derby/impl/services/locks/LockSpace.java: -since you have changed the signature of the LockSpace constructor you should consider adding JavaDoc describing the new parameter (Object owner). -it might also be an idea to explain the "owner" concept in the JavaDoc for the class since this is a new concept you are introducing? (and if you change the JavaDoc for the class, you can probably also change the sentence refeering the "a hashtable keyed by..." to a "hash map keyed by....") I have run the derbyall and Junit test suites with the patch. Only the two tests that currently fail in the thinderbox test failed. +1 to commit this patch. Thanks for the work on this, Knut Anders! Regards, Olav Thanks for the review, Olav! I made some changes to the javadoc as you suggested and committed revision 513222.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Sub-task
Closed
Minor
