From 907bfa9f9d77521f8fa771d36006df84b0ce7f74 Mon Sep 17 00:00:00 2001 From: Andrey Loskutov Date: Tue, 1 Aug 2023 10:57:16 +0200 Subject: [PATCH] AnnotationModel uses inefficient iteration over map, take 2 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 https://github.com/eclipse-platform/eclipse.platform.ui/issues/892 --- bundles/org.eclipse.text/META-INF/MANIFEST.MF | 2 +- .../jface/text/source/AnnotationMap.java | 2 +- .../jface/text/source/AnnotationModel.java | 47 +++++++++++++------ .../jface/text/source/IAnnotationMap2.java | 46 ++++++++++++++++++ 4 files changed, 81 insertions(+), 16 deletions(-) create mode 100644 bundles/org.eclipse.text/src/org/eclipse/jface/text/source/IAnnotationMap2.java diff --git a/bundles/org.eclipse.text/META-INF/MANIFEST.MF b/bundles/org.eclipse.text/META-INF/MANIFEST.MF index 10ef61bde05..17a02b157f9 100644 --- a/bundles/org.eclipse.text/META-INF/MANIFEST.MF +++ b/bundles/org.eclipse.text/META-INF/MANIFEST.MF @@ -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: diff --git a/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationMap.java b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationMap.java index 97312b0a2b3..e4b054388b9 100644 --- a/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationMap.java +++ b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationMap.java @@ -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 diff --git a/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java index 4d24a5f14cf..9b50919d1c8 100644 --- a/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java +++ b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java @@ -640,13 +640,23 @@ private void cleanup(boolean fireModelChanged, boolean forkNotification) { fDocumentChanged= false; ArrayList deleted= new ArrayList<>(); - Iterator 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 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) { @@ -789,15 +799,24 @@ public void removeAllAnnotations() { protected void removeAllAnnotations(boolean fireModelChanged) { IAnnotationMap annotations= getAnnotationMap(); if (fDocument != null) { - Iterator 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 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) -> { + removePosition(fDocument, p); + event.annotationRemoved(a, p); + }); } } diff --git a/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/IAnnotationMap2.java b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/IAnnotationMap2.java new file mode 100644 index 00000000000..3e7d6e1423e --- /dev/null +++ b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/IAnnotationMap2.java @@ -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()} + *

+ * {@inheritDoc} + */ + @Override + default void forEach(BiConsumer action) { + Object lockObject = getLockObject(); + if (lockObject != null) { + synchronized (lockObject) { + IAnnotationMap.super.forEach(action); + } + } else { + IAnnotationMap.super.forEach(action); + } + } +}