HBase
  1. HBase
  2. HBASE-5945

Reduce buffer copies in IPC server response path

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.95.2
    • Fix Version/s: 0.98.0, 0.96.1
    • Component/s: IPC/RPC
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The new PB code is sloppy with buffers and makes several needless copies. This increases GC time a lot. A few simple changes can cut this back down.

      1. 5945v5.txt
        45 kB
        stack
      2. 5945v4.txt
        27 kB
        stack
      3. with_patch.png
        386 kB
        stack
      4. without_patch.png
        384 kB
        stack
      5. 5945v4.txt
        27 kB
        stack
      6. 5945v2.txt
        23 kB
        stack
      7. 5945-in-progress.2.1.patch
        136 kB
        Devaraj Das
      8. 5945-in-progress.2.patch
        144 kB
        Devaraj Das
      9. 5945-in-progress.patch
        101 kB
        Devaraj Das
      10. hbase-5495.txt
        24 kB
        Todd Lipcon
      11. even-fewer-copies.txt
        17 kB
        Todd Lipcon
      12. buffer-copies.txt
        2 kB
        Todd Lipcon

        Issue Links

          Activity

          Hide
          stack added a comment -

          Released in 0.96.1. Issue closed.

          Show
          stack added a comment - Released in 0.96.1. Issue closed.
          Hide
          stack added a comment -

          Committed gathering patch. Further cleanup to be done in other issues (e.g. the issue linked to this one).

          Show
          stack added a comment - Committed gathering patch. Further cleanup to be done in other issues (e.g. the issue linked to this one).
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in hbase-0.96-hadoop2 #125 (See https://builds.apache.org/job/hbase-0.96-hadoop2/125/)
          HBASE-5945 Reduce buffer copies in IPC server response path (stack: rev 1543463)

          • /hbase/branches/0.96/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java
          • /hbase/branches/0.96/hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
          • /hbase/branches/0.96/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java
          • /hbase/branches/0.96/hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java
          • /hbase/branches/0.96/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java
          • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java
          • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServer.java
          • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
          • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java
          • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestBufferChain.java
          • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestIPC.java
          Show
          Hudson added a comment - SUCCESS: Integrated in hbase-0.96-hadoop2 #125 (See https://builds.apache.org/job/hbase-0.96-hadoop2/125/ ) HBASE-5945 Reduce buffer copies in IPC server response path (stack: rev 1543463) /hbase/branches/0.96/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java /hbase/branches/0.96/hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/branches/0.96/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java /hbase/branches/0.96/hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java /hbase/branches/0.96/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServer.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestBufferChain.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestIPC.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in HBase-TRUNK #4688 (See https://builds.apache.org/job/HBase-TRUNK/4688/)
          HBASE-5945 Reduce buffer copies in IPC server response path (stack: rev 1543458)

          • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java
          • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
          • /hbase/trunk/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java
          • /hbase/trunk/hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java
          • /hbase/trunk/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServer.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestBufferChain.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestIPC.java
          Show
          Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #4688 (See https://builds.apache.org/job/HBase-TRUNK/4688/ ) HBASE-5945 Reduce buffer copies in IPC server response path (stack: rev 1543458) /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/trunk/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java /hbase/trunk/hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java /hbase/trunk/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestBufferChain.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestIPC.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in hbase-0.96 #196 (See https://builds.apache.org/job/hbase-0.96/196/)
          HBASE-5945 Reduce buffer copies in IPC server response path (stack: rev 1543463)

          • /hbase/branches/0.96/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java
          • /hbase/branches/0.96/hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
          • /hbase/branches/0.96/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java
          • /hbase/branches/0.96/hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java
          • /hbase/branches/0.96/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java
          • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java
          • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServer.java
          • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
          • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java
          • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestBufferChain.java
          • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestIPC.java
          Show
          Hudson added a comment - SUCCESS: Integrated in hbase-0.96 #196 (See https://builds.apache.org/job/hbase-0.96/196/ ) HBASE-5945 Reduce buffer copies in IPC server response path (stack: rev 1543463) /hbase/branches/0.96/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java /hbase/branches/0.96/hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/branches/0.96/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java /hbase/branches/0.96/hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java /hbase/branches/0.96/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServer.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestBufferChain.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestIPC.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #844 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/844/)
          HBASE-5945 Reduce buffer copies in IPC server response path (stack: rev 1543458)

          • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java
          • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
          • /hbase/trunk/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java
          • /hbase/trunk/hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java
          • /hbase/trunk/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServer.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestBufferChain.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestIPC.java
          Show
          Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #844 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/844/ ) HBASE-5945 Reduce buffer copies in IPC server response path (stack: rev 1543458) /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/trunk/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java /hbase/trunk/hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java /hbase/trunk/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSourceImpl.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestBufferChain.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestIPC.java
          Hide
          stack added a comment -

          I will commit this later today unless objection.

          Show
          stack added a comment - I will commit this later today unless objection.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12614339/5945v5.txt
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          -1 javadoc. The javadoc tool appears to have generated 10 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch does not introduce lines longer than 100

          -1 site. The patch appears to cause mvn site goal to fail.

          +1 core tests. The patch passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12614339/5945v5.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 11 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 10 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7909//console This message is automatically generated.
          Hide
          stack added a comment -

          Fix the zombie created (was making a cell block stream though no cells in it)

          Show
          stack added a comment - Fix the zombie created (was making a cell block stream though no cells in it)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12614169/5945v4.txt
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          -1 javadoc. The javadoc tool appears to have generated 9 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch does not introduce lines longer than 100

          -1 site. The patch appears to cause mvn site goal to fail.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.coprocessor.TestRegionObserverInterface
          org.apache.hadoop.hbase.util.TestHBaseFsck
          org.apache.hadoop.hbase.replication.TestMultiSlaveReplication

          -1 core zombie tests. There are 11 zombie test(s): at org.apache.hadoop.hbase.coprocessor.TestHTableWrapper.testHTableInterfaceMethods(TestHTableWrapper.java:139)
          at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication3(TestMasterReplication.java:243)
          at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication2(TestMasterReplication.java:206)
          at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication1(TestMasterReplication.java:150)
          at org.apache.hadoop.hbase.master.TestDistributedLogSplitting.testReplayCmd(TestDistributedLogSplitting.java:704)
          at org.apache.hadoop.hbase.client.TestFromClientSide3.testHTableExistsMethodMultipleRegionsMultipleGets(TestFromClientSide3.java:348)

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12614169/5945v4.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 5 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 9 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.coprocessor.TestRegionObserverInterface org.apache.hadoop.hbase.util.TestHBaseFsck org.apache.hadoop.hbase.replication.TestMultiSlaveReplication -1 core zombie tests . There are 11 zombie test(s): at org.apache.hadoop.hbase.coprocessor.TestHTableWrapper.testHTableInterfaceMethods(TestHTableWrapper.java:139) at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication3(TestMasterReplication.java:243) at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication2(TestMasterReplication.java:206) at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication1(TestMasterReplication.java:150) at org.apache.hadoop.hbase.master.TestDistributedLogSplitting.testReplayCmd(TestDistributedLogSplitting.java:704) at org.apache.hadoop.hbase.client.TestFromClientSide3.testHTableExistsMethodMultipleRegionsMultipleGets(TestFromClientSide3.java:348) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7891//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12614164/5945v4.txt
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          -1 javadoc. The javadoc tool appears to have generated 9 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch does not introduce lines longer than 100

          -1 site. The patch appears to cause mvn site goal to fail.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.replication.TestMultiSlaveReplication
          org.apache.hadoop.hbase.coprocessor.TestRegionObserverInterface

          -1 core zombie tests. There are 12 zombie test(s): at org.apache.hadoop.hbase.coprocessor.TestHTableWrapper.testHTableInterfaceMethods(TestHTableWrapper.java:139)
          at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication3(TestMasterReplication.java:243)
          at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication2(TestMasterReplication.java:206)
          at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication1(TestMasterReplication.java:150)
          at org.apache.hadoop.hbase.master.TestDistributedLogSplitting.testReplayCmd(TestDistributedLogSplitting.java:704)
          at org.apache.hadoop.hbase.client.TestFromClientSide3.testHTableExistsMethodMultipleRegionsMultipleGets(TestFromClientSide3.java:348)
          at org.apache.hadoop.hbase.io.encoding.TestEncodedSeekers.testEncodedSeeker(TestEncodedSeekers.java:124)

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12614164/5945v4.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 5 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 9 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.replication.TestMultiSlaveReplication org.apache.hadoop.hbase.coprocessor.TestRegionObserverInterface -1 core zombie tests . There are 12 zombie test(s): at org.apache.hadoop.hbase.coprocessor.TestHTableWrapper.testHTableInterfaceMethods(TestHTableWrapper.java:139) at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication3(TestMasterReplication.java:243) at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication2(TestMasterReplication.java:206) at org.apache.hadoop.hbase.replication.TestMasterReplication.testCyclicReplication1(TestMasterReplication.java:150) at org.apache.hadoop.hbase.master.TestDistributedLogSplitting.testReplayCmd(TestDistributedLogSplitting.java:704) at org.apache.hadoop.hbase.client.TestFromClientSide3.testHTableExistsMethodMultipleRegionsMultipleGets(TestFromClientSide3.java:348) at org.apache.hadoop.hbase.io.encoding.TestEncodedSeekers.testEncodedSeeker(TestEncodedSeekers.java:124) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7890//console This message is automatically generated.
          Hide
          stack added a comment -

          Devaraj Das Just saw your comment. Thanks for review. Rough numbers attached. If hadoopqa passes I'll commit. There is more to be had here I'd say but will do in new issue – then go read-side. Different approach on read-side I'd say — look at keeping the request off-heap as long as possible.. we'll see.

          Show
          stack added a comment - Devaraj Das Just saw your comment. Thanks for review. Rough numbers attached. If hadoopqa passes I'll commit. There is more to be had here I'd say but will do in new issue – then go read-side. Different approach on read-side I'd say — look at keeping the request off-heap as long as possible.. we'll see.
          Hide
          stack added a comment -

          Upload patch again so hadoopqa picks up this.

          Show
          stack added a comment - Upload patch again so hadoopqa picks up this.
          Hide
          stack added a comment -

          Here are results running little benchmark on end of TestIPC in its main.
          It sets up the rpc doing a little echo protocol. The echo copies the cells
          it receives onto the response. On cmdline say how many cycles and how many columns. I made Cell size be about 10k and ran the test adding 10 Cells per iteration so we are sending back and forth about 100k. This approximates a small to medium-sized mult call. I cycled 10k times. Below are the test done twice.

          With patch, the test finishes a little sooner... about 5-10% sooner.

          I ran visualvm over a minute+ against each at about same stage in test.
          Without patch we use more CPU and do more GC – just over 36% CPU vs 33% or so and we do a bit more GC'ing... 4.1% or so vs 3.4% or so. W/o the patch, more heap is used. See pictures. Patch seems to be improvement.

          WITHOUT PATCH

          durruti:hbase.git stack$ for i in 1 2 3 4 5; do time ./bin/hbase -Dhbase.defaults.for.version.skip=true org.apache.hadoop.hbase.ipc.TestIPC 100000 10 &> /tmp/wopatch.$i.txt; done

          real 0m42.843s
          user 0m43.902s
          sys 0m17.495s

          real 0m43.357s
          user 0m46.050s
          sys 0m17.712s

          real 0m42.595s
          user 0m44.179s
          sys 0m17.448s

          real 0m43.320s
          user 0m45.578s
          sys 0m17.736s

          real 0m42.647s
          user 0m44.845s
          sys 0m17.583s

          ... and again

          real 0m45.868s
          user 0m46.522s
          sys 0m18.776s

          real 0m42.764s
          user 0m44.505s
          sys 0m17.447s

          real 0m43.080s
          user 0m45.445s
          sys 0m17.585s

          real 0m43.261s
          user 0m45.246s
          sys 0m17.722s

          real 0m42.592s
          user 0m44.102s
          sys 0m17.333s

          WITH PATCH

          durruti:hbase.git stack$ for i in 1 2 3 4 5; do time ./bin/hbase -Dhbase.defaults.for.version.skip=true org.apache.hadoop.hbase.ipc.TestIPC 100000 10 &> /tmp/wpatch.$i.txt; done

          real 0m38.838s
          user 0m40.415s
          sys 0m18.765s

          real 0m37.638s
          user 0m39.246s
          sys 0m18.408s

          real 0m38.696s
          user 0m40.169s
          sys 0m18.700s

          real 0m37.948s
          user 0m39.403s
          sys 0m18.682s

          real 0m38.077s
          user 0m39.519s
          sys 0m18.571s

          ...and again.

          real 0m43.888s
          user 0m44.394s
          sys 0m21.427s

          real 0m40.311s
          user 0m42.553s
          sys 0m19.460s

          real 0m38.489s
          user 0m41.097s
          sys 0m18.761s

          real 0m38.252s
          user 0m39.603s
          sys 0m18.618s

          real 0m38.066s
          user 0m39.656s
          sys 0m18.621s

          Show
          stack added a comment - Here are results running little benchmark on end of TestIPC in its main. It sets up the rpc doing a little echo protocol. The echo copies the cells it receives onto the response. On cmdline say how many cycles and how many columns. I made Cell size be about 10k and ran the test adding 10 Cells per iteration so we are sending back and forth about 100k. This approximates a small to medium-sized mult call. I cycled 10k times. Below are the test done twice. With patch, the test finishes a little sooner... about 5-10% sooner. I ran visualvm over a minute+ against each at about same stage in test. Without patch we use more CPU and do more GC – just over 36% CPU vs 33% or so and we do a bit more GC'ing... 4.1% or so vs 3.4% or so. W/o the patch, more heap is used. See pictures. Patch seems to be improvement. WITHOUT PATCH durruti:hbase.git stack$ for i in 1 2 3 4 5; do time ./bin/hbase -Dhbase.defaults.for.version.skip=true org.apache.hadoop.hbase.ipc.TestIPC 100000 10 &> /tmp/wopatch.$i.txt; done real 0m42.843s user 0m43.902s sys 0m17.495s real 0m43.357s user 0m46.050s sys 0m17.712s real 0m42.595s user 0m44.179s sys 0m17.448s real 0m43.320s user 0m45.578s sys 0m17.736s real 0m42.647s user 0m44.845s sys 0m17.583s ... and again real 0m45.868s user 0m46.522s sys 0m18.776s real 0m42.764s user 0m44.505s sys 0m17.447s real 0m43.080s user 0m45.445s sys 0m17.585s real 0m43.261s user 0m45.246s sys 0m17.722s real 0m42.592s user 0m44.102s sys 0m17.333s WITH PATCH durruti:hbase.git stack$ for i in 1 2 3 4 5; do time ./bin/hbase -Dhbase.defaults.for.version.skip=true org.apache.hadoop.hbase.ipc.TestIPC 100000 10 &> /tmp/wpatch.$i.txt; done real 0m38.838s user 0m40.415s sys 0m18.765s real 0m37.638s user 0m39.246s sys 0m18.408s real 0m38.696s user 0m40.169s sys 0m18.700s real 0m37.948s user 0m39.403s sys 0m18.682s real 0m38.077s user 0m39.519s sys 0m18.571s ...and again. real 0m43.888s user 0m44.394s sys 0m21.427s real 0m40.311s user 0m42.553s sys 0m19.460s real 0m38.489s user 0m41.097s sys 0m18.761s real 0m38.252s user 0m39.603s sys 0m18.618s real 0m38.066s user 0m39.656s sys 0m18.621s
          Hide
          stack added a comment -

          Fixes the h2 build failure (forgot to change the h2 compat module for the metrics change). Does a little fix up on the end of TestIPC so I can do a bit of a benchmark on this patch.

          Show
          stack added a comment - Fixes the h2 build failure (forgot to change the h2 compat module for the metrics change). Does a little fix up on the end of TestIPC so I can do a bit of a benchmark on this patch.
          Hide
          Devaraj Das added a comment -

          Skimmed it. Looks okay to me. Any chance of getting perf numbers. If not, fine as well. Also, just wondering would it be possible to apply the pattern on the read side as well?

          Show
          Devaraj Das added a comment - Skimmed it. Looks okay to me. Any chance of getting perf numbers. If not, fine as well. Also, just wondering would it be possible to apply the pattern on the read side as well?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12613551/5945v2.txt
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          -1 hadoop2.0. The patch failed to compile against the hadoop 2.0 profile.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7862//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12613551/5945v2.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 6 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. -1 hadoop2.0 . The patch failed to compile against the hadoop 2.0 profile. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7862//console This message is automatically generated.
          Hide
          Nicolas Liochon added a comment -

          +1 on the idea. I think we need to push byte buffers all over the place to save on all these copies.

          Show
          Nicolas Liochon added a comment - +1 on the idea. I think we need to push byte buffers all over the place to save on all these copies.
          Hide
          stack added a comment -

          Stuff is very different now. The attached patch gets the big idea. The other fixups in hbase-5495.txt no longer apply. There is no more a HbaseObjectWritable nor Invocation. The hand-making of pbs I did not do but picked up the creation of CodedOutputStream using byte array to avoid it internally assigning buffer. The RPCServer changes make sense still a little complicated by sasl wrapping but we still get a win I believe.

          Will try and do a bit of benchmarking...

          Show
          stack added a comment - Stuff is very different now. The attached patch gets the big idea. The other fixups in hbase-5495.txt no longer apply. There is no more a HbaseObjectWritable nor Invocation. The hand-making of pbs I did not do but picked up the creation of CodedOutputStream using byte array to avoid it internally assigning buffer. The RPCServer changes make sense still a little complicated by sasl wrapping but we still get a win I believe. Will try and do a bit of benchmarking...
          Hide
          stack added a comment -

          Forward port Todd's old idea of saving copies by passing array of ByteBuffers to Channel rather than compose response in a buffer first before putting it on the wire. Revives his BufferChain doohickey.

          Here's some comment on the changes.

          M hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java

          Added getDelimitedMessageAsByteBuffer(final Message m)
          Removed no longer used write method and getDelimitedMessageBytes.

          M hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java

          Changed bytes sent size from int to long (it is long in implementation, just not in interfaces)

          A hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java

          Class to carry an array of ByteBuffers and that can write the byte buffers in order
          out to a GatheringByteChannel in chunks.

          M hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java

          Refactored response sending to use BufferChain. We save on a composition of total
          response in memory. Instead we now pass the parts directly to the socket channel.

          Stuff is a bit ugly in the sasl handling because have to compose the message
          in memory completely so can do the sasl wrapping; this undoes any savings but
          I believe we are still ahead because we removed the serializing of the resulting
          sasl array to an output stream backed by a new buffer.

          Show
          stack added a comment - Forward port Todd's old idea of saving copies by passing array of ByteBuffers to Channel rather than compose response in a buffer first before putting it on the wire. Revives his BufferChain doohickey. Here's some comment on the changes. M hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java Added getDelimitedMessageAsByteBuffer(final Message m) Removed no longer used write method and getDelimitedMessageBytes. M hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerSource.java Changed bytes sent size from int to long (it is long in implementation, just not in interfaces) A hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java Class to carry an array of ByteBuffers and that can write the byte buffers in order out to a GatheringByteChannel in chunks. M hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java Refactored response sending to use BufferChain. We save on a composition of total response in memory. Instead we now pass the parts directly to the socket channel. Stuff is a bit ugly in the sasl handling because have to compose the message in memory completely so can do the sasl wrapping; this undoes any savings but I believe we are still ahead because we removed the serializing of the resulting sasl array to an output stream backed by a new buffer.
          Hide
          Devaraj Das added a comment -

          This patch is really stale. Canceling.

          Show
          Devaraj Das added a comment - This patch is really stale. Canceling.
          Hide
          Devaraj Das added a comment -

          Downgrading priority since I don't think this is a relevant issue anymore. Please discuss if you feel otherwise.

          Show
          Devaraj Das added a comment - Downgrading priority since I don't think this is a relevant issue anymore. Please discuss if you feel otherwise.
          Hide
          Devaraj Das added a comment -

          Looked at this some. HBASE-7905 covered the improvements that have been discussed here. Maybe we can close this issue now..

          Show
          Devaraj Das added a comment - Looked at this some. HBASE-7905 covered the improvements that have been discussed here. Maybe we can close this issue now..
          Hide
          Devaraj Das added a comment -

          Okay, stack, this is fine. We can revisit this issue after HBASE-7533.

          Show
          Devaraj Das added a comment - Okay, stack , this is fine. We can revisit this issue after HBASE-7533 .
          Hide
          stack added a comment -

          ....are you saying that we preserve most of the current RPC header/body structure, and commit the patch here with a focus on removing buffer copies?

          No. If we do that, we have to put rpc back together twice because it will have changed radically twice. We will also reduce buffer copies that we may not be of use in the final rpc incarnation. I was thinking we do the spec first over in 7533. This issue would be about saving buffer copies after we put rpc back together after 7533.

          Show
          stack added a comment - ....are you saying that we preserve most of the current RPC header/body structure, and commit the patch here with a focus on removing buffer copies? No. If we do that, we have to put rpc back together twice because it will have changed radically twice. We will also reduce buffer copies that we may not be of use in the final rpc incarnation. I was thinking we do the spec first over in 7533. This issue would be about saving buffer copies after we put rpc back together after 7533.
          Hide
          Devaraj Das added a comment -

          stack, just trying to understand - are you saying that we preserve most of the current RPC header/body structure, and commit the patch here with a focus on removing buffer copies? (the RPC.proto in the last patch here has the description of the RPC header/body). Then follow it up on 7533 with the spec'ed out RPC header/body and a separate patch ?

          Show
          Devaraj Das added a comment - stack , just trying to understand - are you saying that we preserve most of the current RPC header/body structure, and commit the patch here with a focus on removing buffer copies? (the RPC.proto in the last patch here has the description of the RPC header/body). Then follow it up on 7533 with the spec'ed out RPC header/body and a separate patch ?
          Hide
          stack added a comment -

          Devaraj Das I'd say let this issue be about reducing buffer copies. hbase-7533 should be spec., yeah. New issue for implementation? Or, just leave that over in hbase-7533. Trying to implement the spec. will probably bring on a spec edit or two I'd say.

          Show
          stack added a comment - Devaraj Das I'd say let this issue be about reducing buffer copies. hbase-7533 should be spec., yeah. New issue for implementation? Or, just leave that over in hbase-7533. Trying to implement the spec. will probably bring on a spec edit or two I'd say.
          Hide
          Devaraj Das added a comment -

          stack, yes, there is overlap... I am currently trying to fix up HBASE-7213's test failures. I'll circle back on HBASE-7533 sometime by end of today or tomorrow. We can keep 7533 as a spec jira and use this for impl. Does it work.

          Show
          Devaraj Das added a comment - stack , yes, there is overlap... I am currently trying to fix up HBASE-7213 's test failures. I'll circle back on HBASE-7533 sometime by end of today or tomorrow. We can keep 7533 as a spec jira and use this for impl. Does it work.
          Hide
          stack added a comment -

          Devaraj Das Looking at the patch, there is a bunch of overlap with HBASE-7533 where we are trying to spec out what goes on wire before coding it up. What you reckon? If we remove the 7533 stuff from this patch, what is left over? What buffer copies are we saving in particular? Good on you boss.

          Show
          stack added a comment - Devaraj Das Looking at the patch, there is a bunch of overlap with HBASE-7533 where we are trying to spec out what goes on wire before coding it up. What you reckon? If we remove the 7533 stuff from this patch, what is left over? What buffer copies are we saving in particular? Good on you boss.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12563981/5945-in-progress.2.1.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 lineLengths. The patch introduces lines longer than 100

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.client.TestFromClientSide
          org.apache.hadoop.hbase.TestLocalHBaseCluster

          -1 core zombie tests. There are 8 zombie test(s): at org.apache.hadoop.hbase.master.TestMasterFailover.testMasterFailoverWithMockedRITOnDeadRS(TestMasterFailover.java:833)

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12563981/5945-in-progress.2.1.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 6 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 1 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces lines longer than 100 -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.client.TestFromClientSide org.apache.hadoop.hbase.TestLocalHBaseCluster -1 core zombie tests . There are 8 zombie test(s): at org.apache.hadoop.hbase.master.TestMasterFailover.testMasterFailoverWithMockedRITOnDeadRS(TestMasterFailover.java:833) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3951//console This message is automatically generated.
          Hide
          Devaraj Das added a comment -

          If we want to support BufferChain kind of request data for request as well, we'd need to put in a field in the requestheader to indicate as such to the server.. (thinking aloud..).

          Show
          Devaraj Das added a comment - If we want to support BufferChain kind of request data for request as well, we'd need to put in a field in the requestheader to indicate as such to the server.. (thinking aloud..).
          Hide
          Devaraj Das added a comment -

          This is rebased patch (after HBASE-7479). I have updated RPC.proto in the patch with a description of what the RPC messages are. I think we can handle the KeyValue bytebuffers as well (in the RPC layer by defining a common interface for Message and ByteBuffer[]). For now, I haven't done it. I only introduced a responseBodyType blob that could be used to indicate the type of response. I also put in a TODO in RpcServer.java explaining the interface.

          Is this saving a buffer copy or is it just doing what writeDelimitedTo does?

          Actually, in principle it is doing the same thing as writeDelimitedTo. But writeDelimitedTo creates a buffer internally which is of little use since we are already writing the output to a buffer. I don't know whether this will make any difference or not in practice but I thought why simply create a buffer unnecessarily..I create CodedOutputStream with a buffer size of '1' (buffer size of '0' makes it throw exceptions..).

          Show
          Devaraj Das added a comment - This is rebased patch (after HBASE-7479 ). I have updated RPC.proto in the patch with a description of what the RPC messages are. I think we can handle the KeyValue bytebuffers as well (in the RPC layer by defining a common interface for Message and ByteBuffer[]). For now, I haven't done it. I only introduced a responseBodyType blob that could be used to indicate the type of response. I also put in a TODO in RpcServer.java explaining the interface. Is this saving a buffer copy or is it just doing what writeDelimitedTo does? Actually, in principle it is doing the same thing as writeDelimitedTo. But writeDelimitedTo creates a buffer internally which is of little use since we are already writing the output to a buffer. I don't know whether this will make any difference or not in practice but I thought why simply create a buffer unnecessarily..I create CodedOutputStream with a buffer size of '1' (buffer size of '0' makes it throw exceptions..).
          Hide
          Devaraj Das added a comment -

          I swear that I had generated the patch correctly but it seems like I uploaded the incorrect one again.. I'll update the patch shortly with the comments. Thanks for looking, stack.

          Show
          Devaraj Das added a comment - I swear that I had generated the patch correctly but it seems like I uploaded the incorrect one again.. I'll update the patch shortly with the comments. Thanks for looking, stack .
          Hide
          stack added a comment -

          Looked at Todds hackery again. Its lovely. I'd say lets try get it in.

          Looking at DD's patch, removing special response and request types looks right.... sending the request and response Messages instead w/ method name stuff moved up into header (other changes are coming down the pipe I'd say; e.g. hbase-7479 removes client protocol version altogether and error should be subsumed by response moved from response header .. but for later).

          EmptyMsg looks like it already exists.

          Is this saving a buffer copy or is it just doing what writeDelimitedTo does?

          • call.param.writeDelimitedTo(this.out);
            + CodedOutputStream cos = CodedOutputStream.newInstance(out,1);
            + cos.writeRawVarint32(call.rpcRequest.requestArg.getSerializedSize());
            + call.rpcRequest.requestArg.writeTo(cos);
            + cos.flush();

          There seem to be classes missing from the patch, ClientSideRpcRequest?

          The Todd BufferChain looks nice. Might as well use it if he has put up the code (and a test). We'll want it for sure when we need to pass encoded data blocks after protobuf request/response (HBASE-7233)

          Show
          stack added a comment - Looked at Todds hackery again. Its lovely. I'd say lets try get it in. Looking at DD's patch, removing special response and request types looks right.... sending the request and response Messages instead w/ method name stuff moved up into header (other changes are coming down the pipe I'd say; e.g. hbase-7479 removes client protocol version altogether and error should be subsumed by response moved from response header .. but for later). EmptyMsg looks like it already exists. Is this saving a buffer copy or is it just doing what writeDelimitedTo does? call.param.writeDelimitedTo(this.out); + CodedOutputStream cos = CodedOutputStream.newInstance(out,1); + cos.writeRawVarint32(call.rpcRequest.requestArg.getSerializedSize()); + call.rpcRequest.requestArg.writeTo(cos); + cos.flush(); There seem to be classes missing from the patch, ClientSideRpcRequest? The Todd BufferChain looks nice. Might as well use it if he has put up the code (and a test). We'll want it for sure when we need to pass encoded data blocks after protobuf request/response ( HBASE-7233 )
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12563396/5945-in-progress.2.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3873//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12563396/5945-in-progress.2.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3873//console This message is automatically generated.
          Hide
          Devaraj Das added a comment -

          Had uploaded an incomplete patch earlier.. This is the more appropriate one.

          Show
          Devaraj Das added a comment - Had uploaded an incomplete patch earlier.. This is the more appropriate one.
          Hide
          Devaraj Das added a comment -

          stack, hope it is okay with you that I am attaching a patch here... (I was tempted to do a patch for this issue..)

          The main change in this patch is that it removes the RpcRequestBody from RPC.proto, and instead serializes the rpc method argument directly to the underlying output. There is a lot of change associated with this removal in the patch.

          I didn't change the serialization of the rpc header fields yet (as was done by Todd Lipcon in his earlier patch). The reason being that I don't think the header part is a concern since they will be a few 10s of bytes and mostly will be noise. The ser/de of the rpc request-body/response-body is where I tried to improve on. The codebase compiles with this patch now but there is work still left (and maybe I missed some buffer copies as well; need to dig some more).

          Show
          Devaraj Das added a comment - stack , hope it is okay with you that I am attaching a patch here... (I was tempted to do a patch for this issue..) The main change in this patch is that it removes the RpcRequestBody from RPC.proto, and instead serializes the rpc method argument directly to the underlying output. There is a lot of change associated with this removal in the patch. I didn't change the serialization of the rpc header fields yet (as was done by Todd Lipcon in his earlier patch). The reason being that I don't think the header part is a concern since they will be a few 10s of bytes and mostly will be noise. The ser/de of the rpc request-body/response-body is where I tried to improve on. The codebase compiles with this patch now but there is work still left (and maybe I missed some buffer copies as well; need to dig some more).
          Hide
          Devaraj Das added a comment -

          stack, I am up for helping out with this. I'll try to do some analysis as well and get back... I was planning to do this as part of HBASE-6614.

          Show
          Devaraj Das added a comment - stack , I am up for helping out with this. I'll try to do some analysis as well and get back... I was planning to do this as part of HBASE-6614 .
          Hide
          stack added a comment -

          Assigning myself unless someone else wants to take it. I am trying to dig in on rpc after the cp's have all gone over to pb.

          Show
          stack added a comment - Assigning myself unless someone else wants to take it. I am trying to dig in on rpc after the cp's have all gone over to pb.
          Hide
          Todd Lipcon added a comment -

          Was reminded of this JIRA today. Anyone feel like picking it up? I'm a delinquent HBase contributor these days, so probably better if someone steals it from me (sorry guys!)

          Show
          Todd Lipcon added a comment - Was reminded of this JIRA today. Anyone feel like picking it up? I'm a delinquent HBase contributor these days, so probably better if someone steals it from me (sorry guys!)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525785/hbase-5495.txt
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1908//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12525785/hbase-5495.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1908//console This message is automatically generated.
          Hide
          stack added a comment -

          /me hearts todd

          Show
          stack added a comment - /me hearts todd
          Hide
          Todd Lipcon added a comment -

          attached does the optimization on the client side as well, and fixes a bug with serializing exception responses (forgot to length-prefix it)

          Show
          Todd Lipcon added a comment - attached does the optimization on the client side as well, and fixes a bug with serializing exception responses (forgot to length-prefix it)
          Hide
          Todd Lipcon added a comment -

          Yep, I think the next improvement is to use thread-local or connection-local buffers - perhaps even direct buffers. There's still one more copy from the heap byte buffer to a direct buffer within the SocketChannel.write() which would be avoided if we serialized directly into a DirectByteBuffer. Have to benchmark whether that's worth it.

          We should also do the same hacks on the client side of the connection - should get similar gains.

          Show
          Todd Lipcon added a comment - Yep, I think the next improvement is to use thread-local or connection-local buffers - perhaps even direct buffers. There's still one more copy from the heap byte buffer to a direct buffer within the SocketChannel.write() which would be avoided if we serialized directly into a DirectByteBuffer. Have to benchmark whether that's worth it. We should also do the same hacks on the client side of the connection - should get similar gains.
          Hide
          stack added a comment -

          Looks great Todd.

          Could we keep this DataOutputBuffer for reuse?

          +      DataOutputBuffer buf = new DataOutputBuffer(size);
          
          Show
          stack added a comment - Looks great Todd. Could we keep this DataOutputBuffer for reuse? + DataOutputBuffer buf = new DataOutputBuffer(size);
          Hide
          stack added a comment -

          Making critical so we don't overlook this work.

          Show
          stack added a comment - Making critical so we don't overlook this work.
          Hide
          Todd Lipcon added a comment -

          New rev gets rid of some more. This seems to make a noticeable difference in my oprofile output and YCSB results. Would appreciate if other folks could verify

          (yes, patch still needs more work, please don't review for style/licenses/etc)

          Show
          Todd Lipcon added a comment - New rev gets rid of some more. This seems to make a noticeable difference in my oprofile output and YCSB results. Would appreciate if other folks could verify (yes, patch still needs more work, please don't review for style/licenses/etc)
          Hide
          Ted Yu added a comment -

          Interesting finding.

          Can I assume that the number of occurrences of Message's is higher than that of Result's ?
          If so, the following check can be lifted above that for Result.class:

          +    if (instance instanceof Message) {
          
          Show
          Ted Yu added a comment - Interesting finding. Can I assume that the number of occurrences of Message's is higher than that of Result's ? If so, the following check can be lifted above that for Result.class: + if (instance instanceof Message) {
          Hide
          Todd Lipcon added a comment -

          Here's a first pass. I don't have great benchmark results, but it looked from my "jstat -gcutil" output like this cut the amount of allocation by a factor of 2.5x or so for a benchmark that returned large responses.

          Can probably get rid of at least one more copy in there with a bit more trickery.

          Show
          Todd Lipcon added a comment - Here's a first pass. I don't have great benchmark results, but it looked from my "jstat -gcutil" output like this cut the amount of allocation by a factor of 2.5x or so for a benchmark that returned large responses. Can probably get rid of at least one more copy in there with a bit more trickery.

            People

            • Assignee:
              stack
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              18 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development