Skip to content

Commit

Permalink
Pool dynamic
Browse files Browse the repository at this point in the history
Reviewed By: ahmedre

Differential Revision: D4398446

fbshipit-source-id: ff528b7b52a2b1521627c0fca17b7ee2b18102de
  • Loading branch information
Emil Sjolander authored and facebook-github-bot committed Jan 11, 2017
1 parent 1d9ba50 commit b27c541
Show file tree
Hide file tree
Showing 9 changed files with 99 additions and 16 deletions.
2 changes: 1 addition & 1 deletion ReactAndroid/src/main/java/com/facebook/react/bridge/BUCK
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ android_library(
react_native_dep('java/com/facebook/systrace:systrace'),
react_native_dep('libraries/fbcore/src/main/java/com/facebook/common/logging:logging'),
react_native_dep('libraries/soloader/java/com/facebook/soloader:soloader'),
react_native_dep('third-party/android/support/v4:lib-support-v4'),
react_native_dep('third-party/java/infer-annotations:infer-annotations'),
react_native_dep('third-party/java/jsr-305:jsr-305'),
react_native_target('java/com/facebook/react/common:common'),
Expand All @@ -33,4 +34,3 @@ android_library(
'PUBLIC',
],
)

Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public ReadableNativeArray extractArgument(
@Override
public Dynamic extractArgument(
CatalystInstance catalystInstance, ExecutorToken executorToken, ReadableNativeArray jsArguments, int atIndex) {
return new DynamicFromArray(jsArguments, atIndex);
return DynamicFromArray.create(jsArguments, atIndex);
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,5 @@ public interface Dynamic {
ReadableArray asArray();
ReadableMap asMap();
ReadableType getType();
void recycle();
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,51 +9,92 @@

package com.facebook.react.bridge;

import javax.annotation.Nullable;

import android.support.v4.util.Pools;

/**
* Implementation of Dynamic wrapping a ReadableArray.
*/
public class DynamicFromArray implements Dynamic {
private static final Pools.SimplePool<DynamicFromArray> sPool = new Pools.SimplePool<>(10);

private @Nullable ReadableArray mArray;
private int mIndex = -1;

private final ReadableArray mArray;
private final int mIndex;
// This is a pools object. Hide the constructor.
private DynamicFromArray() {}

public DynamicFromArray(ReadableArray array, int index) {
mArray = array;
mIndex = index;
public static DynamicFromArray create(ReadableArray array, int index) {
DynamicFromArray dynamic = sPool.acquire();
if (dynamic == null) {
dynamic = new DynamicFromArray();
}
dynamic.mArray = array;
dynamic.mIndex = index;
return dynamic;
}

@Override
public void recycle() {
mArray = null;
mIndex = -1;
sPool.release(this);
}

@Override
public boolean asBoolean() {
if (mArray == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mArray.getBoolean(mIndex);
}

@Override
public double asDouble() {
if (mArray == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mArray.getDouble(mIndex);
}

@Override
public int asInt() {
if (mArray == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mArray.getInt(mIndex);
}

@Override
public String asString() {
if (mArray == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mArray.getString(mIndex);
}

@Override
public ReadableArray asArray() {
if (mArray == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mArray.getArray(mIndex);
}

@Override
public ReadableMap asMap() {
if (mArray == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mArray.getMap(mIndex);
}

@Override
public ReadableType getType() {
if (mArray == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mArray.getType(mIndex);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,51 +9,92 @@

package com.facebook.react.bridge;

import javax.annotation.Nullable;

import android.support.v4.util.Pools;

/**
* Implementation of Dynamic wrapping a ReadableMap.
*/
public class DynamicFromMap implements Dynamic {
private static final Pools.SimplePool<DynamicFromMap> sPool = new Pools.SimplePool<>(10);

private @Nullable ReadableMap mMap;
private @Nullable String mName;

private final ReadableMap mMap;
private final String mName;
// This is a pools object. Hide the constructor.
private DynamicFromMap() {}

public DynamicFromMap(ReadableMap map, String name) {
mMap = map;
mName = name;
public static DynamicFromMap create(ReadableMap map, String name) {
DynamicFromMap dynamic = sPool.acquire();
if (dynamic == null) {
dynamic = new DynamicFromMap();
}
dynamic.mMap = map;
dynamic.mName = name;
return dynamic;
}

@Override
public void recycle() {
mMap = null;
mName = null;
sPool.release(this);
}

@Override
public boolean asBoolean() {
if (mMap == null || mName == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mMap.getBoolean(mName);
}

@Override
public double asDouble() {
if (mMap == null || mName == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mMap.getDouble(mName);
}

@Override
public int asInt() {
if (mMap == null || mName == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mMap.getInt(mName);
}

@Override
public String asString() {
if (mMap == null || mName == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mMap.getString(mName);
}

@Override
public ReadableArray asArray() {
if (mMap == null || mName == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mMap.getArray(mName);
}

@Override
public ReadableMap asMap() {
if (mMap == null || mName == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mMap.getMap(mName);
}

@Override
public ReadableType getType() {
if (mMap == null || mName == null) {
throw new IllegalStateException("This dynamic value has been recycled");
}
return mMap.getType(mName);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public JavaOnlyMap getMap(int index) {

@Override
public Dynamic getDynamic(int index) {
return new DynamicFromArray(this, index);
return DynamicFromArray.create(this, index);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public JavaOnlyArray getArray(String name) {

@Override
public Dynamic getDynamic(String name) {
return new DynamicFromMap(this, name);
return DynamicFromMap.create(this, name);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ protected ReadableNativeArray(HybridData hybridData) {

@Override
public Dynamic getDynamic(int index) {
return new DynamicFromArray(this, index);
return DynamicFromArray.create(this, index);
}

public ArrayList<Object> toArrayList() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ protected ReadableNativeMap(HybridData hybridData) {

@Override
public Dynamic getDynamic(String name) {
return new DynamicFromMap(this, name);
return DynamicFromMap.create(this, name);
}

@Override
Expand Down

2 comments on commit b27c541

@haitaoli
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @emilsjolander @ahmedre, I have a question about the usage of sPool object in this commit. If I understand correctly, RN has two threads (or more?) that can update props, the main activity thread and the bridge thread:

android.support.v4.util.Pools$SimplePool.acquire(Pools.java:106)
com.facebook.react.bridge.DynamicFromMap.create(DynamicFromMap.java:29)
com.facebook.react.bridge.ReadableNativeMap.getDynamic(ReadableNativeMap.java:52)
com.facebook.react.uimanager.ReactStylesDiffMap.getDynamic(ReactStylesDiffMap.java:88)
com.facebook.react.uimanager.ViewManagersPropertyCache$DynamicPropSetter.extractProperty(ViewManagersPropertyCache.java:135)
com.facebook.react.uimanager.ViewManagersPropertyCache$PropSetter.updateViewProp(ViewManagersPropertyCache.java:82)
com.facebook.react.uimanager.ViewManagerPropertyUpdater$FallbackViewManagerSetter.setProperty(ViewManagerPropertyUpdater.java:129)
com.facebook.react.uimanager.ViewManagerPropertyUpdater.updateProps(ViewManagerPropertyUpdater.java:48)
com.facebook.react.uimanager.ViewManager.updateProperties(ViewManager.java:36)
com.facebook.react.uimanager.NativeViewHierarchyManager.createView(NativeViewHierarchyManager.java:227)
com.facebook.react.uimanager.UIViewOperationQueue$CreateViewOperation.execute(UIViewOperationQueue.java:150)
com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.dispatchPendingNonBatchedOperations(UIViewOperationQueue.java:923)
com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.doFrameGuarded(UIViewOperationQueue.java:895)
com.facebook.react.uimanager.GuardedFrameCallback.doFrame(GuardedFrameCallback.java:31)
com.facebook.react.modules.core.ReactChoreographer$ReactChoreographerDispatcher.doFrame(ReactChoreographer.java:136)
com.facebook.react.modules.core.ChoreographerCompat$FrameCallback$1.doFrame(ChoreographerCompat.java:107)
android.view.Choreographer$CallbackRecord.run(Choreographer.java:957)
android.view.Choreographer.doCallbacks(Choreographer.java:734)
android.view.Choreographer.doFrame(Choreographer.java:667)
android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:945)
android.os.Handler.handleCallback(Handler.java:751)
android.os.Handler.dispatchMessage(Handler.java:95)
android.os.Looper.loop(Looper.java:154)
android.app.ActivityThread.main(ActivityThread.java:6776)
java.lang.reflect.Method.invoke(Method.java:-2)
com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1520)
com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1410)

and

ReadableNativeMap.java:-2:com.facebook.react.bridge.ReadableNativeMap$ReadableNativeMapKeySetIterator.nextKey  
ViewManagerPropertyUpdater.java:57:com.facebook.react.uimanager.ViewManagerPropertyUpdater.updateProps 
ReactShadowNode.java:272:com.facebook.react.uimanager.ReactShadowNode.updateProperties 
UIImplementation.java:197:com.facebook.react.uimanager.UIImplementation.createView 
UIManagerModule.java:249:com.facebook.react.uimanager.UIManagerModule.createView 
Method.java:-2:java.lang.reflect.Method.invoke 
JavaMethodWrapper.java:363:com.facebook.react.bridge.JavaMethodWrapper.invoke  
JavaModuleWrapper.java::166com.facebook.react.bridge.JavaModuleWrapper.invoke  
NativeRunnable.java:-2:com.facebook.react.bridge.queue.NativeRunnable.run  
Handler.java:751:android.os.Handler.handleCallback 
Handler.java:95:android.os.Handler.dispatchMessage 
MessageQueueThreadHandler.java:31:com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage  
Looper.java:154:android.os.Looper.loop 
MessageQueueThreadImpl.java:194:com.facebook.react.bridge.queue.MessageQueueThreadImpl$3.run 
Thread.java:762:java.lang.Thread.run

But since SimplePool is not thread-safe, I'm seeing crashes when accessing the sPool object. There are also issues when a prop receives wrong value types, e.g. int64 when string is expected. I suspect those are related as well. What do you think?

cc @gpeal

@haitaoli
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lelandrichardson and I looked at this issue together, and we now understand why we only started to see it recently. Currently the only dynamic props are size props such as left, paddingVertical and margin. These props are only used in layout thread so everything works. Recently I made a change to support more values in accessibilityComponentType, and in that process I made this prop dynamic as well. Unfortunately this prop is used in main UI thread. So now both threads can access this sPool object.

Please sign in to comment.