Derby
  1. Derby
  2. DERBY-5786

Duplicate copies of InputStreamUtil.java and DynamicByteArrayOutputStream.java classes in client.net package

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.10.1.1
    • Component/s: Network Client
    • Labels:
      None

      Description

      In solving DERBY-4491, InputStreamUtil.java and DynamicByteArrayOutputStream.java classes which are present in org.apache.derby.iapi.services.io package was needed for the network client also. So now there are duplicate copies of those classes in org.apache.derby.client.net package.

      But the client uses only a small part of the functionality those classes provide. In a discussion in dev list (http://mail-archives.apache.org/mod_mbox/db-derby-dev/201205.mbox/%3Cwjo8r4ucgtfv.fsf%40oracle.com%3E) it was decided to use EncodedInputStream.PublicBufferOutputStream which is already in the client.net package instead of the duplicated classes. In order to do this PublicBufferOutputStream will first be made into a stand-alone class.

      Doing this would help to reduce the code size and increase code coverage, as the duplicate classes currently have no code coverage.

      1. DERBY-5786.patch
        16 kB
        Mohamed Nufail

        Activity

        Hide
        Mohamed Nufail added a comment -

        I analysed the usages of InputStreamUtil.java and DynamicByteArrayOutputStream.java in the network client. InputStreamUtil is used only inside a method in DynamicByteArrayOutputStream which is never used. So it can be removed without a problem.

        But DynamicByteArrayOutputStream has some dynamic resizing abilities, optimized for performance and memory reduction. If PublicBufferOutputStream is used, it will only have ByteArrayOutputStream's resizing capability which just doubles the size of the byte array. So will it be safe to replace DynamicByteArrayOutputStream with PublicBufferOutputStream? Or is it better to leave it as it is?

        Show
        Mohamed Nufail added a comment - I analysed the usages of InputStreamUtil.java and DynamicByteArrayOutputStream.java in the network client. InputStreamUtil is used only inside a method in DynamicByteArrayOutputStream which is never used. So it can be removed without a problem. But DynamicByteArrayOutputStream has some dynamic resizing abilities, optimized for performance and memory reduction. If PublicBufferOutputStream is used, it will only have ByteArrayOutputStream's resizing capability which just doubles the size of the byte array. So will it be safe to replace DynamicByteArrayOutputStream with PublicBufferOutputStream? Or is it better to leave it as it is?
        Hide
        Knut Anders Hatlen added a comment -

        It looks like DynamicByteArrayOutputStream is optimized for the 4KB page size used in store. Assuming the size of a typical UDT is considerably smaller than that, I think the buffer resizing in ByteArrayOutputStream and PublicBufferOutputStream may actually work better for writeUDT(). With DynamicByteArrayOutputStream, we currently allocate a 4KB buffer no matter how small the value is, which may be wasteful.

        Show
        Knut Anders Hatlen added a comment - It looks like DynamicByteArrayOutputStream is optimized for the 4KB page size used in store. Assuming the size of a typical UDT is considerably smaller than that, I think the buffer resizing in ByteArrayOutputStream and PublicBufferOutputStream may actually work better for writeUDT(). With DynamicByteArrayOutputStream, we currently allocate a 4KB buffer no matter how small the value is, which may be wasteful.
        Hide
        Mohamed Nufail added a comment -

        Ok then. I will replace DynamicByteArrayOutputStream with PublicBufferOutputStream and post the results.

        Show
        Mohamed Nufail added a comment - Ok then. I will replace DynamicByteArrayOutputStream with PublicBufferOutputStream and post the results.
        Hide
        Mohamed Nufail added a comment -

        Attached a patch with the following changes.

        • Remove the two classes InputStreamUtil and DynamicByteArrayOutputStream
        • Move out PublicBufferOutputStream from EncodedInputStream and make it a standalone class
        • Do required changes in Request.java
        Show
        Mohamed Nufail added a comment - Attached a patch with the following changes. Remove the two classes InputStreamUtil and DynamicByteArrayOutputStream Move out PublicBufferOutputStream from EncodedInputStream and make it a standalone class Do required changes in Request.java
        Hide
        Bryan Pendleton added a comment -

        This patch looks very clean to me. My build was clean with the patch applied.
        I also ran 'ant javadoc' and it was clean, though I'm not sure if we build JavaDoc
        for this particular package (org.apache.derby.client.net.*) or not.

        I always like a patch which deletes code! Getting the same thing done with less
        code is always a good sign.

        Here is the 'svn stat' that I have; does this look right?

        A java/client/org/apache/derby/client/net/PublicBufferOutputStream.java
        D java/client/org/apache/derby/client/net/InputStreamUtil.java
        M java/client/org/apache/derby/client/net/EncodedInputStream.java
        D java/client/org/apache/derby/client/net/DynamicByteArrayOutputStream.java
        M java/client/org/apache/derby/client/net/Request.java

        What tests have you run with this patch? I ran

        ant -Dderby.junit.testclass=org.apache.derbyTesting.functionTests.tests.derbynet._Suite junit-single

        and it was clean.

        Are there other tests that we need to run to cover this code? Do the derbynet tests from
        the old harness add additional coverage here?

        I found an old suite named 'derbynetclientmats.runall' and ran it, but I'm not sure if that
        adds any additional coverage in this area or not.

        Show
        Bryan Pendleton added a comment - This patch looks very clean to me. My build was clean with the patch applied. I also ran 'ant javadoc' and it was clean, though I'm not sure if we build JavaDoc for this particular package (org.apache.derby.client.net.*) or not. I always like a patch which deletes code! Getting the same thing done with less code is always a good sign. Here is the 'svn stat' that I have; does this look right? A java/client/org/apache/derby/client/net/PublicBufferOutputStream.java D java/client/org/apache/derby/client/net/InputStreamUtil.java M java/client/org/apache/derby/client/net/EncodedInputStream.java D java/client/org/apache/derby/client/net/DynamicByteArrayOutputStream.java M java/client/org/apache/derby/client/net/Request.java What tests have you run with this patch? I ran ant -Dderby.junit.testclass=org.apache.derbyTesting.functionTests.tests.derbynet._Suite junit-single and it was clean. Are there other tests that we need to run to cover this code? Do the derbynet tests from the old harness add additional coverage here? I found an old suite named 'derbynetclientmats.runall' and ran it, but I'm not sure if that adds any additional coverage in this area or not.
        Hide
        Mohamed Nufail added a comment -

        The 'svn stat' looks correct. Those are the introduced changes.

        And I ran that same test you've mentioned. Did not get any errors.

        I also would like to know if any other tests cover this code.

        Show
        Mohamed Nufail added a comment - The 'svn stat' looks correct. Those are the introduced changes. And I ran that same test you've mentioned. Did not get any errors. I also would like to know if any other tests cover this code.
        Hide
        Knut Anders Hatlen added a comment -

        I would assume lang.UDTTest exercises the writeUDT() method.

        I agree with Bryan that the patch looks very clean and would be a good improvement.

        Show
        Knut Anders Hatlen added a comment - I would assume lang.UDTTest exercises the writeUDT() method. I agree with Bryan that the patch looks very clean and would be a good improvement.
        Hide
        Bryan Pendleton added a comment -

        I'll run some additional tests and plan to commit this patch soon.

        Show
        Bryan Pendleton added a comment - I'll run some additional tests and plan to commit this patch soon.
        Hide
        Bryan Pendleton added a comment -

        My additional testing was uneventful, so I've committed the patch to the
        trunk as revision 1346234.

        Thank you for the contribution to Derby!

        Show
        Bryan Pendleton added a comment - My additional testing was uneventful, so I've committed the patch to the trunk as revision 1346234. Thank you for the contribution to Derby!
        Hide
        Mohamed Nufail added a comment -

        Great. Thank you.

        Show
        Mohamed Nufail added a comment - Great. Thank you.
        Hide
        Bryan Pendleton added a comment -

        In the latest code coverage report on the Sun website (http://dbtg.foundry.sun.com/derby/test/coverage/),
        I see that the code coverage numbers for client.net are confirming that this change
        is working as desired. PublicBufferOutputStream has 100% coverage in the report.

        Nufail, I believe you can mark this job as "closed".

        Show
        Bryan Pendleton added a comment - In the latest code coverage report on the Sun website ( http://dbtg.foundry.sun.com/derby/test/coverage/ ), I see that the code coverage numbers for client.net are confirming that this change is working as desired. PublicBufferOutputStream has 100% coverage in the report. Nufail, I believe you can mark this job as "closed".
        Hide
        Mohamed Nufail added a comment -

        Thank you. I've closed the issue.

        Show
        Mohamed Nufail added a comment - Thank you. I've closed the issue.

          People

          • Assignee:
            Mohamed Nufail
            Reporter:
            Mohamed Nufail
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development