Skip to content

Commit

Permalink
Remove unused data from compilation unit annotation model util map #665…
Browse files Browse the repository at this point in the history 
… (#677)

The CompilationUnitDocumentProvider uses in its Java-specific annotation
model a utility map to navigate from document positions back to
annotations. The current implementation is slow whenever a document
contains plenty of annotations (such as the occurrences of the name of a
marked method), as the map internally uses a list for representing the
annotations which becomes slow in adding/removing when containing plenty
of data. This is especially a problem on batch replacements of
annotations (e.g. when selecting a different method to be marked in the
document), as every single annotation is processed by the map
individually. In addition, the map currently stores data that is never
used, as only the specific JavaMarkerAnnotations are used from that map.

This change thus does the following:
- Specializes the utility map to only contain the required type of
annotations
- Refactors the map implementation to simplify the code and make it more
comprehensible

Fixes #665
  • Loading branch information
@HeikoKlare
HeikoKlare authored Aug 1, 2023
1 parent 84d450b commit 979b884
Showing 1 changed file with 71 additions and 95 deletions.
166 changes: 71 additions & 95 deletions ...pse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/CompilationUnitDocumentProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.io.Reader;
import java.net.URI;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
Expand Down Expand Up @@ -434,83 +435,86 @@ public boolean isQuickFixable() {
}

/**
* Internal structure for mapping positions to some value.
* Internal structure for mapping positions to JavaMarkerAnnotations.
* The reason for this specific structure is that positions can
* change over time. Thus a lookup is based on value and not
* on hash value.
*/
protected static class ReverseMap {
private static class ReverseJavaMarkerAnnotationsMap {

static class Entry {
Position fPosition;
Object fValue;
}
private final Position position;

final List<JavaMarkerAnnotation> annotations= new ArrayList<>();

private List<Entry> fList= new ArrayList<>(2);
private int fAnchor= 0;
public Entry(Position position) {
this.position= position;
}

public ReverseMap() {
public boolean isFor(Position positionToCheck) {
return position.equals(positionToCheck);
}
}

public Object get(Position position) {
private List<Entry> entries= new ArrayList<>(2);

Entry entry;
private int anchorIndex= 0;

// behind anchor
int length= fList.size();
for (int i= fAnchor; i < length; i++) {
entry= fList.get(i);
if (entry.fPosition.equals(position)) {
fAnchor= i;
return entry.fValue;
}
public Iterable<JavaMarkerAnnotation> get(Position position) {
// Start behind and store anchor because usually retrieval is performed sequentially over positions
Entry entry= getEntry(position, true);
if (entry != null) {
return entry.annotations;
} else {
return Collections.emptyList();
}
}

// before anchor
for (int i= 0; i < fAnchor; i++) {
entry= fList.get(i);
if (entry.fPosition.equals(position)) {
fAnchor= i;
return entry.fValue;
private Entry getEntry(Position position, boolean useAndStoreAnchor) {
int startIndex= useAndStoreAnchor ? anchorIndex : 0;
int numberOfIndices= entries.size();
for (int i= 0; i < numberOfIndices; i++) {
int indexToCheck= (i + startIndex) % numberOfIndices;
Entry entry= entries.get(indexToCheck);
if (entry.isFor(position)) {
if (useAndStoreAnchor) {
anchorIndex= indexToCheck;
}
return entry;
}
}

return null;
}

private int getIndex(Position position) {
Entry entry;
int length= fList.size();
for (int i= 0; i < length; i++) {
entry= fList.get(i);
if (entry.fPosition.equals(position))
return i;
}
return -1;
public void put(Position position, JavaMarkerAnnotation annotation) {
Entry entry= getOrCreateEntry(position);
entry.annotations.add(annotation);
}

public void put(Position position, Object value) {
int index= getIndex(position);
if (index == -1) {
Entry entry= new Entry();
entry.fPosition= position;
entry.fValue= value;
fList.add(entry);
} else {
Entry entry= fList.get(index);
entry.fValue= value;
private Entry getOrCreateEntry(Position position) {
Entry entry= getEntry(position, false);
if (entry == null) {
entry= new Entry(position);
entries.add(entry);
}
return entry;
}

public void remove(Position position) {
int index= getIndex(position);
if (index > -1)
fList.remove(index);
public void remove(Position position, JavaMarkerAnnotation annotation) {
Entry entry= getEntry(position, false);
if (entry != null) {
if (entry.annotations.size() == 1) {
entries.remove(entry);
} else {
entry.annotations.remove(annotation);
}
}
}

public void clear() {
fList.clear();
entries.clear();
}

}

/**
Expand All @@ -534,7 +538,7 @@ private static class ProblemRequestorState {
private boolean fIsActive= false;
private boolean fIsHandlingTemporaryProblems;

private ReverseMap fReverseMap= new ReverseMap();
private ReverseJavaMarkerAnnotationsMap fReverseMap= new ReverseJavaMarkerAnnotationsMap();
private List<JavaMarkerAnnotation> fPreviouslyOverlaid= null;
private List<JavaMarkerAnnotation> fCurrentlyOverlaid= new ArrayList<>();
private Thread fActiveThread;
Expand Down Expand Up @@ -738,31 +742,22 @@ private void removeMarkerOverlays(boolean isCanceled) {
}

/**
* Overlays value with problem annotation.
* Overlays Java marker annotation with problem annotation.
*
* @param value the value
* @param annotation the Java marker annotation to attach a problem annotation to
* @param problemAnnotation the problem annotation
*/
private void setOverlay(Object value, ProblemAnnotation problemAnnotation) {
if (value instanceof JavaMarkerAnnotation) {
JavaMarkerAnnotation annotation= (JavaMarkerAnnotation) value;
if (annotation.isProblem()) {
annotation.setOverlay(problemAnnotation);
fPreviouslyOverlaid.remove(annotation);
fCurrentlyOverlaid.add(annotation);
}
} else {
private void setOverlay(JavaMarkerAnnotation annotation, ProblemAnnotation problemAnnotation) {
if (annotation.isProblem()) {
annotation.setOverlay(problemAnnotation);
fPreviouslyOverlaid.remove(annotation);
fCurrentlyOverlaid.add(annotation);
}
}

private void overlayMarkers(Position position, ProblemAnnotation problemAnnotation) {
Object value= getAnnotations(position);
if (value instanceof List) {
List<?> list= (List<?>) value;
for (Object name : list)
setOverlay(name, problemAnnotation);
} else {
setOverlay(value, problemAnnotation);
private void overlayMarkers(Position position, ProblemAnnotation problemAnnotation) {
for (JavaMarkerAnnotation annotation : getJavaMarkerAnnotations(position)) {
setOverlay(annotation, problemAnnotation);
}
}

Expand Down Expand Up @@ -826,7 +821,7 @@ public void setIsHandlingTemporaryProblems(boolean enable) {

}

private Object getAnnotations(Position position) {
private Iterable<JavaMarkerAnnotation> getJavaMarkerAnnotations(Position position) {
synchronized (getLockObject()) {
return fReverseMap.get(position);
}
Expand All @@ -839,19 +834,9 @@ private Object getAnnotations(Position position) {
protected void addAnnotation(Annotation annotation, Position position, boolean fireModelChanged) throws BadLocationException {
super.addAnnotation(annotation, position, fireModelChanged);

synchronized (getLockObject()) {
Object cached= fReverseMap.get(position);
if (cached == null)
fReverseMap.put(position, annotation);
else if (cached instanceof List) {
@SuppressWarnings("unchecked")
List<Object> list= (List<Object>) cached;
list.add(annotation);
} else if (cached instanceof Annotation) {
List<Object> list= new ArrayList<>(2);
list.add(cached);
list.add(annotation);
fReverseMap.put(position, list);
if (annotation instanceof JavaMarkerAnnotation javaAnnotation) {
synchronized (getLockObject()) {
fReverseMap.put(position, javaAnnotation);
}
}
}
Expand All @@ -873,18 +858,9 @@ protected void removeAllAnnotations(boolean fireModelChanged) {
@Override
protected void removeAnnotation(Annotation annotation, boolean fireModelChanged) {
Position position= getPosition(annotation);
synchronized (getLockObject()) {
Object cached= fReverseMap.get(position);
if (cached instanceof List) {
@SuppressWarnings("unchecked")
List<Object> list= (List<Object>) cached;
list.remove(annotation);
if (list.size() == 1) {
fReverseMap.put(position, list.get(0));
list.clear();
}
} else if (cached instanceof Annotation) {
fReverseMap.remove(position);
if (annotation instanceof JavaMarkerAnnotation javaAnnotation) {
synchronized (getLockObject()) {
fReverseMap.remove(position, javaAnnotation);
}
}
super.removeAnnotation(annotation, fireModelChanged);
Expand Down

0 comments on commit 979b884

Please sign in to comment.