Optimized the locking on the database #300
Conversation
AlexandraRoatis
commented
Mar 28, 2018
•
AlexandraRoatis
commented
- removed locks from the database implementations
- added a new database that can add read-write locks on top of any IByteArrayKeyValueDatabase
- added locks and cleaned up synchronization inside the:
- TransactionStore
- AionBlockStore
- AionRepositoryImpl
- improved the functionality in the DatabaseFactory
- removed synchronization from ObjectDataSource
- removed the inheritance between TransactionStore and ObjectDataSource
- minor code and documentation cleanup
… removing unnecessary synchronization
…using unlocked databases
…e exceptions stated by the interface
…inter exceptions when the properties are not specified
| @@ -84,14 +88,17 @@ public AionBlock deserialize(byte[] bytes) { | |||
| } | |||
|
|
|||
| public AionBlock getBestBlock() { | |||
| lock.readLock().lock(); | |||
There was a problem hiding this comment.
I suggest a:
lock()
try {
} finally {
unlock()
}Pattern here, just in case any exceptions occur
There was a problem hiding this comment.
Going to refer to this as the deferred unlock pattern, from go:
c.mutex.Lock()
defer c.mutex.Unlock()| return bestBlock; | ||
| } | ||
|
|
||
| public byte[] getBlockHashByNumber(long blockNumber) { | ||
| lock.readLock().lock(); |
There was a problem hiding this comment.
Suggest a deferred unlock pattern here
| addInternalBlock(block, cummDifficulty, mainChain); | ||
| } finally { | ||
| lock.writeLock().unlock(); | ||
| } | ||
| } | ||
|
|
||
| private void addInternalBlock(AionBlock block, BigInteger cummDifficulty, boolean mainChain) { |
There was a problem hiding this comment.
Document that this method is guarded by lock
| @@ -162,10 +189,12 @@ private void addInternalBlock(AionBlock block, BigInteger cummDifficulty, boolea | |||
| } | |||
|
|
|||
| public List<Map.Entry<AionBlock, Map.Entry<BigInteger, Boolean>>> getBlocksByNumber(long number) { | |||
| lock.readLock().lock(); | |||
There was a problem hiding this comment.
Suggest a deferred unlock pattern here
| return result; | ||
| } | ||
|
|
||
| @Override | ||
| public AionBlock getChainBlockByNumber(long number) { | ||
| lock.readLock().lock(); |
There was a problem hiding this comment.
Suggest a deferred unlock pattern here, would also simplify the code!
| @@ -428,19 +419,28 @@ public synchronized void loadAccountState(Address address, Map<Address, AccountS | |||
| } | |||
|
|
|||
| @Override | |||
| public synchronized byte[] getRoot() { | |||
| return worldState.getRootHash(); | |||
| public byte[] getRoot() { | |||
There was a problem hiding this comment.
Suggest deferred unlock pattern
| @Override | ||
| public boolean isCreatedOnDisk() { | ||
| // acquire read lock | ||
| lock.readLock().lock(); |
There was a problem hiding this comment.
Can be simplified to:
lock();
try {
return database.isCreatedOnDisk();
} finally {
unlock();
}| } | ||
| } | ||
|
|
||
| @Override | ||
| public void compact() { | ||
| // acquire read lock | ||
| lock.writeLock().lock(); | ||
|
|
||
| LOG.info("Compacting " + this.toString() + "."); |
There was a problem hiding this comment.
Minor point, not sure if this messages needs to be logged to info, could become noisy
|
|
||
| public boolean put(INFO tx) { | ||
| lock.writeLock().lock(); |
There was a problem hiding this comment.
Suggest deferred unlock pattern here
| return true; | ||
| } | ||
|
|
||
| public INFO get(byte[] txHash, byte[] blockHash) { | ||
| List<INFO> existingInfos = get(txHash); | ||
| lock.readLock().lock(); |
There was a problem hiding this comment.
Suggest deferred unlock pattern here
