Solr
  1. Solr
  2. SOLR-539

Statistics variable atomicity in RequestHandlerBase

    Details

      Description

      Writes are not atomic on longs unless they are volatile. At a minimum, numRequests, numErrors, and totalTime should be marked volatile. More correct, but higher overhead due to contention and synchronization would be to use AtomicLong.

      There is a minor error in the calculation of avgRequestsPerSecond. The first operand of the denominator is cast to a float then has a long subtracted from it, in effect casting both longs to floats then performing the subtraction. It is better to do the subtraction as longs then cast to a float.

        Activity

        Hide
        Sean Timm added a comment -

        This patch marks numRequests, numErrors, and totalTime volatile. It also changes the casting in the calculation of avgRequestsPerSecond.

        Show
        Sean Timm added a comment - This patch marks numRequests, numErrors, and totalTime volatile. It also changes the casting in the calculation of avgRequestsPerSecond.
        Hide
        Sean Timm added a comment -

        This patch changes numRequests, numErrors, and totalTime to AtomicLongs. It also changes the casting in the calculation of avgRequestsPerSecond.

        Show
        Sean Timm added a comment - This patch changes numRequests, numErrors, and totalTime to AtomicLongs. It also changes the casting in the calculation of avgRequestsPerSecond.
        Hide
        Sean Timm added a comment -

        I don't know that I explained the problem with the calculation of avgRequestsPerSecond well. Here is a code snippet that illustrates the problem:

        public class CastingTest {
          public static void main(String args[]) throws Exception{
            long t1 = System.currentTimeMillis();
            long t2 = t1 + 7456L;
            System.out.println("Cast then subtract: " + ((float)t2 - t1));
            System.out.println("Subtract then cast: " + (float)(t2 - t1));
          }
        }
        

        Output:

        % java CastingTest
        Cast then subtract: 0.0
        Subtract then cast: 7456.0
        

        So, at least shortly after start up, the way the casting is currently done results in a divide by zero.

        Show
        Sean Timm added a comment - I don't know that I explained the problem with the calculation of avgRequestsPerSecond well. Here is a code snippet that illustrates the problem: public class CastingTest { public static void main( String args[]) throws Exception{ long t1 = System .currentTimeMillis(); long t2 = t1 + 7456L; System .out.println( "Cast then subtract: " + (( float )t2 - t1)); System .out.println( "Subtract then cast : " + ( float )(t2 - t1)); } } Output: % java CastingTest Cast then subtract: 0.0 Subtract then cast: 7456.0 So, at least shortly after start up, the way the casting is currently done results in a divide by zero.
        Hide
        Sean Timm added a comment -

        Here is an illustration of the divide by zero bug. Notice the value of avgRequestsPerSecond. This is two minutes after server start.

        stats: 	requests : 4255
                errors : 0
                avgTimePerRequest : 196.68672
                avgRequestsPerSecond : Infinity
        
        Show
        Sean Timm added a comment - Here is an illustration of the divide by zero bug. Notice the value of avgRequestsPerSecond. This is two minutes after server start. stats: requests : 4255 errors : 0 avgTimePerRequest : 196.68672 avgRequestsPerSecond : Infinity
        Hide
        Otis Gospodnetic added a comment -

        Merci. I applied the volatile variation.

        Committed revision 658977.

        Show
        Otis Gospodnetic added a comment - Merci. I applied the volatile variation. Committed revision 658977.

          People

          • Assignee:
            Otis Gospodnetic
            Reporter:
            Sean Timm
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development