Skip to content

Commit

Permalink
AnnotationModel uses inefficient iteration over map, take 2
Browse files Browse the repository at this point in the history
Introduced new IAnnotationMap2 interface that overrides default
forEach() to synchronize on lock object.

This halves iteration time spent in cleanup() on huge annotation models.

Fixes #892
  • Loading branch information
iloveeclipse committed Aug 1, 2023
1 parent 1bd79e4 commit 907bfa9
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 16 deletions.
2 changes: 1 addition & 1 deletion bundles/org.eclipse.text/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: %pluginName
Bundle-SymbolicName: org.eclipse.text
Bundle-Version: 3.13.100.qualifier
Bundle-Version: 3.14.0.qualifier
Bundle-Vendor: %providerName
Bundle-Localization: plugin
Export-Package:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
*
* @since 3.0
*/
class AnnotationMap implements IAnnotationMap {
class AnnotationMap implements IAnnotationMap2 {

/**
* The lock object used to synchronize the operations explicitly defined by
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -640,13 +640,23 @@ private void cleanup(boolean fireModelChanged, boolean forkNotification) {
fDocumentChanged= false;

ArrayList<Annotation> deleted= new ArrayList<>();
Iterator<Annotation> e= getAnnotationMap().keySetIterator();
IAnnotationMap annotations= getAnnotationMap();
while (e.hasNext()) {
Annotation a= e.next();
Position p= annotations.get(a);
if (p == null || p.isDeleted())
deleted.add(a);

if (!(annotations instanceof IAnnotationMap2)) {
Iterator<Annotation> e= annotations.keySetIterator();
while (e.hasNext()) {
Annotation a= e.next();
Position p= annotations.get(a);
if (p == null || p.isDeleted())
deleted.add(a);
}
} else {
// foreach is synchronized on map lock object
annotations.forEach((a, p) -> {
if (p == null || p.isDeleted()) {
deleted.add(a);
}
});
}

if (fireModelChanged && forkNotification) {
Expand Down Expand Up @@ -789,15 +799,24 @@ public void removeAllAnnotations() {
protected void removeAllAnnotations(boolean fireModelChanged) {
IAnnotationMap annotations= getAnnotationMap();
if (fDocument != null) {
Iterator<Annotation> e= getAnnotationMap().keySetIterator();
while (e.hasNext()) {
Annotation a= e.next();
Position p= annotations.get(a);
removePosition(fDocument, p);
// p.delete();
synchronized (getLockObject()) {
getAnnotationModelEvent().annotationRemoved(a, p);
AnnotationModelEvent event;
synchronized (getLockObject()) {
event = getAnnotationModelEvent();
}
if (!(annotations instanceof IAnnotationMap2)) {
Iterator<Annotation> e= getAnnotationMap().keySetIterator();
while (e.hasNext()) {
Annotation a= e.next();
Position p= annotations.get(a);
removePosition(fDocument, p);
event.annotationRemoved(a, p);
}
} else {
// foreach is synchronized on map lock object
annotations.forEach((a, p) -> {

This comment has been minimized.

Copy link
@szarnekow

szarnekow Aug 1, 2023

Contributor

I think this iteration must be under same lock as the getAnnotationModelEvent invocation. Otherwise a thread in between might already have submitted the event (theoretically).

This comment has been minimized.

Copy link
@iloveeclipse

iloveeclipse Aug 1, 2023

Author Member

Hmm, right.

removePosition(fDocument, p);
event.annotationRemoved(a, p);
});
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*******************************************************************************
* Copyright (c) 2000, 2015 IBM Corporation and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
* which accompanies this distribution, and is available at
* https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* IBM Corporation - initial API and implementation
*******************************************************************************/
package org.eclipse.jface.text.source;


import java.util.function.BiConsumer;

import org.eclipse.jface.text.Position;


/**
* An {@link IAnnotationMap} that synchronizes {@link #forEach(BiConsumer)} on the
* {@link #getLockObject()}.
*
* @since 3.14
*/
public interface IAnnotationMap2 extends IAnnotationMap {

/**
* Overridden to synchronize iteration on {@link #getLockObject()}
* <p>
* {@inheritDoc}
*/
@Override
default void forEach(BiConsumer<? super Annotation, ? super Position> action) {
Object lockObject = getLockObject();
if (lockObject != null) {
synchronized (lockObject) {
IAnnotationMap.super.forEach(action);
}
} else {
IAnnotationMap.super.forEach(action);
}
}
}

0 comments on commit 907bfa9

Please sign in to comment.