Skip to content

Commit

Permalink
remove useMapNativeAccessor stuff
Browse files Browse the repository at this point in the history 
Summary:
This was quite a rabit hole of remove deps -> delete dead code -> repeat.

Waaay simpler now with less duplicate lookups, redundant type verification, and extra function calls.

Reviewed By: mdvacca

Differential Revision: D14486283

fbshipit-source-id: 035db30181755d046a1ae99760468b954b2449df
  • Loading branch information
@sahrens @facebook-github-bot
sahrens authored and facebook-github-bot committed Mar 25, 2019
1 parent a46fba5 commit b257e06
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 298 deletions.
7 changes: 3 additions & 4 deletions ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/WritableNativeMapTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static org.fest.assertions.api.Assertions.assertThat;

import androidx.test.runner.AndroidJUnit4;
import com.facebook.react.bridge.NoSuchKeyException;
import com.facebook.react.bridge.UnexpectedNativeTypeException;
import com.facebook.react.bridge.WritableNativeArray;
import com.facebook.react.bridge.WritableNativeMap;
Expand All @@ -27,7 +28,6 @@ public void setup() {
mMap.putMap("map", new WritableNativeMap());
mMap.putArray("array", new WritableNativeArray());
mMap.putBoolean("dvacca", true);
mMap.setUseNativeAccessor(true);
}

@Test
Expand Down Expand Up @@ -90,14 +90,13 @@ public void testArrayInvalidType() {
mMap.getArray("string");
}

@Ignore("Needs to be implemented")
@Test
public void testErrorMessageContainsKey() {
String key = "fkg";
try {
mMap.getString(key);
Assert.fail("Expected an UnexpectedNativeTypeException to be thrown");
} catch (UnexpectedNativeTypeException e) {
Assert.fail("Expected an NoSuchKeyException to be thrown");
} catch (NoSuchKeyException e) {
assertThat(e.getMessage()).contains(key);
}
}
Expand Down
121 changes: 9 additions & 112 deletions ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,20 +36,14 @@ protected ReadableNativeMap(HybridData hybridData) {
private @Nullable HashMap<String, ReadableType> mLocalTypeMap;
private static int mJniCallCounter;

public static void setUseNativeAccessor(boolean useNativeAccessor) {
ReactFeatureFlags.useMapNativeAccessor = useNativeAccessor;
}

public static int getJNIPassCounter() {
return mJniCallCounter;
}

private HashMap<String, Object> getLocalMap() {
// Fast return for the common case
if (mLocalMap != null) {
return mLocalMap;
}
// Check and when necessary get keys atomically
synchronized (this) {
if (mKeys == null) {
mKeys = Assertions.assertNotNull(importKeys());
Expand All @@ -73,11 +67,9 @@ private HashMap<String, Object> getLocalMap() {
private native Object[] importValues();

private @Nonnull HashMap<String, ReadableType> getLocalTypeMap() {
// Fast and non-blocking return for common case
if (mLocalTypeMap != null) {
return mLocalTypeMap;
}
// Check and when necessary get keys
synchronized (this) {
if (mKeys == null) {
mKeys = Assertions.assertNotNull(importKeys());
Expand All @@ -101,29 +93,17 @@ private HashMap<String, Object> getLocalMap() {

@Override
public boolean hasKey(@Nonnull String name) {
if (ReactFeatureFlags.useMapNativeAccessor) {
mJniCallCounter++;
return hasKeyNative(name);
}
return getLocalMap().containsKey(name);
}

private native boolean hasKeyNative(String name);

@Override
public boolean isNull(@Nonnull String name) {
if (ReactFeatureFlags.useMapNativeAccessor) {
mJniCallCounter++;
return isNullNative(name);
}
if (getLocalMap().containsKey(name)) {
return getLocalMap().get(name) == null;
}
throw new NoSuchKeyException(name);
}

private native boolean isNullNative(@Nonnull String name);

private @Nonnull Object getValue(@Nonnull String name) {
if (hasKey(name) && !(isNull(name))) {
return Assertions.assertNotNull(getLocalMap().get(name));
Expand Down Expand Up @@ -152,7 +132,7 @@ private <T> T getValue(String name, Class<T> type) {

private void checkInstance(String name, Object value, Class type) {
if (value != null && !type.isInstance(value)) {
throw new ClassCastException(
throw new UnexpectedNativeTypeException(
"Value for "
+ name
+ " cannot be cast from "
Expand All @@ -164,86 +144,43 @@ private void checkInstance(String name, Object value, Class type) {

@Override
public boolean getBoolean(@Nonnull String name) {
if (ReactFeatureFlags.useMapNativeAccessor) {
mJniCallCounter++;
return getBooleanNative(name);
}
return getValue(name, Boolean.class).booleanValue();
}

private native boolean getBooleanNative(String name);

@Override
public double getDouble(@Nonnull String name) {
if (ReactFeatureFlags.useMapNativeAccessor) {
mJniCallCounter++;
return getDoubleNative(name);
}
return getValue(name, Double.class).doubleValue();
}

private native double getDoubleNative(String name);

@Override
public int getInt(@Nonnull String name) {
if (ReactFeatureFlags.useMapNativeAccessor) {
mJniCallCounter++;
return getIntNative(name);
}

// All numbers coming out of native are doubles, so cast here then truncate
return getValue(name, Double.class).intValue();
}

private native int getIntNative(String name);

@Override
public @Nullable String getString(@Nonnull String name) {
if (ReactFeatureFlags.useMapNativeAccessor) {
mJniCallCounter++;
return getStringNative(name);
}
return getNullableValue(name, String.class);
}

private native String getStringNative(String name);

@Override
public @Nullable ReadableArray getArray(@Nonnull String name) {
if (ReactFeatureFlags.useMapNativeAccessor) {
mJniCallCounter++;
return getArrayNative(name);
}
return getNullableValue(name, ReadableArray.class);
}

private native ReadableNativeArray getArrayNative(String name);

@Override
public @Nullable ReadableNativeMap getMap(@Nonnull String name) {
if (ReactFeatureFlags.useMapNativeAccessor) {
mJniCallCounter++;
return getMapNative(name);
}
return getNullableValue(name, ReadableNativeMap.class);
}

private native ReadableNativeMap getMapNative(String name);

@Override
public @Nonnull ReadableType getType(@Nonnull String name) {
if (ReactFeatureFlags.useMapNativeAccessor) {
mJniCallCounter++;
return getTypeNative(name);
}
if (getLocalTypeMap().containsKey(name)) {
return Assertions.assertNotNull(getLocalTypeMap().get(name));
}
throw new NoSuchKeyException(name);
}

private native ReadableType getTypeNative(String name);

@Override
public @Nonnull Dynamic getDynamic(@Nonnull String name) {
return DynamicFromMap.create(this, name);
Expand Down Expand Up @@ -275,42 +212,6 @@ public boolean equals(Object obj) {

@Override
public @Nonnull HashMap<String, Object> toHashMap() {
if (ReactFeatureFlags.useMapNativeAccessor) {
ReadableMapKeySetIterator iterator = keySetIterator();
HashMap<String, Object> hashMap = new HashMap<>();

while (iterator.hasNextKey()) {
// increment for hasNextKey call
mJniCallCounter++;
String key = iterator.nextKey();
// increment for nextKey call
mJniCallCounter++;
switch (getType(key)) {
case Null:
hashMap.put(key, null);
break;
case Boolean:
hashMap.put(key, getBoolean(key));
break;
case Number:
hashMap.put(key, getDouble(key));
break;
case String:
hashMap.put(key, getString(key));
break;
case Map:
hashMap.put(key, Assertions.assertNotNull(getMap(key)).toHashMap());
break;
case Array:
hashMap.put(key, Assertions.assertNotNull(getArray(key)).toArrayList());
break;
default:
throw new IllegalArgumentException("Could not convert object with key: " + key + ".");
}
}
return hashMap;
}

// we can almost just return getLocalMap(), but we need to convert nested arrays and maps to the
// correct types first
HashMap<String, Object> hashMap = new HashMap<>(getLocalMap());
Expand All @@ -337,25 +238,21 @@ public boolean equals(Object obj) {
return hashMap;
}

/** Implementation of a {@link ReadableNativeMap} iterator in native memory. */
@DoNotStrip
private static class ReadableNativeMapKeySetIterator implements ReadableMapKeySetIterator {
@DoNotStrip private final HybridData mHybridData;

// Need to hold a strong ref to the map so that our native references remain valid.
@DoNotStrip private final ReadableNativeMap mMap;
private final Iterator<String> mIterator;

public ReadableNativeMapKeySetIterator(ReadableNativeMap readableNativeMap) {
mMap = readableNativeMap;
mHybridData = initHybrid(readableNativeMap);
mIterator = readableNativeMap.getLocalMap().keySet().iterator();
}

@Override
public native boolean hasNextKey();
public boolean hasNextKey() {
return mIterator.hasNext();
}

@Override
public native String nextKey();

private static native HybridData initHybrid(ReadableNativeMap readableNativeMap);
public String nextKey() {
return mIterator.next();
}
}
}
1 change: 0 additions & 1 deletion ReactAndroid/src/main/jni/react/jni/OnLoad.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ extern "C" JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void* reserved) {
NativeMap::registerNatives();
ReadableNativeMap::registerNatives();
WritableNativeMap::registerNatives();
ReadableNativeMapKeySetIterator::registerNatives();

#ifdef WITH_INSPECTOR
JInspector::registerNatives();
Expand Down
25 changes: 25 additions & 0 deletions ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,31 @@ using namespace facebook::jni;
namespace facebook {
namespace react {

jint makeJIntOrThrow(int64_t integer) {
jint javaint = static_cast<jint>(integer);
if (integer != javaint) {
throwNewJavaException(
exceptions::gUnexpectedNativeTypeExceptionClass,
"Value '%lld' doesn't fit into a 32 bit signed int", integer);
}
return javaint;
}

int64_t convertDynamicIfIntegral(const folly::dynamic& val) {
if (val.isInt()) {
return val.getInt();
}
double dbl = val.getDouble();
int64_t result = static_cast<int64_t>(dbl);
if (dbl != result) {
throwNewJavaException(
exceptions::gUnexpectedNativeTypeExceptionClass,
"Tried to read an int, but got a non-integral double: %f", dbl);
}
return result;
}



// This attribute exports the ctor symbol, so ReadableNativeArray to be
// constructed from other DSOs.
Expand Down
Loading

0 comments on commit b257e06

Please sign in to comment.