Skip to content

Extract Method: isolate END_ELEMENT handling in computeNext() #2489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
EmanuelAry wants to merge 1 commit into
base: main
Choose a base branch
from
+154 −124
Open

Extract Method: isolate END_ELEMENT handling in computeNext() #2489

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
278 changes: 154 additions & 124 deletions .../device_broker/java/com/google/android/apps/common/testing/suite/dex/DexDumpIterator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,148 +117,178 @@ private String getAttributeValue(String attributeName) {
return result;
}

@Override
protected DexClassData computeNext() {

private static class EndElementResult {
final DexClassData.Builder currentClass;
final MethodData.MethodBuilder currentMethod;
final boolean inConstructor;
final DexClassData returnValue;

EndElementResult(DexClassData.Builder currentClass,
MethodData.MethodBuilder currentMethod,
boolean inConstructor,
DexClassData returnValue) {
this.currentClass = currentClass;
this.currentMethod = currentMethod;
this.inConstructor = inConstructor;
this.returnValue = returnValue;
}
}

@Override
protected DexClassData computeNext() {
DexClassData.Builder currentClass = null;
MethodData.MethodBuilder currentMethod = null;
Deque<AnnotationContext> annotationStack = Lists.newLinkedList();
boolean inConstructor = false;

while (streamHasNext()) {
String name = null;
switch (streamNext()) {
case END_DOCUMENT:
return endOfData();
case START_ELEMENT:
currentLocation = xmlStreamReader.getLocation();
name = xmlStreamReader.getLocalName();
if ("package".equals(name)) {
currentPackage = handleNewPackageNode();
} else if ("class".equals(name)) {
currentClass = handleNewClassNode(currentClass);
} else if ("method".equals(name)) {
currentMethod = handleNewMethodNode(currentMethod);
} else if ("parameter".equals(name)) {
currentMethod = handleParameterNode(currentMethod, inConstructor);
} else if ("constructor".equals(name)) {
checkState(!inConstructor, "Already in a constructor node. %s",
currentLocation);
inConstructor = true;
} else if ("annotation".equals(name)) {
AnnotationContext context = new AnnotationContext();
annotationStack.push(context);
context.currentAnnotation = handleNewAnnotationNode();
} else if ("anno_field".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentAnnotationValue = handleNewAnnotationFieldNode(
context.currentAnnotationValue);
annotationStack.push(context);
} else if ("anno_field_value".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentAnnotationValue = handleNewAnnotationFieldValueNode(
context.currentAnnotationValue,
context.currentArrayStatus);
annotationStack.push(context);
} else if ("anno_field_array".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentArrayStatus = handleNewAnnotationFieldArrayNode(
context.currentAnnotationValue,
context.currentArrayStatus);
annotationStack.push(context);
} else if ("array_element".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentArrayStatus = handleNewAnnotationFieldArrayElementNode(
context.currentAnnotationValue,
context.currentArrayStatus);
annotationStack.push(context);
}
break;
case END_ELEMENT:
currentLocation = xmlStreamReader.getLocation();
name = xmlStreamReader.getLocalName();
if ("package".equals(name)) {
currentPackage = handleEndPackageNode();
} else if ("class".equals(name)) {
return handleEndClassNode(currentClass, currentMethod, annotationStack);
} else if ("method".equals(name)) {
currentMethod = handleEndMethodNode(currentClass, currentMethod);
} else if ("constructor".equals(name)) {
checkState(inConstructor, "Not in constructor. %s", currentLocation);
inConstructor = false;
} else if ("annotation".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
checkState(null == context.currentAnnotationValue, "Never closed value node %s",
String name = null;
switch (streamNext()) {
case END_DOCUMENT:
return endOfData();
case START_ELEMENT:
currentLocation = xmlStreamReader.getLocation();
name = xmlStreamReader.getLocalName();
if ("package".equals(name)) {
currentPackage = handleNewPackageNode();
} else if ("class".equals(name)) {
currentClass = handleNewClassNode(currentClass);
} else if ("method".equals(name)) {
currentMethod = handleNewMethodNode(currentMethod);
} else if ("parameter".equals(name)) {
currentMethod = handleParameterNode(currentMethod, inConstructor);
} else if ("constructor".equals(name)) {
checkState(!inConstructor, "Already in a constructor node. %s",
currentLocation);
inConstructor = true;
} else if ("annotation".equals(name)) {
AnnotationContext context = new AnnotationContext();
annotationStack.push(context);
context.currentAnnotation = handleNewAnnotationNode();
} else if ("anno_field".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentAnnotationValue = handleNewAnnotationFieldNode(
context.currentAnnotationValue);
annotationStack.push(context);
} else if ("anno_field_value".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentAnnotationValue = handleNewAnnotationFieldValueNode(
context.currentAnnotationValue,
context.currentArrayStatus);
annotationStack.push(context);
} else if ("anno_field_array".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentArrayStatus = handleNewAnnotationFieldArrayNode(
context.currentAnnotationValue,
context.currentArrayStatus);
annotationStack.push(context);
} else if ("array_element".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentArrayStatus = handleNewAnnotationFieldArrayElementNode(
context.currentAnnotationValue,
context.currentArrayStatus);
annotationStack.push(context);
}
break;
case END_ELEMENT:
currentLocation = xmlStreamReader.getLocation();
name = xmlStreamReader.getLocalName();
EndElementResult result = handleEndElement(name, currentClass, currentMethod,
annotationStack, inConstructor);
currentClass = result.currentClass;
currentMethod = result.currentMethod;
inConstructor = result.inConstructor;
if (result.returnValue != null) {
return result.returnValue;
}
break;
default: { /* don't care */ }
}
currentLocation = null;
}
throw new IllegalStateException("Should never happen. Document end not emitted?");
}

private EndElementResult handleEndElement(String name,
DexClassData.Builder currentClass,
MethodData.MethodBuilder currentMethod,
Deque<AnnotationContext> annotationStack,
boolean inConstructor) {
if ("package".equals(name)) {
currentPackage = handleEndPackageNode();
} else if ("class".equals(name)) {
return new EndElementResult(currentClass, currentMethod, inConstructor,
handleEndClassNode(currentClass, currentMethod, annotationStack));
} else if ("method".equals(name)) {
currentMethod = handleEndMethodNode(currentClass, currentMethod);
} else if ("constructor".equals(name)) {
checkState(inConstructor, "Not in constructor. %s", currentLocation);
inConstructor = false;
} else if ("annotation".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s", currentLocation);
AnnotationContext context = annotationStack.pop();
checkState(null == context.currentAnnotationValue, "Never closed value node %s",
currentLocation);
checkState(null == context.currentArrayStatus, "Never closed array node %s",
checkState(null == context.currentArrayStatus, "Never closed array node %s",
currentLocation);

if (annotationStack.isEmpty()) {
handleEndAnnotationNode(
context.currentAnnotation,
currentClass,
currentMethod,
inConstructor);
} else {
AnnotationContext parentContext = annotationStack.pop();
AnnotationPb nestedAnnotation =
handleEndNestedAnnotation(
context.currentAnnotation,
parentContext.currentAnnotation,
parentContext.currentAnnotationValue);
parentContext.nestedAnnotations.add(nestedAnnotation);
annotationStack.push(parentContext);
}
} else if ("anno_field".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentAnnotationValue = handleEndAnnotationFieldNode(
if (annotationStack.isEmpty()) {
handleEndAnnotationNode(
context.currentAnnotation,
currentClass,
currentMethod,
inConstructor);
} else {
AnnotationContext parentContext = annotationStack.pop();
AnnotationPb nestedAnnotation =
handleEndNestedAnnotation(
context.currentAnnotation,
parentContext.currentAnnotation,
parentContext.currentAnnotationValue);
parentContext.nestedAnnotations.add(nestedAnnotation);
annotationStack.push(parentContext);
}
} else if ("anno_field".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s", currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentAnnotationValue = handleEndAnnotationFieldNode(
context.currentAnnotation,
context.currentAnnotationValue,
context.nestedAnnotations);
context.nestedAnnotations.clear();
annotationStack.push(context);
} else if ("anno_field_value".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
context.nestedAnnotations.clear();
annotationStack.push(context);
} else if ("anno_field_value".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s", currentLocation);
AnnotationContext context = annotationStack.pop();
checkState(null != context.currentAnnotationValue, "anno value not set. %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
checkState(null != context.currentAnnotationValue, "anno value not set. %s",
currentLocation);
annotationStack.push(context);
} else if ("anno_field_array".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentArrayStatus = handleEndAnnotationFieldArrayNode(
annotationStack.push(context);
} else if ("anno_field_array".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s", currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentArrayStatus = handleEndAnnotationFieldArrayNode(
context.currentAnnotationValue,
context.currentArrayStatus);
annotationStack.push(context);
} else if ("array_element".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentArrayStatus = handleEndAnnotationFieldArrayElementNode(
annotationStack.push(context);
} else if ("array_element".equals(name)) {
checkState(!annotationStack.isEmpty(), "Not in annotation! %s", currentLocation);
AnnotationContext context = annotationStack.pop();
context.currentArrayStatus = handleEndAnnotationFieldArrayElementNode(
context.currentAnnotationValue,
context.currentArrayStatus);
annotationStack.push(context);
}
break;
default: { /* don't care */ }
}
currentLocation = null;
annotationStack.push(context);
}
throw new IllegalStateException("Should never happen. Document end not emitted?");
}
return new EndElementResult(currentClass, currentMethod, inConstructor, null);
}
Comment on lines +121 to +291

@gemini-code-assist gemini-code-assist Bot Jun 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Performance & Code Quality Improvement: Eliminate EndElementResult Allocations

The DexDumpIterator class is designed to parse very large XML files (~10-100MB) efficiently, as noted in the class-level JAVADOC. Introducing a new EndElementResult object for every single END_ELEMENT event (which can occur millions of times in a large file) introduces significant and unnecessary garbage collection pressure.

Instead of creating a temporary result holder class to pass mutable state back and forth, we can promote currentClass, currentMethod, annotationStack, and inConstructor to instance variables of DexDumpIterator (similar to how currentPackage and currentLocation are already managed).

This refactoring provides several key benefits:

  1. Performance: Completely eliminates millions of short-lived EndElementResult allocations, significantly reducing GC overhead.
  2. Simplicity: Simplifies the signature of handleEndElement (and potentially other helper methods) as they no longer need to accept or return these state variables.
  3. Readability & Style: The main loop in computeNext() becomes much cleaner, and we can restore the consistent 2-space indentation used throughout the rest of the file.
  private DexClassData.Builder currentClass = null;
  private MethodData.MethodBuilder currentMethod = null;
  private final Deque<AnnotationContext> annotationStack = Lists.newLinkedList();
  private boolean inConstructor = false;

  @Override
  protected DexClassData computeNext() {
    while (streamHasNext()) {
      String name = null;
      switch (streamNext()) {
        case END_DOCUMENT:
          return endOfData();
        case START_ELEMENT:
          currentLocation = xmlStreamReader.getLocation();
          name = xmlStreamReader.getLocalName();
          if ("package".equals(name)) {
            currentPackage = handleNewPackageNode();
          } else if ("class".equals(name)) {
            currentClass = handleNewClassNode(currentClass);
          } else if ("method".equals(name)) {
            currentMethod = handleNewMethodNode(currentMethod);
          } else if ("parameter".equals(name)) {
            currentMethod = handleParameterNode(currentMethod, inConstructor);
          } else if ("constructor".equals(name)) {
            checkState(!inConstructor, "Already in a constructor node. %s",
                currentLocation);
            inConstructor = true;
          } else if ("annotation".equals(name)) {
            AnnotationContext context = new AnnotationContext();
            annotationStack.push(context);
            context.currentAnnotation = handleNewAnnotationNode();
          } else if ("anno_field".equals(name)) {
            checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
                currentLocation);
            AnnotationContext context = annotationStack.pop();
            context.currentAnnotationValue = handleNewAnnotationFieldNode(
                context.currentAnnotationValue);
            annotationStack.push(context);
          } else if ("anno_field_value".equals(name)) {
            checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
                currentLocation);
            AnnotationContext context = annotationStack.pop();
            context.currentAnnotationValue = handleNewAnnotationFieldValueNode(
                context.currentAnnotationValue,
                context.currentArrayStatus);
            annotationStack.push(context);
          } else if ("anno_field_array".equals(name)) {
            checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
                currentLocation);
            AnnotationContext context = annotationStack.pop();
            context.currentArrayStatus = handleNewAnnotationFieldArrayNode(
                context.currentAnnotationValue,
                context.currentArrayStatus);
            annotationStack.push(context);
          } else if ("array_element".equals(name)) {
            checkState(!annotationStack.isEmpty(), "Not in annotation! %s",
                currentLocation);
            AnnotationContext context = annotationStack.pop();
            context.currentArrayStatus = handleNewAnnotationFieldArrayElementNode(
                context.currentAnnotationValue,
                context.currentArrayStatus);
            annotationStack.push(context);
          }
          break;
        case END_ELEMENT:
          currentLocation = xmlStreamReader.getLocation();
          name = xmlStreamReader.getLocalName();
          DexClassData classData = handleEndElement(name);
          if (classData != null) {
            return classData;
          }
          break;
        default: { /* don't care */ }
      }
      currentLocation = null;
    }
    throw new IllegalStateException("Should never happen. Document end not emitted?");
  }

  private DexClassData handleEndElement(String name) {
    if ("package".equals(name)) {
      currentPackage = handleEndPackageNode();
    } else if ("class".equals(name)) {
      DexClassData classData = handleEndClassNode(currentClass, currentMethod, annotationStack);
      currentClass = null;
      return classData;
    } else if ("method".equals(name)) {
      currentMethod = handleEndMethodNode(currentClass, currentMethod);
    } else if ("constructor".equals(name)) {
      checkState(inConstructor, "Not in constructor. %s", currentLocation);
      inConstructor = false;
    } else if ("annotation".equals(name)) {
      checkState(!annotationStack.isEmpty(), "Not in annotation! %s", currentLocation);
      AnnotationContext context = annotationStack.pop();
      checkState(null == context.currentAnnotationValue, "Never closed value node %s",
          currentLocation);
      checkState(null == context.currentArrayStatus, "Never closed array node %s",
          currentLocation);

      if (annotationStack.isEmpty()) {
        handleEndAnnotationNode(
            context.currentAnnotation,
            currentClass,
            currentMethod,
            inConstructor);
      } else {
        AnnotationContext parentContext = annotationStack.pop();
        AnnotationPb nestedAnnotation =
            handleEndNestedAnnotation(
                context.currentAnnotation,
                parentContext.currentAnnotation,
                parentContext.currentAnnotationValue);
        parentContext.nestedAnnotations.add(nestedAnnotation);
        annotationStack.push(parentContext);
      }
    } else if ("anno_field".equals(name)) {
      checkState(!annotationStack.isEmpty(), "Not in annotation! %s", currentLocation);
      AnnotationContext context = annotationStack.pop();
      context.currentAnnotationValue = handleEndAnnotationFieldNode(
          context.currentAnnotation,
          context.currentAnnotationValue,
          context.nestedAnnotations);
      context.nestedAnnotations.clear();
      annotationStack.push(context);
    } else if ("anno_field_value".equals(name)) {
      checkState(!annotationStack.isEmpty(), "Not in annotation! %s", currentLocation);
      AnnotationContext context = annotationStack.pop();
      checkState(null != context.currentAnnotationValue, "anno value not set. %s",
          currentLocation);
      annotationStack.push(context);
    } else if ("anno_field_array".equals(name)) {
      checkState(!annotationStack.isEmpty(), "Not in annotation! %s", currentLocation);
      AnnotationContext context = annotationStack.pop();
      context.currentArrayStatus = handleEndAnnotationFieldArrayNode(
          context.currentAnnotationValue,
          context.currentArrayStatus);
      annotationStack.push(context);
    } else if ("array_element".equals(name)) {
      checkState(!annotationStack.isEmpty(), "Not in annotation! %s", currentLocation);
      AnnotationContext context = annotationStack.pop();
      context.currentArrayStatus = handleEndAnnotationFieldArrayElementNode(
          context.currentAnnotationValue,
          context.currentArrayStatus);
      annotationStack.push(context);
    }
    return null;
  }


private static class AnnotationContext {
private AnnotationPb.Builder currentAnnotation = null;
Expand Down