groovy
  1. groovy
  2. GROOVY-4246

Issue with incrementing array when using Random number generation

    Details

    • Type: Bug Bug
    • Status: Closed 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
    • Testcase included:
      yes
    • Number of attachments :
      2

      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. 4246_v18x.patch
        5 kB
        Roshan Dawrani
      2. Bug.groovy
        0.3 kB
        Krystian Szczesny

        Activity

        Hide
        blackdrag blackdrag 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
        blackdrag blackdrag 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
        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
        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
        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
        blackdrag blackdrag added a comment -

        good work

        Show
        blackdrag blackdrag added a comment - good work
        Hide
        Roshan Dawrani added a comment -

        Thanks Jochen.

        Show
        Roshan Dawrani added a comment - Thanks Jochen.
        Hide
        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: