1 2 Previous Next 16 Replies Latest reply on Dec 22, 2008 3:21 AM by Tim Fox

    Paging code review

    Tim Fox Master

      As discussed in our code review yesterday, here are the outstanding issues in paging that need to be addresses.

      Some should already be covered in JIRA tasks. If the others don't have JIRAs they need to be covered by at least one, if not fixed immediately.

      1. Use OrdereredExecutorFactory for thread management. This should make the global pager redundant.

      2. We should ignore corrupt pages on load, and log a clear warning in the logs so the user can address the issue.

      3. Much of the functionality currently in PagingManagerImpl is better placed in PagingStoreImpl, e.g. addSize, ondePage. This would also simplify the code since less lookups would be needed and several methods could be removed or made private.

      4. PagingStoreImpl - The locking is very complex. Can this be simplified by using Condition objects with the lock? Or perhaps we can disallow concurrent writes to the same page - which would simplify the locking. IS there really much performance to be gained by concurrent writes to the same page? Can this be verified with a test?

      5. PagingStoreImpl:

      if (returnPage.getNumberOfMessages() == 0)

      How can number of messages in a page be zero? Clebert to check

      6. PagingStoreImpl::page()

      Calculation of block size and lock acquisition:

      int bytesToWrite = fileFactory.calculateBlockSize(message.getEncodeSize() + PageImpl.SIZE_RECORD);

      // The only thing single-threaded done on paging is positioning and
      // check-files (verifying if we need to open a new page file)
      positioningGlobalLock.acquire();

      Can this be *after* the currentPage check - since in 99% of cases paging is not occurring and we don't want to get locks / do calculations unnecessarily.

      7. PagingStoreImpl::page

      pagedUsedSize is being incremented twice?

      8. TransactionImpl:

      An address can switch from paging to not paging mid way during a transaction before it is committed, resulting in the transactions messages being routed out of order

      9. TransactionImpl:

      Currently paged messages are paged on commit(). If there are a lot of paged messages this could hold up the remoting thread for a significant amount of time,

      One of the design principles of the server is each operation should be quick to complete to avoid starving other operations.l

      We should consider instead, writing transactional actions straight to a file in the paging case, then on commit, since we know they are on file we can process them on a different thread and the commit can return ok immediately. This would also allow us to deal with huge transactions, much bigger than can fit in memory at once

      10. Currently routing code is duplicated in ServerSessionImpl::send, TransactionImpl and in depage logic. This should be put in one place.

        • 1. Re: Paging code review
          Clebert Suconic Master

          A lot of the minor tasks are done already...

          I have just a point regarding the corrupt pages. I'm going to measure if we need concurrent writes (they were written for AIO), and if they are, we still need to deal with holes (even on NIO).

          Only thing is I need to add a counter per page, not just a global counter any more, same way we do on the journal.

          But if we disable concurrent writes this is moot. Let me check it first.


          • 2. Re: Paging code review
            Tim Fox Master

            One other point:

            11) Need to sync paging on non transactional persistent page, if corresponding config property is set

            • 3. Re: Paging code review
              Tim Fox Master

              12) If paging address is used in directory name we need to ensure it works if the paging address contains characters like "/", "\" or other characters (non ascii etc).

              • 4. Re: Paging code review
                Clebert Suconic Master

                just to register this somewhere..

                For 12) We talked about using org.jboss.messaging.util.Base64 for the directory name.

                • 5. Re: Paging code review
                  Clebert Suconic Master


                  1. Use OrdereredExecutorFactory for thread management. This should make the global pager redundant.


                  I have this done on my local copy already.. I'm just waiting some tests to finish before I commit this tomorrow.

                  3. Much of the functionality currently in PagingManagerImpl is better placed in PagingStoreImpl, e.g. addSize, ondePage. This would also simplify the code since less lookups would be needed and several methods could be removed or made private.


                  done

                  4. PagingStoreImpl - The locking is very complex. Can this be simplified by using Condition objects with the lock? Or perhaps we can disallow concurrent writes to the same page - which would simplify the locking. IS there really much performance to be gained by concurrent writes to the same page? Can this be verified with a test?


                  The readWriteLock is used to avoid synchronized access on the verification on isPaging(). I wanted to not create any contention when paging is not activated.

                  I have changed the code not allow multiple writes, but the lock schema is pretty much the same.


                  6. PagingStoreImpl::page()

                  Calculation of block size and lock acquisition:



                  done


                  7. PagingStoreImpl::page

                  pagedUsedSize is being incremented twice?



                  Not really.. openNewPage is setting it to 0, then it needs to be added again.


                  8. TransactionImpl:

                  An address can switch from paging to not paging mid way during a transaction before it is committed, resulting in the transactions messages being routed out of order

                  9. TransactionImpl:

                  Currently paged messages are paged on commit(). If there are a lot of paged messages this could hold up the remoting thread for a significant amount of time,

                  One of the design principles of the server is each operation should be quick to complete to avoid starving other operations.l

                  We should consider instead, writing transactional actions straight to a file in the paging case, then on commit, since we know they are on file we can process them on a different thread and the commit can return ok immediately. This would also allow us to deal with huge transactions, much bigger than can fit in memory at once



                  I want to do that as part HugeTransaction task.

                  10. Currently routing code is duplicated in ServerSessionImpl::send, TransactionImpl and in depage logic. This should be put in one place.


                  This goes beyond of paging... but I will do that tomorrow.


                  I'm still missing 2, but that should be easy. (Unless we decide to do something more complex such as hole-detection the way I have done on the Journal).

                  • 6. Re: Paging code review
                    Tim Fox Master

                    What about 5) ?

                    • 7. Re: Paging code review
                      Clebert Suconic Master

                      5) The tricky part on paging is to let depage happen at the same time messages are being paged.

                      Depage will aways return old pages, but when depageRate>pageRate, we will eventually be depaging the currentPage, on which case I open a newPage and return the current one.

                      But if no messages were produced on that new page, it will be empty. When we return that empty page, we will exit the pageMode and from that point we will start using the journal again.

                      This is done this way because I can't lock page while depaging records. It all happens concurrently. Once an address enters in pageMode, it will be in pageMode until the rate of removing messages on page is higher the rate on adding messages from page.

                      • 8. Re: Paging code review
                        Clebert Suconic Master

                        The only missing parts now are:

                        13) Add a limit number of Threads limit on the Main Executor for depage. (Which is a very minor task)

                        8-9) it will be made as part of another task

                        and


                        10) Which is not directly related to Paging itself.

                        • 9. Re: Paging code review
                          Tim Fox Master

                          In your latest commit flow control appears broken, and not implemented as we discussed.

                          This needs to be discussed later

                          • 10. Re: Paging code review
                            Clebert Suconic Master

                            My idea yesterday was to use the Header Size as the last chunk size on flow control, but in a second thought yes this is broken as I could have messages never arriving as the very first flow control was not confirmed.

                            I'm fixing it.

                            I will add a property on ClientMessage to store the next flowControl to be used. On regular message that will aways be the encodeSize, and on LargeMessages the size of the last chunk.

                            • 11. Re: Paging code review
                              Clebert Suconic Master

                              10) I have added three methods on PostOffice for this:

                               /** Deliver references previously routed */
                               void deliver(List<MessageReference> references);
                              
                               void scheduleReferences(long scheduledDeliveryTime, List<MessageReference> references) throws Exception;
                              
                               void scheduleReferences( long transactionID, long scheduledDeliveryTime, List<MessageReference> references) throws Exception;
                              
                              



                              I think the Postoffice would be the right place, since this is related to what needs to be done post-routing, during commit.. depage.. etc. If that's not the right place it should be an easy move though.


                              - I have also fixed ScheduledDeliveries on Paging, and few other issues with sizing and Expiry on Paging.

                              • 12. Re: Paging code review
                                Tim Fox Master

                                 

                                "timfox" wrote:
                                12) If paging address is used in directory name we need to ensure it works if the paging address contains characters like "/", "\" or other characters (non ascii etc).


                                Is this done yet? (Doesn't look like it).

                                If not, please make sure it is tracked in a JIRA.

                                Actually, can you look through all the paging and large message threads and make sure everything that is not complete yet is tracked properly. It seems we have some loose ends here.....


                                • 13. Re: Paging code review
                                  Tim Fox Master

                                   

                                  "timfox" wrote:
                                  "timfox" wrote:
                                  12) If paging address is used in directory name we need to ensure it works if the paging address contains characters like "/", "\" or other characters (non ascii etc).


                                  Is this done yet? (Doesn't look like it).


                                  Ah sorry. I can see it is done.

                                  One thing:

                                  final String destinationDirectory = directory + "/" + Base64.encodeBytes(destinationName.getData(), Base64.URL_SAFE);
                                  


                                  Is it same to assume "/" is file separator on all OSes?

                                  Should we use Systm.getproperty("file.separator") ?




                                  • 14. Re: Paging code review
                                    Clebert Suconic Master

                                    I have used '/' in both Windows and Linux, and it has aways worked.


                                    We can change that if you prefer.

                                    1 2 Previous Next