Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.0-ALPHA, 6.0
    • Fix Version/s: 4.0-BETA
    • Component/s: core/other
    • Labels:
    • Environment:

      Tested with 4 different Intel machines

    • Lucene Fields:
      Patch Available

      Description

      Based on the performance measurements of PackedInts.Mutable's in LUCENE-4062, a new version of Packed64 has been created that is consistently faster than the old Packed64 for both get and set.

      1. LUCENE-4171.patch
        11 kB
        Toke Eskildsen
      2. LUCENE-4171.patch
        11 kB
        Toke Eskildsen
      3. LUCENE-4171.patch
        12 kB
        Toke Eskildsen
      4. LUCENE-4171.patch
        12 kB
        Toke Eskildsen

        Activity

        Hide
        Toke Eskildsen added a comment -

        Finished implementation, ready for review & potential merge. TestPackedInts passes.

        Show
        Toke Eskildsen added a comment - Finished implementation, ready for review & potential merge. TestPackedInts passes.
        Hide
        Adrien Grand added a comment -

        Patch looks very good, all core tests pass on my computer too. Maybe still two minor things to fix before commit:

        • you shouldn't override getArray. This method must return a non-null array only when the number of bits per value matches the backing array type (some components use it to directly work with an array instead of a PackedInts.Reader).
        • could you use lower camelCase for instance variables (MASK_LEFT, MASK_RIGHT and BPV_MINUS_BLOCK_SIZE)?
        Show
        Adrien Grand added a comment - Patch looks very good, all core tests pass on my computer too. Maybe still two minor things to fix before commit: you shouldn't override getArray . This method must return a non-null array only when the number of bits per value matches the backing array type (some components use it to directly work with an array instead of a PackedInts.Reader). could you use lower camelCase for instance variables (MASK_LEFT, MASK_RIGHT and BPV_MINUS_BLOCK_SIZE)?
        Hide
        Toke Eskildsen added a comment -

        I did wonder about the getArray-thing, but I did not look all the way back to the Reader interface, where the JavaDoc is very clear on the subject. I have removed the method and corrected the variable names.

        Show
        Toke Eskildsen added a comment - I did wonder about the getArray-thing, but I did not look all the way back to the Reader interface, where the JavaDoc is very clear on the subject. I have removed the method and corrected the variable names.
        Hide
        Adrien Grand added a comment -

        Thanks, Toke. I think there are still a few minor fixes to perform:

        • the maskLeft variable seems unused, can it be removed?
        • the copyright header doens't need to be a javadoc comment (those that start with /** instead of // or /*), can you fix it?
        • since this impl uses the default format, there is no need to override getFormat

        I think it is not necessary to try to keep the same number of blocks as the old impl (cf. comments in ctor #1 and #3). Could we just use size(valueCount, bitsPerValue) to compute the required number of blocks?

        Show
        Adrien Grand added a comment - Thanks, Toke. I think there are still a few minor fixes to perform: the maskLeft variable seems unused, can it be removed? the copyright header doens't need to be a javadoc comment (those that start with /** instead of // or /*), can you fix it? since this impl uses the default format, there is no need to override getFormat I think it is not necessary to try to keep the same number of blocks as the old impl (cf. comments in ctor #1 and #3). Could we just use size(valueCount, bitsPerValue) to compute the required number of blocks?
        Hide
        Toke Eskildsen added a comment -

        I have corrected the issues. I do worry a little bit about the block count thing: As the change is seen in the constructor and the reader, and the writer uses valueCount, the persistent format should be the same, right?

        Show
        Toke Eskildsen added a comment - I have corrected the issues. I do worry a little bit about the block count thing: As the change is seen in the constructor and the reader, and the writer uses valueCount, the persistent format should be the same, right?
        Hide
        Adrien Grand added a comment -

        Even if Packed64 used one or two additional blocks to avoid branches, the reader and the writer always read/wrote the exact number of required blocks (this is why ctor #3 reads size longs instead of blocks.length == size + 1 blocks). So this should not be a problem.

        Can you fix ctor #1 so that it uses the size method instead of valueCount * bitsPerValue / BLOCK_SIZE + 1 (this may create one more block than required: for example, if bitsPerValue=1 and valueCount=64, blocks will have a length of 2 while only 1 block is required)?

        Show
        Adrien Grand added a comment - Even if Packed64 used one or two additional blocks to avoid branches, the reader and the writer always read/wrote the exact number of required blocks (this is why ctor #3 reads size longs instead of blocks.length == size + 1 blocks). So this should not be a problem. Can you fix ctor #1 so that it uses the size method instead of valueCount * bitsPerValue / BLOCK_SIZE + 1 (this may create one more block than required: for example, if bitsPerValue=1 and valueCount=64, blocks will have a length of 2 while only 1 block is required)?
        Hide
        Toke Eskildsen added a comment -

        Attached patch with constructor #1 size calculation change. Unit-test still passes.

        Show
        Toke Eskildsen added a comment - Attached patch with constructor #1 size calculation change. Unit-test still passes.
        Hide
        Adrien Grand added a comment -

        Committed (r1355346 and r1355352 on trunk, r1355359 on branch_4x).

        Thanks a lot, Toke!

        Show
        Adrien Grand added a comment - Committed (r1355346 and r1355352 on trunk, r1355359 on branch_4x). Thanks a lot, Toke!
        Hide
        Hoss Man added a comment -

        hoss20120711-manual-post-40alpha-change

        Show
        Hoss Man added a comment - hoss20120711-manual-post-40alpha-change

          People

          • Assignee:
            Adrien Grand
            Reporter:
            Toke Eskildsen
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 4h
              4h
              Remaining:
              Remaining Estimate - 4h
              4h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development