[GitHub] [commons-codec] aherbert commented on a change in pull request #85: Reduce byte[] allocations by reusing buffers

[GitBox <gi...@apache.org>]

aherbert commented on a change in pull request #85:
URL: https://github.com/apache/commons-codec/pull/85#discussion_r629669314



##########
File path: src/main/java/org/apache/commons/codec/binary/BaseNCodec.java
##########
@@ -75,6 +75,7 @@
          * Buffer for streaming.
          */
         byte[] buffer;
+        byte[] bufferCache;

Review comment:
       This should have a comment such as:
   ```
   A reuseable buffer that is allocated and assigned to 'buffer' when initialising 'buffer'
   for processing. Note that when processing finishes the 'buffer' is assigned to null to
   indicate the end of input. The allocated memory is still available in 'bufferCache' for
   reuse on subsequent input. The cache is only used when the Context is recycled as
   in the case of using a BaseNCodecInputStream to read bytes.
   ```

##########
File path: src/main/java/org/apache/commons/codec/binary/BaseNCodecInputStream.java
##########
@@ -39,12 +39,15 @@
 
     private final byte[] singleByte = new byte[1];
 
+    private final byte[] buf;

Review comment:
       Add a comment about a reusable buffer to avoid repeat allocations in the read method's while loop.

##########
File path: src/main/java/org/apache/commons/codec/binary/BaseNCodec.java
##########
@@ -569,7 +570,11 @@ public String encodeToString(final byte[] pArray) {
      */
     protected byte[] ensureBufferSize(final int size, final Context context){
         if (context.buffer == null) {
-            context.buffer = new byte[Math.max(size, getDefaultBufferSize())];
+            int bufferSize = Math.max(size, getDefaultBufferSize());
+            if (context.bufferCache == null || context.bufferCache.length != bufferSize) {

Review comment:
       I think it is only possible to call `ensureBufferSize` with a recycled `Context` when using the same BaseNCodecInputStream instance. These only encode or decode bytes in chunks of 4096 or 8192 respectively. Thus when `bufferCache` is not null then it should always be required to be initialised to the same `bufferSize`. Thus you may be able to change this to:
   ```java
   if (context.bufferCache == null) {
   ```
   
   Ideally this requires a new test that reuses a BaseNCodecInputStream instance to encode or decode a series of `byte[]` inputs to show coverage of execution paths. The Travis CI job should report coverage via Coveralls so that will indicate if the existing tests execute all these new code paths.
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org