Skip to content

Commit

Permalink
[6] Revert notification dispatch performance optimization
Browse files Browse the repository at this point in the history
This reverts commit bc7f02f and
05b8566 from bugzilla
https://eclip.se/128445. This implemented a low-level performance
optimization in notification dispatch by reused the same Java objects.

It caused invalid behaviors in multi-threaded contexts (see bugzill
288442) and while it may have been relevant to minimize object
allocations in 2006 it's probably not worth on modern JVMs and CPUs.

Bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=128445
Bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=288442
Bug: #6
  • Loading branch information
eobrienPilz authored and pcdavid committed Sep 16, 2024
1 parent d07d687 commit 5707e5f
Show file tree
Hide file tree
Showing 6 changed files with 194 additions and 66 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/**
* Copyright (c) 2005, 2007 IBM Corporation and others.
* Copyright (c) 2005, 2024 IBM Corporation and others.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,24 +98,18 @@ public class TransactionalEditingDomainImpl
new java.util.ArrayList<ResourceSetListener>();
private final List<ResourceSetListener> aggregatePrecommitListeners =
new java.util.ArrayList<ResourceSetListener>();
private final List<ResourceSetListener> postcommitListeners =
new java.util.ArrayList<ResourceSetListener>();

// reusable notification list and event for unbatched change events
private final List<Notification> unbatchedNotifications =
new java.util.ArrayList<Notification>(1);
private final ResourceSetChangeEvent unbatchedChangeEvent =
new ResourceSetChangeEvent(this, null, unbatchedNotifications);

private final List<ResourceSetListener> postcommitListeners =
new java.util.ArrayList<ResourceSetListener>();

// this is editable by clients for backwards compatibility with 1.1
private final Map<Object, Object> undoRedoOptions = new java.util.HashMap<Object, Object>(
TransactionImpl.DEFAULT_UNDO_REDO_OPTIONS);

private LifecycleImpl lifecycle;
private Transaction.OptionMetadata.Registry optionMetadata;

private boolean disposed = false;
private boolean disposed = false;

/**
* Initializes me with my adapter factory, command stack, and resource set.
*
Expand Down Expand Up @@ -806,20 +800,24 @@ public void run() {
// Documentation copied from the inherited specification
public void broadcastUnbatched(Notification notification) {
final ResourceSetListener[] listeners = getPostcommitListeners();
unbatchedNotifications.add(notification);

final List<Notification> notifications = Collections.singletonList(notification);

try {
runExclusive(new Runnable() {
public void run() {
for (ResourceSetListener element : listeners) {
try {
List<Notification> filtered = FilterManager.getInstance().selectUnbatched(
unbatchedNotifications,
notifications,
element.getFilter());

if (!filtered.isEmpty()) {
element.resourceSetChanged(unbatchedChangeEvent);
element.resourceSetChanged(
new ResourceSetChangeEvent(
TransactionalEditingDomainImpl.this,
null,
filtered));
}
} catch (Exception e) {
Tracing.catching(TransactionalEditingDomainImpl.class, "broadcastUnbatched", e); //$NON-NLS-1$
Expand All @@ -842,9 +840,6 @@ public void run() {
Messages.postcommitInterrupted,
e);
EMFTransactionPlugin.INSTANCE.log(status);
} finally {
// remove the unbatched notification from our reusable cache
unbatchedNotifications.remove(0);
}
}

Expand Down
3 changes: 2 additions & 1 deletion tests/org.eclipse.emf.transaction.tests/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ Require-Bundle: org.eclipse.emf.transaction;bundle-version="1.9.0",
org.eclipse.emf.examples.library.edit;bundle-version="[2.3.0,3.0.0)",
org.junit;bundle-version="[4.0.0,5.0.0)",
org.eclipse.ui;bundle-version="[3.2.0,4.0.0)",
org.eclipse.core.filesystem;bundle-version="[1.2.0,2.0.0)"
org.eclipse.core.filesystem;bundle-version="[1.2.0,2.0.0)",
org.eclipse.emf.workspace;bundle-version="1.5.2"
Eclipse-LazyStart: true; exceptions="org.eclipse.emf.transaction.tests.constraints"
Bundle-ActivationPolicy: lazy
Export-Package: org.eclipse.emf.transaction.multithread.tests,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public static Test suite() {
suite.addTest(ReadOperationTest.suite());
suite.addTest(WriteOperationTest.suite());
suite.addTest(ReadWriteOperationTest.suite());
suite.addTest(EMFTransansactionTest.suite());

return suite;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
/**
* Copyright (c) 2009, 2024 Obeo
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Mariot Chauvin <[email protected]> - initial API and implementation
*/
package org.eclipse.emf.transaction.multithread.tests;

import java.io.File;
import java.lang.Thread.UncaughtExceptionHandler;
import java.util.ArrayList;
import java.util.Collection;
import java.util.concurrent.atomic.AtomicBoolean;

import org.eclipse.core.commands.operations.DefaultOperationHistory;
import org.eclipse.core.commands.operations.IOperationHistory;
import org.eclipse.core.runtime.ILogListener;
import org.eclipse.core.runtime.IStatus;
import org.eclipse.emf.common.notify.Notification;
import org.eclipse.emf.common.util.URI;
import org.eclipse.emf.ecore.EClass;
import org.eclipse.emf.ecore.EPackage;
import org.eclipse.emf.ecore.EReference;
import org.eclipse.emf.ecore.EcoreFactory;
import org.eclipse.emf.ecore.EcorePackage;
import org.eclipse.emf.ecore.InternalEObject;
import org.eclipse.emf.ecore.impl.ENotificationImpl;
import org.eclipse.emf.ecore.resource.Resource;
import org.eclipse.emf.ecore.resource.ResourceSet;
import org.eclipse.emf.ecore.resource.impl.ResourceSetImpl;
import org.eclipse.emf.transaction.RecordingCommand;
import org.eclipse.emf.transaction.TransactionalEditingDomain;
import org.eclipse.emf.transaction.internal.EMFTransactionPlugin;
import org.eclipse.emf.workspace.WorkspaceEditingDomainFactory;

import junit.framework.Test;
import junit.framework.TestCase;
import junit.framework.TestSuite;

/**
* This class contains a test case for EMF transaction.
*
* @author mchauvin
*/
public class EMFTransansactionTest extends TestCase {

private TransactionalEditingDomain editingDomain;

private UncaughtExceptionHandler exceptionHandler;

private ILogListener listener;

private AtomicBoolean errorDetected = new AtomicBoolean();

public static Test suite() {
return new TestSuite(EMFTransansactionTest.class, "Concurrent Transaction Tests"); //$NON-NLS-1$
}

@Override
protected void setUp() throws Exception {
super.setUp();
exceptionHandler = new UncaughtExceptionHandler() {
public void uncaughtException(Thread t, Throwable e) {
errorDetected.set(true);
}
};
}

/**
* Due to threads synchronization complexity, this test may succeed. It should
* be run several times (at least 4) to be sure than the bug is not present.
*
* @throws Exception
*/
public void testSynchronizationBug() throws Exception {
/* create a resource set */
final ResourceSet rset = new ResourceSetImpl();
/* create an editing domain */
editingDomain = createEditingDomain(rset);

/* initialize the first model */
final EPackage ePackage = EcoreFactory.eINSTANCE.createEPackage();
final EClass eClass = EcoreFactory.eINSTANCE.createEClass();
ePackage.getEClassifiers().add(eClass);
final EReference eReference = EcoreFactory.eINSTANCE.createEReference();
eClass.getEReferences().add(eReference);

/* create resource and and add it initialized model */
final URI fileUri = URI.createFileURI(new File("test.ecore").getAbsolutePath());
final Resource rs = rset.createResource(fileUri);
editingDomain.getCommandStack().execute(new RecordingCommand(editingDomain) {
@Override
protected void doExecute() {
rs.getContents().add(ePackage);
}
});

/* initialize the second model */
final EPackage ePackage2 = EcoreFactory.eINSTANCE.createEPackage();
final EClass eClass2 = EcoreFactory.eINSTANCE.createEClass();
ePackage2.getEClassifiers().add(eClass2);
final EReference eReference2 = EcoreFactory.eINSTANCE.createEReference();
eClass2.getEReferences().add(eReference2);

/* create resource and and add it initialized model */
final URI file2Uri = URI.createFileURI(new File("test2.ecore").getAbsolutePath());
final Resource rs2 = rset.createResource(file2Uri);
editingDomain.getCommandStack().execute(new RecordingCommand(editingDomain) {
@Override
protected void doExecute() {
rs2.getContents().add(ePackage2);
}
});

/* set a link between first and second class */
editingDomain.getCommandStack().execute(new RecordingCommand(editingDomain) {
@Override
protected void doExecute() {
eReference.setEOpposite(eReference2);
}
});

Thread.setDefaultUncaughtExceptionHandler(exceptionHandler);

listener = (IStatus status, String plugin) -> {
if (status.getSeverity() == IStatus.ERROR) {
errorDetected.set(true);
}
};

EMFTransactionPlugin.getPlugin().getLog().addLogListener(listener);

final Collection<Thread> threads = new ArrayList<Thread>();
/* Launch a unload and a resolution */
for (int i = 0; i < 100; i++) {
threads.add(launchNotificationInANewThread(eReference, eReference2));
}

for (final Thread thread : threads) {
thread.join();
}

EMFTransactionPlugin.getPlugin().getLog().removeLogListener(listener);

/* an exception occurs in another thread */
if (errorDetected.get()) {
fail();
}
}

private TransactionalEditingDomain createEditingDomain(ResourceSet rset) {
IOperationHistory history = new DefaultOperationHistory();
return WorkspaceEditingDomainFactory.INSTANCE.createEditingDomain(rset, history);
}

private Thread launchNotificationInANewThread(final EReference ref1, final EReference ref2) throws Exception {
final Thread t = new Thread(() -> {
for (int i = 0; i < 200; i++) {
ref1.eNotify(new ENotificationImpl((InternalEObject) ref1, Notification.RESOLVE,
EcorePackage.EREFERENCE__EOPPOSITE, null, ref2));
}
});
t.start();
return t;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,12 @@
* Contributors:
* IBM - Initial API and implementation
* Zeligsoft - Bug 177642
* $Id: ResourceSetListenersTest.java,v 1.4 2006/02/22 22:13:57 cdamus Exp $
*/
package org.eclipse.emf.transaction.tests;

import java.util.Collections;
import java.util.List;

import junit.framework.Test;
import junit.framework.TestSuite;

import org.eclipse.emf.common.command.Command;
import org.eclipse.emf.common.notify.Notification;
import org.eclipse.emf.common.util.URI;
Expand All @@ -41,6 +38,9 @@
import org.eclipse.emf.transaction.tests.fixtures.TestCommand;
import org.eclipse.emf.transaction.tests.fixtures.TestListener;

import junit.framework.Test;
import junit.framework.TestSuite;


/**
* Tests resource listener events.
Expand Down Expand Up @@ -679,47 +679,6 @@ public void test_unbatchedNotifications() {
fail(e);
}
}

/**
* Tests that events are correctly reused for unbatched notifications.
*/
public void test_unbatchedNotifications_reuseEvents_128445() {
try {
class Listener extends ResourceSetListenerImpl {
ResourceSetChangeEvent lastEvent = null;
List<Notification> lastNotifications = null;
int count = 0;

@Override
public void resourceSetChanged(ResourceSetChangeEvent event) {
count++;

if (lastEvent == null) {
lastEvent = event;
lastNotifications = event.getNotifications();
} else {
assertSame(lastEvent, event);
assertSame(lastNotifications, event.getNotifications());
assertEquals(1, lastNotifications.size());
}
}
}

testResource.unload();

Listener localListener = new Listener();

domain.addResourceSetListener(localListener);

// cause a bunch of unbatched events
testResource.load(Collections.EMPTY_MAP);

// check that there was actually an opportunity to reuse the event
assertTrue(localListener.count > 1);
} catch (Exception e) {
fail(e);
}
}

/**
* Tests that notifications resulting from reads performed by post-commit
Expand Down

0 comments on commit 5707e5f

Please sign in to comment.