From f4dce23411b9944930efc74e7c2a23ec1a6c4697 Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Tue, 1 Jul 2014 15:33:08 -0500 Subject: [PATCH 1/7] ObjectIndexTest: improve getAll() test coverage Specifically, we test that lazy object resolution works in conjunction with a call to getAll(), to ensure that pending objects get resolved. --- .../org/scijava/object/ObjectIndexTest.java | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/scijava/object/ObjectIndexTest.java b/src/test/java/org/scijava/object/ObjectIndexTest.java index 06d44487f..10c17e715 100644 --- a/src/test/java/org/scijava/object/ObjectIndexTest.java +++ b/src/test/java/org/scijava/object/ObjectIndexTest.java @@ -38,6 +38,8 @@ import static org.junit.Assert.assertTrue; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; import java.util.Iterator; import java.util.List; @@ -60,11 +62,25 @@ public void testGetAll() { objectIndex.add(o1); objectIndex.add(o2); objectIndex.add(o3); + + final String o4 = "quick", o5 = "brown", o6 = "fox"; + objectIndex.addLater(new LazyObjects() { + + @Override + public Collection get() { + return Arrays.asList(o4, o5, o6); + } + + }); + final List all = objectIndex.getAll(); - assertEquals(3, all.size()); + assertEquals(6, all.size()); assertSame(o1, all.get(0)); assertSame(o2, all.get(1)); assertSame(o3, all.get(2)); + assertSame(o4, all.get(3)); + assertSame(o5, all.get(4)); + assertSame(o6, all.get(5)); } @Test From 5ef3f68b2105ac29ef20bf68e649fe5b0c08f4f1 Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Tue, 1 Jul 2014 16:39:24 -0500 Subject: [PATCH 2/7] AbstractSingletonService: use more specific type The LazyObjects instance will definitely return a collection of objects of type PT, not just Object. This distinction is important now that the ObjectIndex is more selective about which pending objects it resolves. --- .../java/org/scijava/plugin/AbstractSingletonService.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/scijava/plugin/AbstractSingletonService.java b/src/main/java/org/scijava/plugin/AbstractSingletonService.java index 83852fa8a..79eeda24c 100644 --- a/src/main/java/org/scijava/plugin/AbstractSingletonService.java +++ b/src/main/java/org/scijava/plugin/AbstractSingletonService.java @@ -85,11 +85,11 @@ public

P getInstance(final Class

pluginClass) { @Override public void initialize() { // add singleton instances to the object index... IN THE FUTURE! - objectService.getIndex().addLater(new LazyObjects() { + objectService.getIndex().addLater(new LazyObjects() { @Override - public ArrayList get() { - return new ArrayList(getInstances()); + public ArrayList get() { + return new ArrayList(getInstances()); } }); } From 8bbb47a56d0003f04b6b021531356886769c9526 Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Sat, 5 Jul 2014 21:51:28 -0500 Subject: [PATCH 3/7] LazyObjects: add getType() method This breaks backwards compatibility, so bumps the development snapshot version to 3.0.0. The rationale is to make it possible for the ObjectIndex to better discriminate which objects should be resolved for a particular call to ObjectIndex#get(Class). The current behavior is to resolve _all_ pending objects with _any_ call to get(Class), which is generally suboptimal. --- pom.xml | 2 +- src/main/java/org/scijava/object/LazyObjects.java | 7 +++++++ .../java/org/scijava/plugin/AbstractSingletonService.java | 6 ++++++ src/main/java/org/scijava/script/DefaultScriptService.java | 5 +++++ src/test/java/org/scijava/object/ObjectIndexTest.java | 5 +++++ 5 files changed, 24 insertions(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 9868b80ee..35e36da1e 100644 --- a/pom.xml +++ b/pom.xml @@ -10,7 +10,7 @@ scijava-common - 2.41.1-SNAPSHOT + 3.0.0-SNAPSHOT SciJava Common SciJava Common is a shared library for SciJava software. It provides a plugin framework, with an extensible mechanism for service discovery, backed by its own annotation processor, so that plugins can be loaded dynamically. It is used by both ImageJ and SCIFIO. diff --git a/src/main/java/org/scijava/object/LazyObjects.java b/src/main/java/org/scijava/object/LazyObjects.java index 975efa89b..bd2e2b2d5 100644 --- a/src/main/java/org/scijava/object/LazyObjects.java +++ b/src/main/java/org/scijava/object/LazyObjects.java @@ -45,4 +45,11 @@ public interface LazyObjects { /** Gets the collection of objects. */ Collection get(); + /** + * The type of the objects which will be resolved from a call to + * {@link #get()}. This information is used to determine whether to resolve + * the objects in response to an {@link ObjectIndex#get(Class)} call. + */ + Class getType(); + } diff --git a/src/main/java/org/scijava/plugin/AbstractSingletonService.java b/src/main/java/org/scijava/plugin/AbstractSingletonService.java index 79eeda24c..e6f4c3133 100644 --- a/src/main/java/org/scijava/plugin/AbstractSingletonService.java +++ b/src/main/java/org/scijava/plugin/AbstractSingletonService.java @@ -91,6 +91,12 @@ public void initialize() { public ArrayList get() { return new ArrayList(getInstances()); } + + @Override + public Class getType() { + return getPluginType(); + } + }); } diff --git a/src/main/java/org/scijava/script/DefaultScriptService.java b/src/main/java/org/scijava/script/DefaultScriptService.java index 646feb111..9388de2cb 100644 --- a/src/main/java/org/scijava/script/DefaultScriptService.java +++ b/src/main/java/org/scijava/script/DefaultScriptService.java @@ -285,6 +285,11 @@ public Collection get() { return scripts().values(); } + @Override + public Class getType() { + return ScriptInfo.class; + } + }); } diff --git a/src/test/java/org/scijava/object/ObjectIndexTest.java b/src/test/java/org/scijava/object/ObjectIndexTest.java index 10c17e715..bf9b993ac 100644 --- a/src/test/java/org/scijava/object/ObjectIndexTest.java +++ b/src/test/java/org/scijava/object/ObjectIndexTest.java @@ -71,6 +71,11 @@ public Collection get() { return Arrays.asList(o4, o5, o6); } + @Override + public Class getType() { + return String.class; + } + }); final List all = objectIndex.getAll(); From 29ff80b02b4d51803bb397c928caf792324ea878 Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Tue, 1 Jul 2014 15:22:57 -0500 Subject: [PATCH 4/7] ObjectIndex: only resolve relevant pending objects If a pending object will not be of a type compatible with the requested type, then skip resolution of that pending object. This improves performance of the get(Class) and getAll() methods in cases where there are a large number of pending objects in the index. It also helps to eliminate difficulties like those in issue #90. --- .../java/org/scijava/object/ObjectIndex.java | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/scijava/object/ObjectIndex.java b/src/main/java/org/scijava/object/ObjectIndex.java index 1baee99ff..2d18b682e 100644 --- a/src/main/java/org/scijava/object/ObjectIndex.java +++ b/src/main/java/org/scijava/object/ObjectIndex.java @@ -129,7 +129,7 @@ public List getAll() { */ public List get(final Class type) { // lazily register any pending objects - if (!pending.isEmpty()) resolvePending(); + if (!pending.isEmpty()) resolvePending(type); List list = retrieveList(type); // NB: Return a copy of the data, to facilitate thread safety. @@ -380,15 +380,32 @@ protected List retrieveList(final Class type) { return list; } - private void resolvePending() { + private void resolvePending(final Class type) { synchronized (pending) { - while (!pending.isEmpty()) { - final LazyObjects c = pending.remove(0); + for (int p = pending.size() - 1; p >= 0; p--) { + final LazyObjects c = pending.get(p); + + // NB: If this pending callback returns objects of a different + // type than the one we are interested in, it can be skipped. + if (!isCompatibleType(c, type)) continue; + + // trigger the callback and add the results + pending.remove(p); addAll(c.get()); } } } + // -- Helper methods -- + + private boolean isCompatibleType(final LazyObjects c, + final Class type) + { + if (type == All.class) return true; + final Class cType = c.getType(); + return cType.isAssignableFrom(type) || type.isAssignableFrom(cType); + } + // -- Helper classes -- private static class All { From 843f863226f76fe4ec5c53eefa16bf2643bc657b Mon Sep 17 00:00:00 2001 From: Curtis Rueden Date: Tue, 1 Jul 2014 15:33:17 -0500 Subject: [PATCH 5/7] ObjectIndexTest: add a lazy object resolution test This tests that the addLater method works in conjunction with calls to get(Class). The logic is nontrivial because we only want to resolve pending objects when the types are compatible with the requested one. --- .../org/scijava/object/ObjectIndexTest.java | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/src/test/java/org/scijava/object/ObjectIndexTest.java b/src/test/java/org/scijava/object/ObjectIndexTest.java index bf9b993ac..666b49f65 100644 --- a/src/test/java/org/scijava/object/ObjectIndexTest.java +++ b/src/test/java/org/scijava/object/ObjectIndexTest.java @@ -37,6 +37,7 @@ import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import java.math.BigInteger; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -243,4 +244,98 @@ public void testToString() { assertArrayEquals(expected, actual); } + @Test + public void testAddLater() { + final ObjectIndex objectIndex = + new ObjectIndex(Object.class); + objectIndex.add(new Integer(5)); + objectIndex.add(new Float(2.5f)); + objectIndex.add(new Integer(3)); + + final LazyThings lazyIntegers = new LazyThings(9, -7); + objectIndex.addLater(lazyIntegers); + + final LazyThings lazyFloats = + new LazyThings(6.6f, -3.3f, -5.1f, 12.3f); + objectIndex.addLater(lazyFloats); + + final LazyThings lazyBigIntegers = + new LazyThings(BigInteger.ONE, BigInteger.TEN); + objectIndex.addLater(lazyBigIntegers); + + // verify that no pending objects have been resolved yet + assertFalse(lazyIntegers.wasAccessed()); + assertFalse(lazyFloats.wasAccessed()); + assertFalse(lazyBigIntegers.wasAccessed()); + + // verify list of Integers; this will resolve the pending ones + final List integerObjects = objectIndex.get(Integer.class); + assertEquals(4, integerObjects.size()); + assertEquals(5, integerObjects.get(0)); + assertEquals(3, integerObjects.get(1)); + assertEquals(9, integerObjects.get(2)); + assertEquals(-7, integerObjects.get(3)); + + // verify that pending Integers have now been resolved + assertTrue(lazyIntegers.wasAccessed()); + + // verify that the other pending objects have still not been resolved + assertFalse(lazyFloats.wasAccessed()); + assertFalse(lazyBigIntegers.wasAccessed()); + + // verify list of Floats; this will resolve the pending ones + final List floatObjects = objectIndex.get(Float.class); + assertEquals(5, floatObjects.size()); + assertEquals(2.5f, floatObjects.get(0)); + assertEquals(6.6f, floatObjects.get(1)); + assertEquals(-3.3f, floatObjects.get(2)); + assertEquals(-5.1f, floatObjects.get(3)); + assertEquals(12.3f, floatObjects.get(4)); + + // verify that pending Floats have now been resolved + assertTrue(lazyFloats.wasAccessed()); + + // verify that pending BigIntegers have still not been resolved + assertFalse(lazyBigIntegers.wasAccessed()); + + // verify list of BigIntegers; this will resolve the pending ones + final List bigIntegerObjects = objectIndex.get(BigInteger.class); + assertEquals(2, bigIntegerObjects.size()); + assertEquals(BigInteger.ONE, bigIntegerObjects.get(0)); + assertEquals(BigInteger.TEN, bigIntegerObjects.get(1)); + + // verify that pending BigIntegers have finally been resolved + assertTrue(lazyBigIntegers.wasAccessed()); + } + + // -- Helper classes -- + + public static class LazyThings implements LazyObjects { + + private Collection objects; + private Class type; + private boolean accessed; + + public LazyThings(T... objects) { + this.objects = Arrays.asList(objects); + this.type = objects[0].getClass(); + } + + @Override + public Collection get() { + accessed = true; + return objects; + } + + @Override + public Class getType() { + return type; + } + + public boolean wasAccessed() { + return accessed; + } + + } + } From 05b15376bb9a38fe813092390dae834326ec0d53 Mon Sep 17 00:00:00 2001 From: Mark Hiner Date: Thu, 31 Jul 2014 11:12:23 -0500 Subject: [PATCH 6/7] AbstractSingletonService: fix helper method The filterInstances method should not alter the return type of the input list. The purpose of that method is to potentially remove some elements from the input list -- nothing more -- so it should still return a List, not something else. --- .../java/org/scijava/platform/DefaultPlatformService.java | 2 +- .../java/org/scijava/plugin/AbstractSingletonService.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/scijava/platform/DefaultPlatformService.java b/src/main/java/org/scijava/platform/DefaultPlatformService.java index d399caad3..19887566d 100644 --- a/src/main/java/org/scijava/platform/DefaultPlatformService.java +++ b/src/main/java/org/scijava/platform/DefaultPlatformService.java @@ -134,7 +134,7 @@ public boolean registerAppMenus(final Object menus) { } @Override - public List filterInstances(final List list) { + public List filterInstances(final List list) { final Iterator iter = list.iterator(); while (iter.hasNext()) { if (!iter.next().isTarget()) { diff --git a/src/main/java/org/scijava/plugin/AbstractSingletonService.java b/src/main/java/org/scijava/plugin/AbstractSingletonService.java index e6f4c3133..7d4eb8dbb 100644 --- a/src/main/java/org/scijava/plugin/AbstractSingletonService.java +++ b/src/main/java/org/scijava/plugin/AbstractSingletonService.java @@ -88,7 +88,7 @@ public void initialize() { objectService.getIndex().addLater(new LazyObjects() { @Override - public ArrayList get() { + public List get() { return new ArrayList(getInstances()); } @@ -108,7 +108,7 @@ public Class getType() { * @param list the initial list of instances * @return the filtered list of instances */ - protected List filterInstances(final List list) { + protected List filterInstances(final List list) { return list; } From d6a7eba0005796f0a26ea75fff4e0f6018f1d7da Mon Sep 17 00:00:00 2001 From: Mark Hiner Date: Thu, 31 Jul 2014 11:12:23 -0500 Subject: [PATCH 7/7] Lean on object service for singleton retrieval Instead of maintaining our own list of singletons, we should be using the ObjectService. --- .../plugin/AbstractSingletonService.java | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/scijava/plugin/AbstractSingletonService.java b/src/main/java/org/scijava/plugin/AbstractSingletonService.java index 7d4eb8dbb..88b5262e2 100644 --- a/src/main/java/org/scijava/plugin/AbstractSingletonService.java +++ b/src/main/java/org/scijava/plugin/AbstractSingletonService.java @@ -31,7 +31,6 @@ package org.scijava.plugin; -import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -58,10 +57,7 @@ public abstract class AbstractSingletonService private ObjectService objectService; // TODO: Listen for PluginsAddedEvent and PluginsRemovedEvent - // and update the list of singletons accordingly. - - /** List of singleton plugin instances. */ - private List instances; + // and update the map of singletons accordingly. private Map, PT> instanceMap; @@ -69,8 +65,8 @@ public abstract class AbstractSingletonService @Override public List getInstances() { - if (instances == null) initInstances(); - return instances; + final List plugins = objectService.getObjects(getPluginType()); + return Collections.unmodifiableList(plugins); } @SuppressWarnings("unchecked") @@ -89,7 +85,7 @@ public void initialize() { @Override public List get() { - return new ArrayList(getInstances()); + return createInstances(); } @Override @@ -115,15 +111,12 @@ protected List filterInstances(final List list) { // -- Helper methods -- private synchronized void initInstances() { - if (instances != null) return; - - final List list = - Collections.unmodifiableList(filterInstances(getPluginService() - .createInstancesOfType(getPluginType()))); + if (instanceMap != null) return; final HashMap, PT> map = new HashMap, PT>(); + final List list = getInstances(); for (final PT plugin : list) { @SuppressWarnings("unchecked") final Class ptClass = @@ -131,11 +124,17 @@ private synchronized void initInstances() { map.put(ptClass, plugin); } - log.debug("Found " + list.size() + " " + getPluginType().getSimpleName() + - " plugins."); - instanceMap = map; - instances = list; + } + + private List createInstances() { + final List instances = + filterInstances(getPluginService().createInstancesOfType(getPluginType())); + + log.info("Found " + instances.size() + " " + + getPluginType().getSimpleName() + " plugins."); + + return instances; } }