Skip to content

Commit

Permalink
More efficient log rotation checking
Browse files Browse the repository at this point in the history
Merely checking need for log rotation shows up on the radar when profiling,
especially if there are many concurrent committers. When appending there's
a natural selection of one out of all that gets to force the log.
This commit piggy-backs on that decision and lets that thread, and only
that one out of all concurrent committers, to check log rotation.

Also introduced double checked locking for checking need for log rotation
due to a lot of unnecessary hogging of that monitor in most calls,
since log rotation isn't needed very often

These two minor changes improves transaction commit concurrency
  • Loading branch information
tinwelint committed Aug 4, 2017
1 parent e94eddd commit d19bcd8
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 13 deletions.
Expand Up @@ -93,11 +93,6 @@ public void start() throws Throwable
@Override
public long append( TransactionToApply batch, LogAppendEvent logAppendEvent ) throws IOException
{
// We put log rotation check outside the private append method since it must happen before
// we generate the next transaction id
boolean logRotated = logRotation.rotateLogIfNeeded( logAppendEvent );
logAppendEvent.setLogRotated( logRotated );

// Assigned base tx id just to make compiler happy
long lastTransactionId = TransactionIdStore.BASE_TX_ID;
// Synchronized with logFile to get absolute control over concurrent rotations happening
Expand Down Expand Up @@ -132,7 +127,13 @@ public long append( TransactionToApply batch, LogAppendEvent logAppendEvent ) th
// as committed since they haven't been forced to disk yet. So here we force, or potentially
// piggy-back on another force, but anyway after this call below we can be sure that all our transactions
// in this batch exist durably on disk.
forceAfterAppend( logAppendEvent );
if ( forceAfterAppend( logAppendEvent ) )
{
// We got lucky and were the one forcing the log. It's enough if ones of all doing concurrent committerss
// checks the need for log rotation.
boolean logRotated = logRotation.rotateLogIfNeeded( logAppendEvent );
logAppendEvent.setLogRotated( logRotated );
}

// Mark all transactions as committed
publishAsCommitted( batch );
Expand Down Expand Up @@ -236,8 +237,9 @@ private TransactionCommitment appendToLog( TransactionRepresentation transaction
* Called by the appender that just appended a transaction to the log.
*
* @param logForceEvents A trace event for the given log append operation.
* @return {@code true} if we got lucky and were the ones forcing the log.
*/
protected void forceAfterAppend( LogForceEvents logForceEvents ) throws IOException
protected boolean forceAfterAppend( LogForceEvents logForceEvents ) throws IOException
{
// There's a benign race here, where we add our link before we update our next pointer.
// This is okay, however, because unparkAll() spins when it sees a null next pointer.
Expand Down Expand Up @@ -284,6 +286,7 @@ protected void forceAfterAppend( LogForceEvents logForceEvents ) throws IOExcept
databaseHealth.assertHealthy( IOException.class );
}
}
return attemptedForce;
}

private void forceLog( LogForceEvents logForceEvents ) throws IOException
Expand Down
Expand Up @@ -49,18 +49,21 @@ public boolean rotateLogIfNeeded( LogAppendEvent logAppendEvent ) throws IOExcep
* doing force (think batching of writes), such that it can't see a bad state of the writer
* even when rotating underlying channels.
*/
synchronized ( logFile )
if ( logFile.rotationNeeded() )
{
if ( logFile.rotationNeeded() )
synchronized ( logFile )
{
try ( LogRotateEvent rotateEvent = logAppendEvent.beginLogRotate() )
if ( logFile.rotationNeeded() )
{
doRotate();
try ( LogRotateEvent rotateEvent = logAppendEvent.beginLogRotate() )
{
doRotate();
}
return true;
}
return true;
}
return false;
}
return false;
}

/**
Expand Down

0 comments on commit d19bcd8

Please sign in to comment.