Refactor hot methods

[DomGarguilo]

In #1099 there are some lists of methods that have been reported as "hot method too big".

This PR refactors some of those methods to make them smaller. After these changes, the following methods are no longer reported as "hot method too big":

• org.apache.accumulo.core.file.rfile.RelativeKey::<init>
• org.apache.accumulo.core.file.rfile.RelativeKey::readFields
• org.apache.accumulo.server.fs.FileManager$ScanFileManager::openFiles

The following have been refactored to reduce size, but are still being reported:

• org.apache.accumulo.core.iteratorsImpl.system.LocalityGroupIterator::_seek
• org.apache.accumulo.tserver.scan.LookupTask::run

[DomGarguilo]

I have marked this as WIP since there is still work to be done here:

• Look into refactoring more methods appearing in the list of methods in #1099
• Rename new methods. I am not sure what to name the new methods so suggestions are very welcome.
  • Namely, the new methods in RelativeKey: extracted() and getBytes(). Both of which I have left as thier IDE generated names

[EdColeman]

Sorry - this comment applies to #2812

Looking at the method in 'Key' the method that performs the equality check on a byte array private static boolean isEqual(byte[] a1, byte[] a2) is already optimized for Accumulo row comparisons, checking the last few bytes first and does not seem to need additional optimization.

[DomGarguilo]

Looking at the method in 'Key' the method that performs the equality check on a byte array private static boolean isEqual(byte[] a1, byte[] a2) is already optimized for Accumulo row comparisons, checking the last few bytes first and does not seem to need additional optimization.

Is this in relation to #2812? If so, do you think its worth investigating rearranging some of the comparisons made as mentioned in your comments here, or do you think #2812 should be closed?

[DomGarguilo]

A few more methods were found (details). I'll look into those now, before this is merged.

[DomGarguilo]

A few more methods were found (details). I'll look into those now, before this is merged.

Never mind. Took a look and I don't think those methods will be changed in this PR. This PR is ready for review.

[DomGarguilo]

I have refactored Key.equals() in a way that makes it not show up in the hot methods list any more. Here is the method after refactoring:

  public boolean equals(Key other, PartialKey part) {
    boolean result = isEqual(row, other.row);
    if (part == ROW)
      return result;
    result &= isEqual(colFamily, other.colFamily);
    if (part == ROW_COLFAM)
      return result;
    result &= isEqual(colQualifier, other.colQualifier);
    if (part == ROW_COLFAM_COLQUAL)
      return result;
    result &= isEqual(colVisibility, other.colVisibility);
    if (part == ROW_COLFAM_COLQUAL_COLVIS)
      return result;
    result &= (timestamp == other.timestamp);
    if (part == ROW_COLFAM_COLQUAL_COLVIS_TIME)
      return result;
    result &= (deleted == other.deleted);
    if (part == ROW_COLFAM_COLQUAL_COLVIS_TIME_DEL)
      return result;

    throw new IllegalArgumentException("Unrecognized partial key specification " + part);
  }

It will fall through and compare each PartialKey until it reaches part and returns. Here is the current code to compare: https://github.com/apache/accumulo/blob/388d3ffe11b3eaffe5a0022907d10e18bef73253/core/src/main/java/org/apache/accumulo/core/data/Key.java#L977-L1002

While this refactored method is smaller and no longer "hot", it should probably be determined if this has any significant performance hits which is what is being invstigated in #2812

[milleruntime]

Thanks for taking a look at these methods. I will take a look at this today.

[DomGarguilo]

In 89a2f32 I added the equals method described above. This updated method does not appear on the hot methods list.

[milleruntime]

@DomGarguilo is this PR ready to merge? If so, I can take another close look at it.

[DomGarguilo]

@DomGarguilo is this PR ready to merge? If so, I can take another close look at it.

Yes, I think I am done making changes here and was just waiting for someone to verify these changes.