GEODE-9641: Remove unnecessary allocation of HeapDataOutputStream

[kamilla1201]

GEODE-9641 was introduced by the fix for GEODE-9204.
This change removes the unnecessary allocation of HeapDataOutputStream in ReplyMessage.toData().
I ran P2pPartitionedPutBytesBenchmark at least 15 times to test this fix and all runs passed.

For all changes:

• Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

• Has your PR been rebased against the latest commit within the target branch (typically develop)?

• Is your initial contribution a single, squashed commit?

• Does gradlew build run cleanly?

• Have you written or updated unit tests to verify your changes?

• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

[nonbinaryprogrammer]

nice performance improvement

[kirklund]

[comment] (Just a comment, not a request for changes) It would be best to close hdos in a finally-block. The try-with-resource syntax is generally the preferred syntax. If you make this change, you would probably have to do some refactoring to extract methods or it won't be very clean.

It's safest to do refactoring like the following example if there's a decent unit test. ReplyMessageTest exists but unfortunately, it doesn't cover much.

You could try something like this which also seems to be more readable. In order: extracted writeStatus, extracted writeProcessProcessorId, moved hdos into a try-with-resource block, polished up the try-with-resource block, extracted writeReturnValue which pulled the nested try-catch block out:

  @Override
  public void toData(DataOutput out,
      SerializationContext context) throws IOException {
    super.toData(out, context);

    boolean failedSerialization = false;
    final boolean hasReturnValue = this.returnValueIsException || this.returnValue != null;
    if (hasReturnValue) {
      try (HeapDataOutputStream hdos = new HeapDataOutputStream(context.getSerializationVersion())) {
        writeReturnValue(out, context, failedSerialization, hdos);
      }
    } else {
      writeStatus(out);
      writeProcessorId(out);
    }
  }

  private void writeReturnValue(DataOutput out, SerializationContext context, 
      boolean failedSerialization, HeapDataOutputStream hdos) throws IOException {
    try {
      context.getSerializer().writeObject(this.returnValue, hdos);
    } catch (NotSerializableException e) {
      logger.warn("Unable to serialize a reply to " + getRecipientsDescription(), e);
      failedSerialization = true;
      this.returnValue = new ReplyException(e);
      this.returnValueIsException = true;
    }
    writeStatus(out);
    writeProcessorId(out);
    if (failedSerialization) {
      context.getSerializer().writeObject(this.returnValue, out);
    } else {
      hdos.sendTo(out);
    }
  }

  private void writeProcessorId(DataOutput out) throws IOException {
    if (this.processorId != 0) {
      out.writeInt(processorId);
    }
  }

  private void writeStatus(DataOutput out) throws IOException {
    byte status = 0;
    if (this.ignored) {
      status |= IGNORED_FLAG;
    }
    if (this.returnValueIsException) {
      status |= EXCEPTION_FLAG;
    } else if (this.returnValue != null) {
      status |= OBJECT_FLAG;
    }
    if (this.processorId != 0) {
      status |= PROCESSOR_ID_FLAG;
    }
    if (this.closed) {
      status |= CLOSED_FLAG;
    }
    if (this.internal) {
      status |= INTERNAL_FLAG;
    }
    out.writeByte(status);
  }

[dschneider-pivotal]

something else to consider in this method is should you even call hdos.close? If you look at it all it is doing is freeing up some heap resources and reseting the BufferDataOutputStream which actually mean you could continue to use it (which seems a bit off since close usually ends the life of an object). If you had a HDOS that was stored in something that was going to live longer than close could help free some memory up early. But in this case HDOS becomes garbage at the end of this method. The best practice in that case is to just let the garbage collector free it up and to not class close. But I'd be okay with you leaving the close call in but if it is going to cause a major refactor you might want to instead not call it.

[dschneider-pivotal]

something else to consider in this method is should you even call hdos.close? If you look at it all it is doing is freeing up some heap resources and reseting the BufferDataOutputStream which actually mean you could continue to use it (which seems a bit off since close usually ends the life of an object). If you had a HDOS that was stored in something that was going to live longer than close could help free some memory up early. But in this case HDOS becomes garbage at the end of this method. The best practice in that case is to just let the garbage collector free it up and to not class close. But I'd be okay with you leaving the close call in but if it is going to cause a major refactor you might want to instead not call it.

[lgtm-com]

This pull request introduces 1 alert when merging 4d9027e into 3514680 - view on LGTM.com

new alerts:

• 1 for Potential output resource leak