Harmony
  1. Harmony
  2. HARMONY-6346

[classlib] [archive] Several archive bugfixes and optimizations

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0M12
    • Component/s: Classlib
    • Labels:
      None
    • Environment:
      SVN Revision: 820775
    • Patch Info:
      Patch Available
    • Estimated Complexity:
      Advanced

      Description

      The attached patch includes several miscellaneous bugfixes, optimizations and simplifications that we created for Android's copy of the archive module. Here's an overview of what's changed:

      InflaterInputStream
      Removes a bogus magic number check. Previously I submitted a patch to get this check to work; but the whole premise is flawed. To demonstrate, attempt to inflate data that was deflated using non-default settings.

      InflaterInputStream, ZipInputStream, GZIPInputStream
      These now make sure that available() returns 0 when the end of the stream is reached. Previously available() could return 1 even if read() was guaranteed to return -1.

      GZIPOutputStream
      Slight performance fix: cast from long to int only once

      JarFile
      Verifies the entry when the last byte is returned. This is similar to the available() problem in ZipInputStream etc.
      Move getMetaEntriesImpl() from native to Java.

      ZipFile
      Limit the amount of memory used while reading files. Previously we would create arbitrarily large buffers.
      Move several methods from native to Java.

      MsgHelp
      Keep resource bundles in memory with soft references
      Use the system classloader always.

      Tests
      New tests to demonstrate problems above, recovery on broken manifests, verification of empty entries

      Note that there are some problems with the patch, so it should not be applied! I will to upload an improved patch early next week. I figured I'd let everyone preview the patch in its current state; the problems are minor and include removing dead code and fixing tests with invalid assumptions.

      1. archive_from_android_2.patch
        146 kB
        Jesse Wilson
      2. archive_from_android.patch
        121 kB
        Jesse Wilson
      3. EmptyEntries_signed.jar
        2 kB
        Jesse Wilson

        Activity

        Hide
        Jesse Wilson added a comment -

        This completes the patch, addressing a few outstanding test problems and removing the obsolete native methods.

        In order to support data deflated with non-default settings, the bogus magic number check must be removed. This prevents us from failing early when inflating the single byte -1, but the code continues to detect a problem.

        This patch moves unzip from native code to Java. The primary benefit is that zip entries do not need to be fully-decompressed into an in-memory byte array. This is very important for mobile, and critical anywhere that zip files contain large entries. This regresses our ability to support reset() on zip file entry streams, but this limitation is consistent with the RI.

        Show
        Jesse Wilson added a comment - This completes the patch, addressing a few outstanding test problems and removing the obsolete native methods. In order to support data deflated with non-default settings, the bogus magic number check must be removed. This prevents us from failing early when inflating the single byte -1, but the code continues to detect a problem. This patch moves unzip from native code to Java. The primary benefit is that zip entries do not need to be fully-decompressed into an in-memory byte array. This is very important for mobile, and critical anywhere that zip files contain large entries. This regresses our ability to support reset() on zip file entry streams, but this limitation is consistent with the RI.
        Hide
        Jesse Wilson added a comment -

        Promoting to major priority. Streaming unzipped entries, and inflating files encoded with non-default settings are significant fixes to the current codebase.

        Show
        Jesse Wilson added a comment - Promoting to major priority. Streaming unzipped entries, and inflating files encoded with non-default settings are significant fixes to the current codebase.
        Hide
        Jesse Wilson added a comment -

        to working_classlib/support/src/test/java/tests/resources

        Show
        Jesse Wilson added a comment - to working_classlib/support/src/test/java/tests/resources
        Hide
        Tim Ellison added a comment -

        Thanks Jesse,

        Some interesting changes here. I've applied a subset to start with:

        • Having so many whitespace changes in there made it harder to review. While it is good to get formatting/tidy-up changes, best if they are in a separate patch for clarity sake.
        • I've not applied the changes to call the MsgHelp class (yet?). I think this is part of a larger discussion that we should have about error messages, but again in any case it feels like a different type of enhancement I can go on to look at now the others are in place.

        Please check that you are happy with the patch that was applied at repo revision r822846.

        I won't close the JIRA just yet so we won't forget the other improvements.

        Show
        Tim Ellison added a comment - Thanks Jesse, Some interesting changes here. I've applied a subset to start with: Having so many whitespace changes in there made it harder to review. While it is good to get formatting/tidy-up changes, best if they are in a separate patch for clarity sake. I've not applied the changes to call the MsgHelp class (yet?). I think this is part of a larger discussion that we should have about error messages, but again in any case it feels like a different type of enhancement I can go on to look at now the others are in place. Please check that you are happy with the patch that was applied at repo revision r822846. I won't close the JIRA just yet so we won't forget the other improvements.
        Show
        Tim Ellison added a comment - FYI the patch introduced new FindBugs warnings http://hudson.zones.apache.org/hudson/view/Harmony/job/Harmony-1.5-head-linux-x86_64/466/findbugsResult/new/
        Hide
        Jesse Wilson added a comment -

        Looks Good To Me.

        I reviewed the commits. The code looks great, even better that the patched I submitted. Thanks Tim.

        Show
        Jesse Wilson added a comment - Looks Good To Me. I reviewed the commits. The code looks great, even better that the patched I submitted. Thanks Tim.
        Hide
        Mark Hindess added a comment -

        The commit r822846 for this JIRA causes a regression in several tests in the sound module. Specifically:

        AudioSystemTest testMixer fails with junit.framework.AssertionFailedError: null at org.apache.harmony.sound.tests.javax.sound.sampled.AudioSystemTest.testMixer (AudioSystemTest.java:61) at java.lang.reflect.AccessibleObject.invokeV(AccessibleObject.java:197)

        AudioSystemTest testFormatConversion fails with junit.framework.AssertionFailedError: null at org.apache.harmony.sound.tests.javax.sound.sampled.AudioSystemTest.testFormatConversion (AudioSystemTest.java:128) at java.lang.reflect.AccessibleObject.invokeV(AccessibleObject.java:197)

        AudioSystemTest testAudioFile has error javax.sound.sampled.UnsupportedAudioFileException: File is not a supported file type at javax.sound.sampled.AudioSystem.getAudioFileFormat(AudioSystem.java:460) at org.apache.harmony.sound.tests.javax.sound.sampled.AudioSystemTest.testAudioFile (AudioSystemTest.java:43)

        AudioSystemTest testAudioInputStream has error javax.sound.sampled.UnsupportedAudioFileException: Could not get audio input stream from input file at javax.sound.sampled.AudioSystem.getAudioInputStream(AudioSystem.java:519) at org.apache.harmony.sound.tests.javax.sound.sampled.AudioSystemTest.testAudioInputStream (AudioSystemTest.java:98)

        AudioSystemTest testGetLine has error java.lang.IllegalArgumentException: Could not get line at javax.sound.sampled.AudioSystem.getLine(AudioSystem.java:270) at javax.sound.sampled.AudioSystem.getLine(AudioSystem.java:220) at org.apache.harmony.sound.tests.javax.sound.sampled.AudioSystemTest.testGetLine (AudioSystemTest.java:153)

        This is on linux/x86_64 though I think the failures might occur on other platforms too.

        Show
        Mark Hindess added a comment - The commit r822846 for this JIRA causes a regression in several tests in the sound module. Specifically: AudioSystemTest testMixer fails with junit.framework.AssertionFailedError: null at org.apache.harmony.sound.tests.javax.sound.sampled.AudioSystemTest.testMixer (AudioSystemTest.java:61) at java.lang.reflect.AccessibleObject.invokeV(AccessibleObject.java:197) AudioSystemTest testFormatConversion fails with junit.framework.AssertionFailedError: null at org.apache.harmony.sound.tests.javax.sound.sampled.AudioSystemTest.testFormatConversion (AudioSystemTest.java:128) at java.lang.reflect.AccessibleObject.invokeV(AccessibleObject.java:197) AudioSystemTest testAudioFile has error javax.sound.sampled.UnsupportedAudioFileException: File is not a supported file type at javax.sound.sampled.AudioSystem.getAudioFileFormat(AudioSystem.java:460) at org.apache.harmony.sound.tests.javax.sound.sampled.AudioSystemTest.testAudioFile (AudioSystemTest.java:43) AudioSystemTest testAudioInputStream has error javax.sound.sampled.UnsupportedAudioFileException: Could not get audio input stream from input file at javax.sound.sampled.AudioSystem.getAudioInputStream(AudioSystem.java:519) at org.apache.harmony.sound.tests.javax.sound.sampled.AudioSystemTest.testAudioInputStream (AudioSystemTest.java:98) AudioSystemTest testGetLine has error java.lang.IllegalArgumentException: Could not get line at javax.sound.sampled.AudioSystem.getLine(AudioSystem.java:270) at javax.sound.sampled.AudioSystem.getLine(AudioSystem.java:220) at org.apache.harmony.sound.tests.javax.sound.sampled.AudioSystemTest.testGetLine (AudioSystemTest.java:153) This is on linux/x86_64 though I think the failures might occur on other platforms too.
        Hide
        Mark Hindess added a comment -

        Not really must fix but we must resolve the regressions at least and I want to make sure it shows up in our release checklist.

        Show
        Mark Hindess added a comment - Not really must fix but we must resolve the regressions at least and I want to make sure it shows up in our release checklist.
        Hide
        Tim Ellison added a comment -

        I think this can be re-closed<g>, assuming the regressions Mark refers to are those in HARMONY-6386.

        Show
        Tim Ellison added a comment - I think this can be re-closed<g>, assuming the regressions Mark refers to are those in HARMONY-6386 .
        Hide
        Mark Hindess added a comment -

        Agreed. Thanks Tim. (It was late and I wanted to make sure this problem was captured somewhere before I went to sleep.

        Show
        Mark Hindess added a comment - Agreed. Thanks Tim. (It was late and I wanted to make sure this problem was captured somewhere before I went to sleep.

          People

          • Assignee:
            Tim Ellison
            Reporter:
            Jesse Wilson
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

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

                Development