Derby
  1. Derby
  2. DERBY-2920

Share code between readExternal() and readExternalFromArray()

    Details

    • Urgency:
      Normal

      Description

      Most of the implementations of DataValueDescriptor.readExternalFromArray(ArrayInputStream) are identical to their corresponding Externalizable.readExternal(ObjectInput) methods. Since ArrayInputStream implements ObjectInput, readExternalFromArray() could in those cases just have forwarded calls to readExternal() instead of duplicating the code. A default forwarding implementation of readExternalFromArray() could be placed in org.apache.derby.iapi.types.DataType, and all the existing implementations, except those with optimizations for ArrayInputStream, could be removed.

      1. d2920-1a.diff
        26 kB
        Knut Anders Hatlen
      2. ReadInts.java
        3 kB
        Knut Anders Hatlen
      3. ReadInts2.java
        3 kB
        Knut Anders Hatlen
      4. d2920-2a-unused.diff
        6 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          There are 23 implementations of readExternalFromArray(). 19 of them are identical to the readExternal() method in the same class.

          The attached patch adds a readExternalFromArray() method to the DataType class, which is a common base class for all the classes that implement readExternalFromArray(). The new method simply forwards calls to readExternal().

          The patch also removes the 19 readExternalFromArray() methods that are identical to readExternal(), as they would work correctly by inheriting the method from DataType. The four classes that have optimized implementations that are not identical to readExternal(), keep their current implementations.

          The four classes that have optimized versions of readExternalFromArray(), are SQLChar, SQLClob, XML and HeapRowLocation.

          All the regression tests passed with the patch.

          Show
          Knut Anders Hatlen added a comment - There are 23 implementations of readExternalFromArray(). 19 of them are identical to the readExternal() method in the same class. The attached patch adds a readExternalFromArray() method to the DataType class, which is a common base class for all the classes that implement readExternalFromArray(). The new method simply forwards calls to readExternal(). The patch also removes the 19 readExternalFromArray() methods that are identical to readExternal(), as they would work correctly by inheriting the method from DataType. The four classes that have optimized implementations that are not identical to readExternal(), keep their current implementations. The four classes that have optimized versions of readExternalFromArray(), are SQLChar, SQLClob, XML and HeapRowLocation. All the regression tests passed with the patch.
          Hide
          Mike Matrigali added a comment -

          I worry about adding extra method calls in this area, but admit it has been a long time and many jvm versions ago since I looked
          at performance resulst in this area. The issue here is that this is a very high traffic routine.
          For instance I think the worst case would be the case of a 10 collumn row and looking at 1 million rows this change may add
          1 million extra function calls. Maybe JIT fixes, not sure.

          I also wonder if we lose performance by moving the routines out of final classes.

          I know some code duplication was added in the area of read and write external in the past as the result of performance tuning.

          Show
          Mike Matrigali added a comment - I worry about adding extra method calls in this area, but admit it has been a long time and many jvm versions ago since I looked at performance resulst in this area. The issue here is that this is a very high traffic routine. For instance I think the worst case would be the case of a 10 collumn row and looking at 1 million rows this change may add 1 million extra function calls. Maybe JIT fixes, not sure. I also wonder if we lose performance by moving the routines out of final classes. I know some code duplication was added in the area of read and write external in the past as the result of performance tuning.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Mike. I was wondering about that myself, so I had started a test run with the attached ReadInts test class. The test is similar to what you suggested, but the table was smaller (3 integer columns, 10000 rows).

          After 100 runs with trunk and 100 runs with the patch (each run performs a full table scan 20000 times for warmup and 10000 times while collecting data), I found that on average there was a 0.02% difference. That is, no measurable difference. This was with JDK 7u9 on Solaris 11.

          I'll change the size of the test table to 10x1000000 and see if that changes anything.

          Show
          Knut Anders Hatlen added a comment - Thanks, Mike. I was wondering about that myself, so I had started a test run with the attached ReadInts test class. The test is similar to what you suggested, but the table was smaller (3 integer columns, 10000 rows). After 100 runs with trunk and 100 runs with the patch (each run performs a full table scan 20000 times for warmup and 10000 times while collecting data), I found that on average there was a 0.02% difference. That is, no measurable difference. This was with JDK 7u9 on Solaris 11. I'll change the size of the test table to 10x1000000 and see if that changes anything.
          Hide
          Knut Anders Hatlen added a comment -

          The attached ReadInts2 class is the updated test which scans a table with 10x1000000 integers. I ran it in the same environment as the previous test (Solaris 11, x86, JDK 7u9, 64-bit server VM). This time I had it running overnight so that I got 300 runs with a clean trunk and 300 runs with the patch. On average, the test ran 0.12% slower with the patch, so there was virtually no difference.

          Show
          Knut Anders Hatlen added a comment - The attached ReadInts2 class is the updated test which scans a table with 10x1000000 integers. I ran it in the same environment as the previous test (Solaris 11, x86, JDK 7u9, 64-bit server VM). This time I had it running overnight so that I got 300 runs with a clean trunk and 300 runs with the patch. On average, the test ran 0.12% slower with the patch, so there was virtually no difference.
          Hide
          Knut Anders Hatlen added a comment -

          I also ran a modified variant of ReadInts2 where half the columns were integers and the other half boolean, in case the run-time compiler was able to optimize the calls in the original test more aggressively because it detected that all calls to DataType.readExternalFromArray() would end up calling SQLInteger.readExternal(). That experiment also resulted in ~0.1% difference. I think this means the code duplication doesn't provide any significant benefit on modern JVMs, so I committed the patch with revision 1407432. If we find cases where this change causes measurable performance loss, we can add overrides for the affected data types.

          Show
          Knut Anders Hatlen added a comment - I also ran a modified variant of ReadInts2 where half the columns were integers and the other half boolean, in case the run-time compiler was able to optimize the calls in the original test more aggressively because it detected that all calls to DataType.readExternalFromArray() would end up calling SQLInteger.readExternal(). That experiment also resulted in ~0.1% difference. I think this means the code duplication doesn't provide any significant benefit on modern JVMs, so I committed the patch with revision 1407432. If we find cases where this change causes measurable performance loss, we can add overrides for the affected data types.
          Hide
          Mike Matrigali added a comment -

          thanks for running the tests, i think it is ok to check this in. I set my low limit at around .3-.4 % for a code change like this where it was a share/codesize vs performance issue. and higher if the change was more complicated rather than just hand inlining code, ...

          I remember now that these routines would also show much higher for some profilers vs deliverable performance runs. So the change may make it a little harder to determine where you can
          get real performance wins. Again this is old profiler, especially those that had to instrument each routine to get their results. Again I think it ok to check in, just noting this in case it can help
          anyone in the future if they see readExternal...() showing up high in profiles.

          Show
          Mike Matrigali added a comment - thanks for running the tests, i think it is ok to check this in. I set my low limit at around .3-.4 % for a code change like this where it was a share/codesize vs performance issue. and higher if the change was more complicated rather than just hand inlining code, ... I remember now that these routines would also show much higher for some profilers vs deliverable performance runs. So the change may make it a little harder to determine where you can get real performance wins. Again this is old profiler, especially those that had to instrument each routine to get their results. Again I think it ok to check in, just noting this in case it can help anyone in the future if they see readExternal...() showing up high in profiles.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks Mike. It makes sense that such code may have skewed the results of profilers in the past, especially instrumenting profilers that maintain counters on method boundaries. I would expect that effect to be smaller on a sampling profiler, and hopefully also on newer instrumenting profilers, so that it doesn't cause much confusion.

          Show
          Knut Anders Hatlen added a comment - Thanks Mike. It makes sense that such code may have skewed the results of profilers in the past, especially instrumenting profilers that maintain counters on method boundaries. I would expect that effect to be smaller on a sampling profiler, and hopefully also on newer instrumenting profilers, so that it doesn't cause much confusion.
          Hide
          Knut Anders Hatlen added a comment -

          Reopening since I found more duplication that I didn't address in the first patch. The classes FormatArrayHolder, FormatableHashtable, FormatableIntHolder, FormatableLongHolder, FormatableProperties and B2IUndo all have two readExternal() methods; one that takes ObjectInput and one that takes ArrayInputStream.

          In these classes, the variants that take ArrayInputStream are not called readExternalFromArray(), which is why they didn't show up in my original search. However, as far as I can see, none of the readExternal(ArrayInputStream) methods have any callers, so removing them should be harmless.

          Show
          Knut Anders Hatlen added a comment - Reopening since I found more duplication that I didn't address in the first patch. The classes FormatArrayHolder, FormatableHashtable, FormatableIntHolder, FormatableLongHolder, FormatableProperties and B2IUndo all have two readExternal() methods; one that takes ObjectInput and one that takes ArrayInputStream. In these classes, the variants that take ArrayInputStream are not called readExternalFromArray(), which is why they didn't show up in my original search. However, as far as I can see, none of the readExternal(ArrayInputStream) methods have any callers, so removing them should be harmless.
          Hide
          Knut Anders Hatlen added a comment -

          Attaching d2920-2a-unused.diff, which removes the unused readExternal() methods. All the regression tests ran cleanly with the patch.

          Show
          Knut Anders Hatlen added a comment - Attaching d2920-2a-unused.diff, which removes the unused readExternal() methods. All the regression tests ran cleanly with the patch.
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 1411164.

          Show
          Knut Anders Hatlen added a comment - Committed revision 1411164.

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Knut Anders Hatlen
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development