12 Replies Latest reply on Nov 30, 2009 8:32 PM by clebert.suconic

Locking in TimedBuffer

timfox Nov 30, 2009 7:05 AM

I am trying to understand the esoteric locking scheme in the TimedBuffer class.

In particular:

1) LatchTimer.

There is a comment in the code:

// This is used to pause and resume the timer
 // This is a reusable Latch, that uses java.util.concurrent base classes

But I don't see anywhere the timer being paused and/or resumed.

Can you clarify the purpose of this task.

2)

private final Lock lock = new ReentrantReadWriteLock().writeLock();

Firstly, why use a read write lock? Looks like you just need a standard re-entrant mutex:

private final Lock lock = new ReentrantLock(true);

But the important question is, what is the purpose of this lock?

1. Re: Locking in TimedBuffer

timfox Nov 30, 2009 7:23 AM (in response to timfox)

From what I can work out from the code, latchTimer, is used in the following way:

when addBytes is called(), the latch is released with down()

when flush is called the latch is reset, with up()

when the timer runs, in checkTimer() it tries to obtain the latch.

This means that after a flush has occurred, no timer flush can occur until more bytes are added to the buffer.

I don't understand the point of this, if no bytes have been added to the buffer, then flush won't occur anyway since writerIndex will be zero.

(BTW the way, quite apart from not understanding the purpose of this, the way the latch is currently being used, a simple Semaphore with a single token could have been used instead)
Actions
2. Re: Locking in TimedBuffer

clebert.suconic Nov 30, 2009 9:16 AM (in response to timfox)

1) We don't use a real sleep if you remember. We just spin waiting for the next time. The VariableLatch is used here just to stop that spinning. Maybe I could have used a Semaphore, yes. But it would also work with the Latch.

2) The ReadWriteLock is for the method disableAutoFlush and enableAutoFlush as used by the journal. It just prevents a flush after the size of the buffer was checked by the journal.
Actions
3. Re: Locking in TimedBuffer

clebert.suconic Nov 30, 2009 9:19 AM (in response to timfox)

"clebert.suconic@jboss.com" wrote:
1) We don't use a real sleep if you remember. We just spin waiting for the next time. The VariableLatch is used here just to stop that spinning. Maybe I could have used a Semaphore, yes. But it would also work with the Latch.

I did that also, because .. say the server is idle..

If I don't stop spinning the timer, a top command on Linux (or task manager on windows) would show high cpu consumption.

So, this is to avoid users asking "Why the CPU consumption is high when the server is idle?"
Actions
4. Re: Locking in TimedBuffer

clebert.suconic Nov 30, 2009 9:24 AM (in response to timfox)

2) hmmm... in a second thought.. I don't think I need to avoid a flush... the addBytes is alrady synchronized. the worse that could happen is the journal getting a new buffer. if It fitted the old buffer it will certainly fit a new one.. so I can remove that.
Actions
5. Re: Locking in TimedBuffer

timfox Nov 30, 2009 9:29 AM (in response to timfox)

"clebert.suconic@jboss.com" wrote:

I did that also, because .. say the server is idle..

If I don't stop spinning the timer, a top command on Linux (or task manager on windows) would show high cpu consumption.

So, this is to avoid users asking "Why the CPU consumption is high when the server is idle?"

Ok fair enough.

I will replace it with a Semaphore though and add some comments so it's clearer what it's purpose is.
Actions
6. Re: Locking in TimedBuffer

timfox Nov 30, 2009 9:30 AM (in response to timfox)

"clebert.suconic@jboss.com" wrote:
2) hmmm... in a second thought.. I don't think I need to avoid a flush... the addBytes is alrady synchronized. the worse that could happen is the journal getting a new buffer. if It fitted the old buffer it will certainly fit a new one.. so I can remove that.

I have already removed it locally, since I couldn't see the need for it.

I just wanted to check with you, in case you knew I reason I had missed.
Actions
7. Re: Locking in TimedBuffer

clebert.suconic Nov 30, 2009 4:56 PM (in response to timfox)

I just remembered why I needed to avoid a flush after calling fits.

A Flush could come after the fits:

Because of alignments on AIO, the new buffer could be actually smaller. The previous one flushing could have an empty space to fill up the 512 bytes required by AIO. The new one could be actually smaller and we would end up with more bytes written on the journal files.

So, because of that.. I was making sure the flush wouldn't happen while the journal was verifying the size of the buffer and setting the currentFile on the journal.
Actions
8. Re: Locking in TimedBuffer

timfox Nov 30, 2009 6:12 PM (in response to timfox)

Can you rephrase your last post?

I am having trouble understanding it.
Actions
9. Re: Locking in TimedBuffer

clebert.suconic Nov 30, 2009 6:58 PM (in response to timfox)
Currently, the write happens as follows (in summary):

// Disable auto flush on the timer. The Timer should'nt flush anything currentFile.getFile().disableAutoFlush(); if (!currentFile.getFile().fits(size)) { currentFile.getFile().enableAutoFlush(); moveNextFile(false); currentFile.getFile().disableAutoFlush(); } currentFile.getFile().write(.....) currentFile.getFile().enableAutoFlush();

This was to make sure the data to be written will fit on the buffer (or at the current file).

We could either get a new buffer inside the fits on TimedBuffer, or the Journal could move to a new file if it didn't fit.

If the alignment was 1 that would mean no matter how it flushed I would certainly have space at the time of writing.

However, if flush happened after the call to fits and before the write was called, you could have a race where you don't have space on the file to fit the record, as the previous flush may have added blank spaces to complete the 512 alignment.

I couldn't remember about all that earlier before today, and I had my mind set on NIO when I said there was no reason for it.

Also.. This is using a ReadWriteLock().writeLock(); for a mistake. I played with readWrite/ writeLocks while developing this and forgot to just use a regular ReentrantLock.

So, in summary if we remove the disableAutoFlush/enableAutoFlush without a proper way to avoid a flush between the fits call and the write, you may have intermitent issues with AIO.
Actions

10. Re: Locking in TimedBuffer

timfox Nov 30, 2009 7:57 PM (in response to timfox)

If we need to prevent flush being called between checkSize being called and addBytes being called, can't we just set a flag in checkSize:


public synchronized boolean checkSize(final int sizeChecked)
 {
 if (sizeChecked > bufferSize)
 {
 throw new IllegalStateException("Can't write records bigger than the bufferSize(" + bufferSize +
 ") on the journal");
 }

 checkSizeCalled = true;

...

Then test this flag in flush:

public void flush()
 {
 ByteBuffer bufferToFlush = null;

 boolean useSync = false;

 List<IOAsyncTask> callbacksToCall = null;

 synchronized (this)
 {
 if (!checkSizeCalled && buffer.writerIndex() > 0)
 {

And set it to false in addBytes:

public synchronized void addBytes(final EncodingSupport bytes, final boolean sync, final IOAsyncTask callback)
 {
 checkSizeCalled = false;

This would prevent a flush() occurring in the intervening period.

11. Re: Locking in TimedBuffer

clebert.suconic Nov 30, 2009 8:25 PM (in response to timfox)

You would take at least 2X more time to flush... Basically when the time is done, It should flush (with a minimal wait on the lock).

The way it currently works, it would enter the lock and as soon as it is written it would flush the buffer.

Maybe there is a way to avoid the lock and have TimedBuffer controlling move to the next file. (But whatever we do we need to make sure the Bindings Journal will also work, since it doesn't use the TimedBuffer there).

So.. I guess it's a tricky change.
Actions
12. Re: Locking in TimedBuffer

clebert.suconic Nov 30, 2009 8:32 PM (in response to timfox)

If you think, that lock is not a big deal (IMO at least)

... addData is already single thread at that point being protected by the appendLock on the journal. That's only avoiding the flush to happen.

The flush would happen on every X milliseconds and being a very fast operation as it will only transfer data to the writeExecutor (and writeQueue on AIO).
Actions

Go to original post