Derby
  1. Derby
  2. DERBY-5210

Use java.nio.ByteBuffer in client.net.Request

    Details

      Description

      We should see if we could use a java.nio.ByteBuffer instead of a byte array in org.apache.derby.client.net.Request, similar to what we did for DDMWriter in DERBY-2936. ByteBuffer provides some helper methods that allows us to simplify the code that puts multi-byte values into the buffer (like ByteBuffer.putShort(), putInt(), putLong()). It may also be a first step on the way to using a CharsetEncoder to encode strings without going via an intermediate throw-away byte array (see DERBY-5068 for details).

      1. d5210-1a.diff
        52 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          Attaching a patch that removes the bytes_ and offset_ fields from the Request class and replaces them with a java.nio.ByteBuffer field. The Request class and its subclasses were changed to use the new field. Apart from the mechanical changes (that is, replacing the direct byte array accesses with accesses via ByteBuffer), the patch changes the following:

          1) Changed the meaning of the length parameter in the ensureLength() method. Previously, it represented the minimum total length of the byte buffer, whereas it now represents the minimum number of available bytes at the end of the buffer. (Actually, the existing comments in the method match the new behaviour, so it looks like that was how it was intended to work.) This seemed like a reasonable simplification now that ensureLength() needed to be rewritten in any case (because it needed to check and reallocate a ByteBuffer instead of a byte array) and also all calls needed to be changed (because almost all referred to the removed offset_ field).

          2) As I updated various pieces of the code, I noticed that there was a lot of duplication. The places where I had seen a helper method that did the exact same thing, I changed the code to use the helper methods instead of just mechanically translating it.

          3) Removed some helper methods in client.am.SignedBinary and client.am.FloatingPoint since we now use methods in java.nio.ByteBuffer that provide the same functionality.

          The patch adds 199 lines and removes 414 lines, so it results in a net removal of 215 lines.

          All the regression tests ran cleanly with the patch. I'm currently running some performance tests to see if these changes affect the performance negatively.

          Show
          Knut Anders Hatlen added a comment - Attaching a patch that removes the bytes_ and offset_ fields from the Request class and replaces them with a java.nio.ByteBuffer field. The Request class and its subclasses were changed to use the new field. Apart from the mechanical changes (that is, replacing the direct byte array accesses with accesses via ByteBuffer), the patch changes the following: 1) Changed the meaning of the length parameter in the ensureLength() method. Previously, it represented the minimum total length of the byte buffer, whereas it now represents the minimum number of available bytes at the end of the buffer. (Actually, the existing comments in the method match the new behaviour, so it looks like that was how it was intended to work.) This seemed like a reasonable simplification now that ensureLength() needed to be rewritten in any case (because it needed to check and reallocate a ByteBuffer instead of a byte array) and also all calls needed to be changed (because almost all referred to the removed offset_ field). 2) As I updated various pieces of the code, I noticed that there was a lot of duplication. The places where I had seen a helper method that did the exact same thing, I changed the code to use the helper methods instead of just mechanically translating it. 3) Removed some helper methods in client.am.SignedBinary and client.am.FloatingPoint since we now use methods in java.nio.ByteBuffer that provide the same functionality. The patch adds 199 lines and removes 414 lines, so it results in a net removal of 215 lines. All the regression tests ran cleanly with the patch. I'm currently running some performance tests to see if these changes affect the performance negatively.
          Hide
          Knut Anders Hatlen added a comment -

          I ran the sr_select performance test client with 10 threads to see if the patch affected the performance of client/server communication (tested derbyclient.jar from 10.8.1.2 against trunk patched with the 1a patch). Taking the average of 100 runs with each version, I could not see any difference at all in the throughput with this kind of load. The number of CPU cycles per transaction spent in the client driver seems to have increased by 1% because of the patch. I hope, though, when we add the improvements suggested in DERBY-5068, that we will end up reducing the number of CPU cycles spent in the client driver. I'll do some experimenting with the suggested DERBY-5068 changes in combination with this patch before I go ahead with committing it.

          Show
          Knut Anders Hatlen added a comment - I ran the sr_select performance test client with 10 threads to see if the patch affected the performance of client/server communication (tested derbyclient.jar from 10.8.1.2 against trunk patched with the 1a patch). Taking the average of 100 runs with each version, I could not see any difference at all in the throughput with this kind of load. The number of CPU cycles per transaction spent in the client driver seems to have increased by 1% because of the patch. I hope, though, when we add the improvements suggested in DERBY-5068 , that we will end up reducing the number of CPU cycles spent in the client driver. I'll do some experimenting with the suggested DERBY-5068 changes in combination with this patch before I go ahead with committing it.
          Hide
          Knut Anders Hatlen added a comment -

          I still intend to do some more testing, but I think the patch is ready for review, so I'm setting the patch available flag.

          Show
          Knut Anders Hatlen added a comment - I still intend to do some more testing, but I think the patch is ready for review, so I'm setting the patch available flag.
          Hide
          Knut Anders Hatlen added a comment -

          I changed Utf8CcsidManager to use a CharsetEncoder to encode strings directly into the ByteBuffer and reran the experiment (will post a patch on DERBY-5068 soon). With these changes, I saw reduced CPU usage compared to 10.8.1.2 (still no observable change in throughput, though). So I think it's safe to assume that although the 1a patch seen in isolation makes the client driver use slightly more CPU, it enables us to add optimizations that outweigh the extra cost. And, regardless of that, the extra cost seems to be so small that it might be worthwhile to check in the patch in any case, just because it makes the code simpler.

          Show
          Knut Anders Hatlen added a comment - I changed Utf8CcsidManager to use a CharsetEncoder to encode strings directly into the ByteBuffer and reran the experiment (will post a patch on DERBY-5068 soon). With these changes, I saw reduced CPU usage compared to 10.8.1.2 (still no observable change in throughput, though). So I think it's safe to assume that although the 1a patch seen in isolation makes the client driver use slightly more CPU, it enables us to add optimizations that outweigh the extra cost. And, regardless of that, the extra cost seems to be so small that it might be worthwhile to check in the patch in any case, just because it makes the code simpler.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for this cleanup! Nice to see so much bit-tweaking go away. In spite of the slight CPU increase of this patch in isolation, you say that with changes planned in DERBY-5068 which reply on this change if I understand correctly, CPU usage will go down again, so on that basis also, I am positive to this change: +1

          Show
          Dag H. Wanvik added a comment - Thanks for this cleanup! Nice to see so much bit-tweaking go away. In spite of the slight CPU increase of this patch in isolation, you say that with changes planned in DERBY-5068 which reply on this change if I understand correctly, CPU usage will go down again, so on that basis also, I am positive to this change: +1
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for looking at the patch, Dag. We can probably wait with committing this patch until we've decided which approach to go for in DERBY-5068. Then it will be clearer what the performance implications are.

          Show
          Knut Anders Hatlen added a comment - Thanks for looking at the patch, Dag. We can probably wait with committing this patch until we've decided which approach to go for in DERBY-5068 . Then it will be clearer what the performance implications are.
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 1102730.

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development