5 Replies Latest reply on Jan 23, 2009 2:14 PM by clebert.suconic

    Some comments on Clebert's last commit

    timfox

      1) Forcing depage. I don't get this. Looking at the code I can see you force a depage if the reference isn't found in the queue

      a) I notice you only force one depage - How can you be sure the required message is in the next page? It might be in any page after that.

      b) Let's say you force a depage and the message *is* in there, the message will get depaged and routed to the queue. I can't see anywhere where the reference is then removed from the queue. Could this lead to duplicate deliveries?

      2) Address is being passed on replicate delivery mesage. This worries me for performance reasons. Can't we do this another way?

      3) Precalculation of flow control

      a) if ((creditsUsed -= chunk.getRequiredBufferSize()) < 0)

      Looks suspicious - this will actually decrement the creditsUsed variable

      b) Can you be sure that the first large message chunk gets sent iirespective or available credits? - we need to ensure this to avoid blocking

      c) pre-calculation of credits seems very complex... looks like it could do with some optimisation

        • 1. Re: Some comments on Clebert's last commit
          timfox

          One other thing I noticed is MultiThreadRandomFailoverTest is running a lot slower than it used to.

          Can you think of any changes made recently that might affect the speed of replication?

          (I'm thinking stuff done blocking now which was done non blocking before? etc )

          • 2. Re: Some comments on Clebert's last commit
            timfox

            Clebert - My understand is that your changes in your last commit should have *zero* run-time overhead for the standard message and not paging case, is this correct?

            I just want to confirm that any code changes you made do not affect anything in the standard use case.

            • 3. Re: Some comments on Clebert's last commit
              clebert.suconic

               

              "Tim Fox" wrote:
              1) Forcing depage. I don't get this. Looking at the code I can see you force a depage if the reference isn't found in the queue

              a) I notice you only force one depage - How can you be sure the required message is in the next page? It might be in any page after that.

              b) Let's say you force a depage and the message *is* in there, the message will get depaged and routed to the queue. I can't see anywhere where the reference is then removed from the queue. Could this lead to duplicate deliveries?


              There's a loop there:
              for (;;) // I'm changing this for while(true)
               {
               // Can't have the same store being depaged in more than one thread
               synchronized (store)
               {
               // as soon as it gets the lock, it needs to verify if another thread couldn't find the reference
               ref = messageQueue.removeReferenceWithID(id);
               if (ref == null)
               {
               // force a depage
               if (!store.readPage()) // the only time this is false is when you don't have more pages
               {
               break;
               }
               }
               else
               {
               break;
               }
               }
               }
              



              And on the backup, that's the only time we depage until it is activated

              "timfox" wrote:

              2) Address is being passed on replicate delivery mesage. This worries me for performance reasons. Can't we do this another way?


              I have removed it here locally. I'll use the messageQueue.getName() :-)

              "timfox" wrote:

              3) Precalculation of flow control

              a) if ((creditsUsed -= chunk.getRequiredBufferSize()) < 0)

              Looks suspicious - this will actually decrement the creditsUsed variable



              I'm renaming this variable to precalculateAvailableCredits.
              We take the credits in advance from the preCalculateFlowControl, and I keep sending while I have precalculateAvailableCredits

              "timfox" wrote:


              b) Can you be sure that the first large message chunk gets sent iirespective or available credits? - we need to ensure this to avoid blocking


              I will take a look, but how this would avoid blocking?



              "Tim Fox" wrote:

              c) pre-calculation of credits seems very complex... looks like it could do with some optimisation



              I don't know...

              All the equations I tried had an indefinition where I would need a loop series to solve it.

              I will try to think a little bit more.


              "Tim Fox" wrote:


              MultiThreadRandomFailoverTest is running a lot slower




              I just did a comparisson before and after my commit, and it ran the same.

              However the test runs much slower on Hudson. Maybe there is a VM on the non-vm queue by accident?

              • 4. Re: Some comments on Clebert's last commit
                clebert.suconic

                 

                "timfox" wrote:
                Clebert - My understand is that your changes in your last commit should have *zero* run-time overhead for the standard message and not paging case, is this correct?

                I just want to confirm that any code changes you made do not affect anything in the standard use case.


                Just found what is likely the cause of this.

                *Lots* of "JBM_ROUTE_TO:" are being added to the MessageHeader on QueueImpl::route, and that is being sent to the client.

                On route, there is a comment saying this is is a temporary measure:

                QueueImpl::route
                 // Temp
                 SimpleString routeToHeader = MessageImpl.HDR_ROUTE_TO_PREFIX.concat(name);
                


                • 5. Re: Some comments on Clebert's last commit
                  clebert.suconic

                  route should be removing it.. but for some reason that still exists on the backup, and that is also being replicated.


                  I locally hacked the code to print the properties, and those properties will exist right before the message is delivered.