Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.3.1, 6.0
    • Component/s: modules/highlighter
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This test fail reproduces on IBM J9:

      NOTE: reproduce with: ant test  -Dtestcase=TestPostingsHighlighter -Dtests.method=testCambridgeMA -Dtests.seed=2A9A93DAC39E0938 -Dtests.multiplier=3 -Dtests.slow=true -Dtests.locale=es_HN -Dtests.timezone=America/Yellowknife -Dtests.file.encoding=UTF-8
      
      Stack Trace:
      java.lang.ArrayIndexOutOfBoundsException: Array index out of range: 37
              at __randomizedtesting.SeedInfo.seed([2A9A93DAC39E0938:AB8FF071AD305139]:0)
              at org.apache.lucene.search.postingshighlight.Passage.addMatch(Passage.java:53)
              at org.apache.lucene.search.postingshighlight.PostingsHighlighter.highlightDoc(PostingsHighlighter.java:547)
              at org.apache.lucene.search.postingshighlight.PostingsHighlighter.highlightField(PostingsHighlighter.java:425)
              at org.apache.lucene.search.postingshighlight.PostingsHighlighter.highlightFields(PostingsHighlighter.java:364)
              at org.apache.lucene.search.postingshighlight.PostingsHighlighter.highlightFields(PostingsHighlighter.java:268)
              at org.apache.lucene.search.postingshighlight.PostingsHighlighter.highlight(PostingsHighlighter.java:198)
              at org.apache.lucene.search.postingshighlight.TestPostingsHighlighter.testCambridgeMA(TestPostingsHighlighter.java:373)
      

      I think it's because J9 grows arrays in a different progression than other JVMs ... we should fix PostingsHighlighter to forcefully grow the arrays to the same length instead of this:

          if (numMatches == matchStarts.length) {
            matchStarts = ArrayUtil.grow(matchStarts, numMatches+1);
            matchEnds = ArrayUtil.grow(matchEnds, numMatches+1);
            BytesRef newMatchTerms[] = new BytesRef[ArrayUtil.oversize(numMatches+1, RamUsageEstimator.NUM_BYTES_OBJECT_REF)];
            System.arraycopy(matchTerms, 0, newMatchTerms, 0, numMatches);
            matchTerms = newMatchTerms;
          }
      
      1. LUCENE-4948.patch
        2 kB
        Robert Muir
      2. LUCENE-4948.patch
        2 kB
        Robert Muir

        Issue Links

          Activity

          Hide
          Robert Muir added a comment -

          Thanks for looking into this Mike! I agree the current code is wrong (stinky)

          Show
          Robert Muir added a comment - Thanks for looking into this Mike! I agree the current code is wrong (stinky)
          Hide
          Uwe Schindler added a comment - - edited

          This is easy to explain: J9 does not know compressed oops. On Oracle 64 bit JVMs the pointers are compressed, so a reference is 32 bits, which is identical to the size of ints. So they grow identical. In IBM J9 (64 bit, it only fails with 64 bits!) a reference is 8 bytes. So the reference array grows different than the int[] array.

          This bug also affect useres with Oracle JDK, if they have > 32 Gig of heap...

          Whenever we use ArrayUtil.grow() we have to take care that the sizes may differ. Please don't change the growing pattern, the code should (when it requires that several arrays are sized in parallel) calculate the new size before and use a suitable type to do that.

          A good test would be: We make the growing pattern in ArrayUtil allow "test mode" maybe and make the factor random? Just an idea. Maybe we can do this before running tests using bytecode modification.

          Show
          Uwe Schindler added a comment - - edited This is easy to explain: J9 does not know compressed oops. On Oracle 64 bit JVMs the pointers are compressed, so a reference is 32 bits, which is identical to the size of ints. So they grow identical. In IBM J9 (64 bit, it only fails with 64 bits!) a reference is 8 bytes. So the reference array grows different than the int[] array. This bug also affect useres with Oracle JDK, if they have > 32 Gig of heap... Whenever we use ArrayUtil.grow() we have to take care that the sizes may differ. Please don't change the growing pattern, the code should (when it requires that several arrays are sized in parallel) calculate the new size before and use a suitable type to do that. A good test would be: We make the growing pattern in ArrayUtil allow "test mode" maybe and make the factor random? Just an idea. Maybe we can do this before running tests using bytecode modification.
          Hide
          Uwe Schindler added a comment -

          Another idea: I can configure Policeman Jenkins to sometimes run test without compressed oops. Thats the quickest fix. Then J9 is not the only JVM that tests those bugs. And its good to have another random JVM param.

          Show
          Uwe Schindler added a comment - Another idea: I can configure Policeman Jenkins to sometimes run test without compressed oops. Thats the quickest fix. Then J9 is not the only JVM that tests those bugs. And its good to have another random JVM param.
          Hide
          Uwe Schindler added a comment -

          FYI: The test fails always on Oracle, too, if you run your 64 bit Oracle JVM with:

          $ ant test -Dargs="-XX:-UseCompressedOops" -Dtestcase=TestPostingsHighlighter
          [...]
          [junit4:junit4] Tests with failures:
          [junit4:junit4]   - org.apache.lucene.search.postingshighlight.TestPostingsHighlighter.testCambridgeMA
          
          Show
          Uwe Schindler added a comment - FYI: The test fails always on Oracle, too, if you run your 64 bit Oracle JVM with: $ ant test -Dargs="-XX:-UseCompressedOops" -Dtestcase=TestPostingsHighlighter [...] [junit4:junit4] Tests with failures: [junit4:junit4] - org.apache.lucene.search.postingshighlight.TestPostingsHighlighter.testCambridgeMA
          Hide
          Uwe Schindler added a comment -

          +1 for committing the patch. It should pass with above ant-line on Oracle, if you don't have a IBM J9 installed.

          Show
          Uwe Schindler added a comment - +1 for committing the patch. It should pass with above ant-line on Oracle, if you don't have a IBM J9 installed.
          Hide
          Robert Muir added a comment -

          there was still a stinkbug in the patch, i wanted to make it clear all are the same size

          Show
          Robert Muir added a comment - there was still a stinkbug in the patch, i wanted to make it clear all are the same size
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] rmuir
          http://svn.apache.org/viewvc?view=revision&revision=1470642

          LUCENE-4948: Fix stinkbug in PostingsHighlighter (wrong array sizing on some jvms)

          Show
          Commit Tag Bot added a comment - [trunk commit] rmuir http://svn.apache.org/viewvc?view=revision&revision=1470642 LUCENE-4948 : Fix stinkbug in PostingsHighlighter (wrong array sizing on some jvms)
          Hide
          Commit Tag Bot added a comment -

          [branch_4x commit] rmuir
          http://svn.apache.org/viewvc?view=revision&revision=1470643

          LUCENE-4948: Fix stinkbug in PostingsHighlighter (wrong array sizing on some jvms)

          Show
          Commit Tag Bot added a comment - [branch_4x commit] rmuir http://svn.apache.org/viewvc?view=revision&revision=1470643 LUCENE-4948 : Fix stinkbug in PostingsHighlighter (wrong array sizing on some jvms)
          Hide
          Robert Muir added a comment -

          Thanks guys.

          Another idea: I can configure Policeman Jenkins to sometimes run test without compressed oops. Thats the quickest fix. Then J9 is not the only JVM that tests those bugs. And its good to have another random JVM param.

          +1

          Show
          Robert Muir added a comment - Thanks guys. Another idea: I can configure Policeman Jenkins to sometimes run test without compressed oops. Thats the quickest fix. Then J9 is not the only JVM that tests those bugs. And its good to have another random JVM param. +1
          Hide
          Uwe Schindler added a comment -

          I changed the Policeman Jenkins configs to randomly also select (un)compressed oops on 64 bit JVMs. For 32 bit JVMs it only chooses -client and -server, so this is now similar.

          Show
          Uwe Schindler added a comment - I changed the Policeman Jenkins configs to randomly also select (un)compressed oops on 64 bit JVMs. For 32 bit JVMs it only chooses -client and -server, so this is now similar.
          Hide
          Steve Rowe added a comment -

          If there are no objections, I'd like to backport this to 4.3.1.

          Show
          Steve Rowe added a comment - If there are no objections, I'd like to backport this to 4.3.1.
          Hide
          Shalin Shekhar Mangar added a comment -

          Backported to 4.3.1 r1483258

          Show
          Shalin Shekhar Mangar added a comment - Backported to 4.3.1 r1483258
          Hide
          Shalin Shekhar Mangar added a comment -

          Bulk closing after 4.3.1 release

          Show
          Shalin Shekhar Mangar added a comment - Bulk closing after 4.3.1 release

            People

            • Assignee:
              Unassigned
              Reporter:
              Michael McCandless
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development