Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This outputs zlib header/footer and computes adler32 for each block. The space is nothing, but the adler32 computation on encode/decode has a cost, and we already have our own checksum.

      Since we currently compress/decompress at merge, this reduces the overall time of merging stored fields with deflate vs lz4, from 1.8x to 1.5x, reducing some of the pain.

      1. LUCENE-6090.patch
        2 kB
        Robert Muir
      2. LUCENE-6090.patch
        1 kB
        Robert Muir
      3. LUCENE-6090.patch
        1 kB
        Robert Muir

        Activity

        Hide
        Robert Muir added a comment -

        updated patch, explicitly set dummy byte to 0. this is paranoia since only an ancient zlib needs it, but the javadocs say its required.

        Show
        Robert Muir added a comment - updated patch, explicitly set dummy byte to 0. this is paranoia since only an ancient zlib needs it, but the javadocs say its required.
        Hide
        Adrien Grand added a comment -

        I am wondering if this dummy byte is actually required. Although the javadocs state so, I found this comment from Mark Adler who says that it was only required until zlib 1.1.4: http://stackoverflow.com/questions/9770364/decompressing-a-string-using-java-util-zip-inflater (see 2nd answer).

        If we want to keep it for conformity with javadocs, can you put a comment that Inflater requires it?

        Show
        Adrien Grand added a comment - I am wondering if this dummy byte is actually required. Although the javadocs state so, I found this comment from Mark Adler who says that it was only required until zlib 1.1.4: http://stackoverflow.com/questions/9770364/decompressing-a-string-using-java-util-zip-inflater (see 2nd answer). If we want to keep it for conformity with javadocs, can you put a comment that Inflater requires it?
        Hide
        Robert Muir added a comment -

        Yeah its a very ancient thing. But we should follow the javadocs here. I will add another code comment about this.

        Show
        Robert Muir added a comment - Yeah its a very ancient thing. But we should follow the javadocs here. I will add another code comment about this.
        Hide
        Robert Muir added a comment -

        Thanks adrien: i updated with docs. I also cleaned up this dummy byte padding and this fixed a bug found by tests

        Show
        Robert Muir added a comment - Thanks adrien: i updated with docs. I also cleaned up this dummy byte padding and this fixed a bug found by tests
        Hide
        Adrien Grand added a comment -

        +1

        Show
        Adrien Grand added a comment - +1
        Hide
        Anshum Gupta added a comment -

        Bulk close after 5.0 release.

        Show
        Anshum Gupta added a comment - Bulk close after 5.0 release.

          People

          • Assignee:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development