Groovy
  1. Groovy
  2. GROOVY-4246

Issue with incrementing array when using Random number generation

        Details

        • Type: Bug Bug
        • Status: Closed
        • Priority: Major Major
        • Resolution: Fixed
        • Affects Version/s: 1.7.2
        • Fix Version/s: 1.7.4, 1.8-beta-1
        • Component/s: None
        • Labels:
          None
        • Environment:
          Windows 7 64 bit

          Description

          I have found an issue with array incrementation, when using randomly generated number as array index.
          The idea was to randomly increment one of the indexes.
          For array of size = 3 and 10 incrementations I can see array was modified like this [example from system.out]:
          [0, 0, 1]
          [0, 0, 1]
          [2, 0, 1] should be [1,0,1]
          [2, 3, 1] should be [1,1,1]
          [3, 3, 1] should be [2,1,1]
          [3, 2, 1] now I'm puzzled as the digit is decremented!
          [3, 2, 1] what was incremented? no one knows! ;]
          [3, 2, 2]
          [3, 2, 3]
          [3, 3, 3]

          See the code attached.

          Additional info:
          When the randomiser invocation is moved to assign value to a variable, and then variable is used as array index it works fine.

          1. Text File
            4246_v18x.patch
            5 kB
            Roshan Dawrani
          2. File
            Bug.groovy
            0.3 kB
            Krystian Szczesny

            Activity

            Ascending order - Click to sort in descending order
            Hide
            Permalink
            Jochen Theodorou added a comment -

            after playing around a bit I think that the call to rand() is done multiple times... I added a simple counter to the method and found, that rand() is called 30 here. I think that a[exp]++ is converted into: 

            a.putAt(exp, a.getAt(exp).next())
a.getAt(exp)

            while the correct code should look more like:

            tmp = exp
a.putAt(tmp,a.getAt(tmp).next())
a.getAt(tmp)

            arguably the code could be improved further to do:

            tmp = exp
res = a.getAt(tmp).next()
a.putAt(tmp,res)
res

            This would save one more getAt call, increasin performance a little.

            Show
            Jochen Theodorou added a comment - after playing around a bit I think that the call to rand() is done multiple times... I added a simple counter to the method and found, that rand() is called 30 here. I think that a [exp] ++ is converted into: a.putAt(exp, a.getAt(exp).next()) a.getAt(exp) while the correct code should look more like: tmp = exp a.putAt(tmp,a.getAt(tmp).next()) a.getAt(tmp) arguably the code could be improved further to do: tmp = exp res = a.getAt(tmp).next() a.putAt(tmp,res) res This would save one more getAt call, increasin performance a little.
            Hide
            Permalink
            Roshan Dawrani added a comment -

            The given code consistently works if I change 

            arr[rand()]++

            to

            int x = rand()
arr[x]++

            More after some more investigation.

            Show
            Roshan Dawrani added a comment - The given code consistently works if I change arr[rand()]++ to int x = rand() arr[x]++ More after some more investigation.
            Hide
            Permalink
            Roshan Dawrani added a comment -

            Simultaneous posts. Looks like Jochen already investigated the cause.

            Show
            Roshan Dawrani added a comment - Simultaneous posts. Looks like Jochen already investigated the cause.
            Hide
            Permalink
            Roshan Dawrani added a comment -

            Hello.

            Attaching here a patch for review. Here is how the bytecode looks like after applying the patch for the postfix evaluation part of "arr[exp]++":

            Object tmp = exp;
Object localObject1 = "getAt".call(arr, tmp);
Object localObject2 = "next".call(localObject1);
"putAt".call(arr, tmp, localObject2); 
return localObject1;

            As captured in the test, the rand() expression is now correctly evaluated only 10 times instead of 30.

            Show
            Roshan Dawrani added a comment - Hello. Attaching here a patch for review. Here is how the bytecode looks like after applying the patch for the postfix evaluation part of "arr [exp] ++": Object tmp = exp; Object localObject1 = "getAt" .call(arr, tmp); Object localObject2 = "next" .call(localObject1); "putAt" .call(arr, tmp, localObject2); return localObject1; As captured in the test, the rand() expression is now correctly evaluated only 10 times instead of 30.
            Hide
            Permalink
            Jochen Theodorou added a comment -

            good work

            Show
            Jochen Theodorou added a comment - good work
            Hide
            Permalink
            Roshan Dawrani added a comment -

            Thanks Jochen.

            Show
            Roshan Dawrani added a comment - Thanks Jochen.
            Hide
            Permalink
            Krystian Szczesny added a comment -

            Thanks guys. Looking forward to stable release with this fix.
            Groovy will become groovier.

            Show
            Krystian Szczesny added a comment - Thanks guys. Looking forward to stable release with this fix. Groovy will become groovier.

              People

              • Assignee:
                Roshan Dawrani
                Reporter:
                Krystian Szczesny
              • Votes:
                0 Vote for this issue
                Watchers:
                0 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Development