12 Replies Latest reply on Nov 30, 2009 8:32 PM by Clebert Suconic

    Locking in TimedBuffer

    Tim Fox Master

      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
          Tim Fox Master

          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)

          • 2. Re: Locking in TimedBuffer
            Clebert Suconic Master

            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.

            • 3. Re: Locking in TimedBuffer
              Clebert Suconic Master

               

              "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?"

              • 4. Re: Locking in TimedBuffer
                Clebert Suconic Master

                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.

                • 5. Re: Locking in TimedBuffer
                  Tim Fox Master

                   

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

                  • 6. Re: Locking in TimedBuffer
                    Tim Fox Master

                     

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

                    • 7. Re: Locking in TimedBuffer
                      Clebert Suconic Master

                      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.

                      • 8. Re: Locking in TimedBuffer
                        Tim Fox Master

                        Can you rephrase your last post?

                        I am having trouble understanding it.

                        • 9. Re: Locking in TimedBuffer
                          Clebert Suconic Master

                          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.

                          • 10. Re: Locking in TimedBuffer
                            Tim Fox Master

                            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 Master

                              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.

                              • 12. Re: Locking in TimedBuffer
                                Clebert Suconic Master

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