Add ScopeFactory API #136

Leland-Takamine · 2019-10-04T18:58:21Z

Resolves #125

andreasnomikos

Leaving open for pending questions

andreasnomikos · 2019-10-05T15:37:52Z

lib/src/main/java/motif/ScopeFactory.java

+
+    private ScopeFactory() {}
+
+    private static final Map<Class<?>, Constructor<?>> scopeClassToCreateMethod = new HashMap<>();


this caching layer might be redundant post android 7.0 where the reflection apis where optimized with native invocations. At that point the methods are already cached in the native representation of the class Object so the vm maintains its own cache for them and we don't need to tax the heap with it.
Do you want to spend time to look into that and decide if this needs to be api gated ?

Interesting - didn't know about this. Do you remember where you read this or the relevant source code?

http://androidxref.com/9.0.0_r3/xref/art/runtime/native/java_lang_Class.cc

I guess we will still incur the overhead of the result array
So the tradeoff of the heap cache is that we are taxing the heap with
the map vs allocating a new array every time we need to create a root scope
either to get the result of getDeclaredConstructors or to pass params to getDeclaredConstructor

Got it - in this case I'll move forward with the current approach since it's the simplest and safest.

andreasnomikos · 2019-10-05T15:41:16Z

lib/src/main/java/motif/ScopeFactory.java

+        if (constructor == null) {
+            Class<?> scopeImplClass = getScopeImplClass(scopeClass);
+            constructor = scopeImplClass.getDeclaredConstructors()[0];
+            scopeClassToCreateMethod.put(scopeClass, constructor);


this second field access is redundant just hoist it to a local var at the top

Not sure what you mean here.

scopeClass is a field reference so its doing a double field read

I'd rather tackle this as a post processing step at the binary level if we're concerned about field access performance.

lib/src/main/java/motif/ScopeFactory.java

andreasnomikos · 2019-10-05T15:45:53Z

lib/src/main/java/motif/ScopeFactory.java

+    private static Class<?> getScopeImplClass(Class<?> scopeClass) {
+        String scopeImplClassName = getScopeImplClassName(scopeClass);
+        try {
+            return Class.forName(scopeImplClassName, false, scopeClass.getClassLoader());


is there a reason that you avoid the initialization here?
If it hasn't been done already It will probably have to do it anyway immediately on the new instance
unless you are considering warming the cache separately for some reason.

Which initialization are you referring to here?

the class initialization controlled by the false parameter

Gotcha - will update

lib/src/main/java/motif/ScopeFactory.java

andreasnomikos · 2019-10-05T15:50:06Z

lib/src/main/java/motif/ScopeFactory.java

+
+    // Inspired by https://github.com/square/javapoet/issues/295
+    private static String getScopeImplClassName(Class<?> scopeClass) {
+        StringBuilder sb = new StringBuilder();


smarter init value than 16 chars ?

What would be a better initial value?

tests/src/main/java/testcases/T068_scope_factory/config.pro

Leland-Takamine · 2019-12-03T07:20:53Z

lib/src/main/java/motif/ScopeFactory.java

+
+    private static final Map<Class<?>, Constructor<?>> scopeClassToCreateMethod = new HashMap<>();
+
+    public static <S extends Creatable<NoDependencies>> S create(Class<S> scopeClass) {


@andreasnomikos @ugonna @Ericliu001 Heads up that I've added this convenience API for Scopes that don't have any dependencies. Let me know if you have any concerns with this.

ugonna · 2019-12-03T20:59:39Z

lib/src/main/java/motif/ScopeFactory.java

+            } else {
+                return (S) constructor.newInstance(dependencies);
+            }
+        } catch (IllegalAccessException e) {


Since you're handling them all the same, do you not want to:

try { } catch (IllegalAccessException | InvocationTargetException | InstantiationException e) {}

This would implicitly bump the min-sdk to 19:

While it may make sense to only support SDK 19 and up, I'd rather make that a conscious decision rather than forcing that limitation to optimize just this piece of code.

ugonna · 2019-12-03T21:02:57Z

lib/src/main/java/motif/ScopeFactory.java

+    private static final Map<Class<?>, Constructor<?>> scopeClassToCreateMethod = new HashMap<>();
+
+    public static <S extends Creatable<NoDependencies>> S create(Class<S> scopeClass) {
+        return create(scopeClass, NO_DEPENDENCIES);


And if someone sends in a Creatable with a non-zero arg constructor to this method? We would be passing NO_DEPENDENCIES to that constructor. That's legal, but incorrect usage. Do we want to warn or fail against that case?

The class that we end up instantiating is the ScopeImpl associated with the Scope class that is passed in. If someone passes in a Creatable that is not a Scope then we will fail to find the ScopeImpl and crash at runtime. We could add an explicit error if the passed class is not annotated with @Scope but that adds some runtime overhead. Might be worth it - we can discuss this as a potential follow up.

Leland-Takamine requested review from kurtisnelson, tyvsmith, ugonna, andreasnomikos, Ericliu001 and oliviernotteghem October 4, 2019 18:58

Leland-Takamine force-pushed the scope-factory branch 2 times, most recently from aa8933d to a455430 Compare October 4, 2019 19:50

andreasnomikos reviewed Oct 5, 2019

View reviewed changes

Leland-Takamine force-pushed the creatable branch from d27718c to 90f6bf3 Compare October 7, 2019 19:34

Leland-Takamine force-pushed the scope-factory branch from a455430 to ba23e63 Compare October 7, 2019 19:47

andreasnomikos approved these changes Oct 9, 2019

View reviewed changes

Leland-Takamine force-pushed the scope-factory branch from ba23e63 to 9c15c79 Compare October 9, 2019 21:46

Ericliu001 approved these changes Oct 18, 2019

View reviewed changes

Leland-Takamine force-pushed the scope-factory branch from 9c15c79 to f4f6add Compare December 2, 2019 21:08

Leland-Takamine force-pushed the creatable branch 2 times, most recently from 6cd2d97 to e00e899 Compare December 2, 2019 21:32

Leland-Takamine force-pushed the scope-factory branch from f4f6add to 25004d6 Compare December 2, 2019 21:33

Leland-Takamine force-pushed the creatable branch from e00e899 to dca40c2 Compare December 2, 2019 21:53

Leland-Takamine force-pushed the scope-factory branch from 25004d6 to a208b19 Compare December 2, 2019 21:54

Leland-Takamine force-pushed the creatable branch from dca40c2 to c6e8526 Compare December 3, 2019 03:10

Leland-Takamine force-pushed the scope-factory branch from a208b19 to 00ba8f8 Compare December 3, 2019 03:10

Leland-Takamine force-pushed the creatable branch from c6e8526 to 423d3b5 Compare December 3, 2019 03:29

Leland-Takamine force-pushed the scope-factory branch 2 times, most recently from d921921 to 21343e5 Compare December 3, 2019 05:47

Leland-Takamine force-pushed the creatable branch 2 times, most recently from 6485ab2 to 8de6db6 Compare December 3, 2019 07:05

Leland-Takamine force-pushed the scope-factory branch 2 times, most recently from 9c24149 to 9fd42d8 Compare December 3, 2019 07:19

Leland-Takamine commented Dec 3, 2019

View reviewed changes

ugonna approved these changes Dec 3, 2019

View reviewed changes

Leland-Takamine changed the base branch from creatable to master December 3, 2019 23:41

Add ScopeFactory API

76780a4

Leland-Takamine force-pushed the scope-factory branch from 9fd42d8 to 76780a4 Compare December 3, 2019 23:42

Leland-Takamine merged commit 09b1c95 into master Dec 4, 2019

Leland-Takamine deleted the scope-factory branch December 4, 2019 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ScopeFactory API #136

Add ScopeFactory API #136

Leland-Takamine commented Oct 4, 2019

andreasnomikos left a comment

andreasnomikos Oct 5, 2019

Leland-Takamine Oct 7, 2019

andreasnomikos Oct 9, 2019

Leland-Takamine Oct 9, 2019

andreasnomikos Oct 5, 2019

Leland-Takamine Oct 7, 2019

andreasnomikos Oct 9, 2019

Leland-Takamine Oct 9, 2019

andreasnomikos Oct 5, 2019

Leland-Takamine Oct 7, 2019

andreasnomikos Oct 9, 2019

Leland-Takamine Oct 9, 2019

andreasnomikos Oct 5, 2019

Leland-Takamine Oct 7, 2019

Leland-Takamine Dec 3, 2019

ugonna Dec 3, 2019

Leland-Takamine Dec 4, 2019

ugonna Dec 3, 2019

Leland-Takamine Dec 3, 2019


		private ScopeFactory() {}

		private static final Map<Class<?>, Constructor<?>> scopeClassToCreateMethod = new HashMap<>();


		private static final Map<Class<?>, Constructor<?>> scopeClassToCreateMethod = new HashMap<>();

		public static <S extends Creatable<NoDependencies>> S create(Class<S> scopeClass) {

Add ScopeFactory API #136

Add ScopeFactory API #136

Conversation

Leland-Takamine commented Oct 4, 2019

andreasnomikos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment