Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/leaks and performance loss observed using jsonfactory #361

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Bugfix/leaks and performance loss observed using jsonfactory #361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6671eb0
fix: fixes leak and improves performance of generic calls
flsobral Jul 19, 2021
4d5917a
fix: Method leaking parameter and exception type arrays
flsobral Jul 19, 2021
e9c9147
chore: skip parameter check if looking for method w/o arguments
flsobral Jul 19, 2021
b64f34c
fix: fix casting addresses for 64 bits systems
flsobral Jul 19, 2021
ae84826
chore: caches the Class' methods after first call to getMethods
flsobral Jul 19, 2021
398865b
refactor: move instance fields macros to new header
flsobral Jul 19, 2021
309579c
chore: caches Class' methods after first call to parse
flsobral Jul 19, 2021
3dd1c54
style: source reformat
flsobral Jul 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
248 changes: 137 additions & 111 deletions TotalCrossSDK/src/main/java/totalcross/json/JSONFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,48 +10,55 @@
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;

/**
The JSONFactory class helps converting json objects into Java objects, using reflection.

Some examples:

<pre>
class Car
{
private int id;
private String description;

public int getId()
{
return id;
}
public void setId(int id)
{
this.id = id;
}
public String getDescription()
{
return description;
}
public void setDescription(String description)
{
this.description = description;
}
}
</pre>

You can retrieve a new Car object using:
<pre>
Car cc = JSONFactory.parse("{\"carro\":{\"id\":-1,\"descricao\":\"GOL\"}}", Carro.class);
</pre>

You may also retrieve a list or an array. See the JSONSample in the TotalCrossAPI.
* The JSONFactory class helps converting json objects into Java objects, using
* reflection.
*
* Some examples:
*
* <pre>
* class Car {
* private int id;
* private String description;
*
* public int getId() {
* return id;
* }
*
* public void setId(int id) {
* this.id = id;
* }
*
* public String getDescription() {
* return description;
* }
*
* public void setDescription(String description) {
* this.description = description;
* }
* }
* </pre>
*
* You can retrieve a new Car object using:
*
* <pre>
* Car cc = JSONFactory.parse("{\"carro\":{\"id\":-1,\"descricao\":\"GOL\"}}", Carro.class);
* </pre>
*
* You may also retrieve a list or an array. See the JSONSample in the
* TotalCrossAPI.
*/
public class JSONFactory {
public static <T> List<T> asList(String json, Class<T> classOfT) throws InstantiationException,
IllegalAccessException, IllegalArgumentException, InvocationTargetException, JSONException, ArrayIndexOutOfBoundsException, NoSuchMethodException, SecurityException {
private static Map<Class<?>, Map<String, Method>> classes = new HashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using this as a source of cache, one should be aware that it isn't thread safe OR the thread safety must be provided by the implementor


public static <T> List<T> asList(String json, Class<T> classOfT)
throws InstantiationException, IllegalAccessException, IllegalArgumentException, InvocationTargetException,
JSONException, ArrayIndexOutOfBoundsException, NoSuchMethodException, SecurityException {
List<T> list = new ArrayList<T>();
try {
JSONArray jsonArray = new JSONArray(json);
Expand Down Expand Up @@ -88,13 +95,15 @@ public static <T> T parse(String json, Class<T> classOfT) throws InstantiationEx
return parse(new JSONObject(json), classOfT);
}

public static <T> T parse(JSONArray jsonArray, Class<T> classOfT) throws InstantiationException,
IllegalAccessException, IllegalArgumentException, InvocationTargetException, JSONException, ArrayIndexOutOfBoundsException, NoSuchMethodException, SecurityException {
return parse(null, jsonArray, classOfT);
public static <T> T parse(JSONArray jsonArray, Class<T> classOfT)
throws InstantiationException, IllegalAccessException, IllegalArgumentException, InvocationTargetException,
JSONException, ArrayIndexOutOfBoundsException, NoSuchMethodException, SecurityException {
return parse(null, jsonArray, classOfT);
}

private static <T> T parse(Object outerObject, JSONArray jsonArray, Class<T> classOfT) throws InstantiationException,
IllegalAccessException, IllegalArgumentException, InvocationTargetException, JSONException, ArrayIndexOutOfBoundsException, NoSuchMethodException, SecurityException {

private static <T> T parse(Object outerObject, JSONArray jsonArray, Class<T> classOfT)
throws InstantiationException, IllegalAccessException, IllegalArgumentException, InvocationTargetException,
JSONException, ArrayIndexOutOfBoundsException, NoSuchMethodException, SecurityException {
if (classOfT.isArray()) {
T array;
try {
Expand All @@ -110,95 +119,112 @@ private static <T> T parse(Object outerObject, JSONArray jsonArray, Class<T> cla
}
return parse(outerObject, jsonArray, classOfT);
}

public static <T> T parse(JSONObject jsonObject, Class<T> classOfT) throws InstantiationException,
IllegalAccessException, IllegalArgumentException, InvocationTargetException, JSONException, NoSuchMethodException, SecurityException {
return parse(null, jsonObject, classOfT);

public static <T> T parse(JSONObject jsonObject, Class<T> classOfT)
throws InstantiationException, IllegalAccessException, IllegalArgumentException, InvocationTargetException,
JSONException, NoSuchMethodException, SecurityException {
return parse(null, jsonObject, classOfT);
}

private static <T> T parse(Object outerObject, JSONObject jsonObject, Class<T> classOfT) throws InstantiationException,
IllegalAccessException, IllegalArgumentException, InvocationTargetException, JSONException, NoSuchMethodException, SecurityException {
private static <T> T parse(Object outerObject, JSONObject jsonObject, Class<T> classOfT)
throws InstantiationException, IllegalAccessException, IllegalArgumentException, InvocationTargetException,
JSONException, NoSuchMethodException, SecurityException {
if (classOfT.isArray()) {
throw new IllegalArgumentException();
}
T object = null;
try {
object = classOfT.newInstance();
object = classOfT.newInstance();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to me that the creation of a single object based on its class could also be cached, in a similar way that it is possible to cache methods.

It should be easier to read if the creation of the object was simply:

T object = createNewInstanceOfT(classOfT, outerObject);

and let createNewInstanceOfT throws all the exceptions. createNewInstanceOfT could be like this (not taking care of race conditions yet):

private static final Supplier<Object> FORBIDEN = () -> {
  throw new RuntimeException("oops");
};

private static final Function<Object, Object> FORBIDEN_FUNCTION = () -> {
  throw new RuntimeException("oops");
};

private static <T> T createNewInstanceOfT(Class<T> classOfT, Object outerObject) {
  Supplier<T> supplierOfT = (Supplier<T>) cacheCtor.get(claffOfT);
  if (supplierOfT != null && supplierOfT != FORBIDEN) {
    return supplierOfT .get();
  } else if (supplierOfT == null) {
    try {
      T obj = classOfT.newInstance();
      cacheCtor,put(classOfT, classOfT::newInstance); // working around exceptions, not literally RACE-CONDITION-AWARE
      return obj;
    } catch (Exception e) {
      cacheCtor.put(classOfT, FORBIDEN); // mark such that won't try to fetch default ctor again RACE-CONDITION-AWARE
    }
  }
  if (outerObject == null ||  classOfT.getName().indexOf(outerObject.getClass().getName()) == -1) {
    // CANNOT INSTANTIATE
    throw new ProperCantInstantiateException();
  }
  HashMap<Class<?>, Function<Object, T>> mapConstructorOfTWithParam = (Function<Object, T>) cacheCtorWithParam(classOfT);
  if (mapConstructorOfTWithParam != null) {
    Function<Object, T> constructorOfTWithParam = mapConstructorOfTWithParam .get(outerObject.getClass());
    if (constructorOfTWithParam  == FORBIDEN_FUNCTION ) {
      // CANNOT INSTANTIATE
      throw new ProperCantInstantiateException();
    } else if (constructorOfTWithParam != null) {
      return constructorOfTWithParam.apply(outerObject);
    }
  } else {
    mapConstructorOfTWithParam = new HashMap<>();
    cacheCtorWithParam.put(classOfT, mapConstructorOfTWithParam); // RACE-CONDITION-AWARE
  }
  Constructor<T> constructorOfT = classOfT.getDeclaredConstructor(outerObject.getClass());
  if (constructorOfT == null) {
    mapConstructorOfTWithParam.put(outerObject.getClass(), FORBIDEN_FUNCTION); // mark such that won't try to fetch ctor with this class again RACE-CONDITION-AWARE
    // CANNOT INSTANTIATE
    throw new ProperCantInstantiateException();
  }
  mapConstructorOfTWithParam.put(outerObject.getClass(), oObject -> constructorOfT.newInstance(oObject)); // working around exceptions, not literally RACE-CONDITION-AWARE
  return constructorOfT.newInstance(outerObject);
}

I took some extra care with situations where it won't be possible to create such objects, but perharps this shouldn't?
The idea was to get a proper headstart to know which ctor to call, but if it is invalid then MAYBE my worries are just overengineering

} catch (InstantiationException e) {
if (outerObject != null && classOfT.getName().indexOf(outerObject.getClass().getName()) != -1) {
Constructor<T> constructorOfT = classOfT.getDeclaredConstructor(outerObject.getClass());
if (constructorOfT != null) {
object = constructorOfT.newInstance(outerObject);
}
if (outerObject != null && classOfT.getName().indexOf(outerObject.getClass().getName()) != -1) {
Constructor<T> constructorOfT = classOfT.getDeclaredConstructor(outerObject.getClass());
if (constructorOfT != null) {
object = constructorOfT.newInstance(outerObject);
}
if (object == null) {
throw e;
}
if (object == null) {
throw e;
}
}

Map<String, Method> methodCache = classes.get(classOfT);
if (methodCache == null) {
methodCache = mapClassMethodsToJsonNames(classOfT);
classes.put(classOfT, methodCache);
}
Comment on lines +150 to +154
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you got a race condition

Maybe trying a double-checked lock?


Iterator<String> keyIterator = jsonObject.keys();
while (keyIterator.hasNext()) {
final String name = keyIterator.next();
Method method = methodCache.get(name);
if (method == null || jsonObject.isNull(name)) {
continue;
}

Class<?> parameterType = method.getParameterTypes()[0];
if (parameterType.isPrimitive()) {
if (parameterType.isAssignableFrom(boolean.class)) {
method.invoke(object, jsonObject.getBoolean(name));
} else if (parameterType.isAssignableFrom(int.class)) {
method.invoke(object, jsonObject.getInt(name));
} else if (parameterType.isAssignableFrom(long.class)) {
method.invoke(object, jsonObject.getLong(name));
} else if (parameterType.isAssignableFrom(double.class)) {
method.invoke(object, jsonObject.getDouble(name));
}
} else if (parameterType.isAssignableFrom(String.class)) {
method.invoke(object, jsonObject.getString(name));
} else if (parameterType.isAssignableFrom(Double.class)) {
method.invoke(object, jsonObject.getDouble(name));
} else if (parameterType.isAssignableFrom(Integer.class)) {
method.invoke(object, jsonObject.getInt(name));
} else if (parameterType.isAssignableFrom(Long.class)) {
method.invoke(object, jsonObject.getLong(name));
} else if (parameterType.isAssignableFrom(Boolean.class)) {
method.invoke(object, jsonObject.getBoolean(name));
} else if (parameterType.isArray()) {
method.invoke(object, parse(object, jsonObject.getJSONArray(name), parameterType));
} else {
method.invoke(object, parse(object, jsonObject.getJSONObject(name), parameterType));
}
}
return object;
}

private static Map<String, Method> mapClassMethodsToJsonNames(Class<?> classOfT) {
final Map<String, Method> methodCache = new HashMap<>();
Method[] methods = classOfT.getMethods();
for (Method method : methods) {
String methodName = method.getName();
Class<?>[] paramTypes = method.getParameterTypes();
if (paramTypes != null && paramTypes.length == 1 && methodName.length() > 3 && methodName.startsWith("set")) {
final String originalName = Character.toLowerCase(methodName.charAt(3)) + methodName.substring(4);
String name = null;
// look for the field name in the json based on the method name
if (jsonObject.has(originalName)) {
name = originalName;
} else if (!jsonObject.has(name = originalName.toLowerCase())) {
// not found as-is or lowercased? try replacing camel case with underscore
/*
* originally done using regex, but totalcross implementation has some bugs and
* until they are fixed this is done looping through the characters
* originalName.replaceAll("(.)(\\p{Upper})", "$1_$2").toLowerCase();
*/
StringBuilder sb = new StringBuilder();
boolean lastWasUnderscored = false;
for(int i = 0 ; i < originalName.length() ; i++) {
char c = originalName.charAt(i);
if (!lastWasUnderscored && Character.isUpperCase(c)) {
lastWasUnderscored = true;
sb.append('_');
} else {
lastWasUnderscored = false;
}
sb.append(Character.toLowerCase(c));
}
final String underscoredName = sb.toString();
if (jsonObject.has(underscoredName)) {
name = underscoredName;
}
}
if (!jsonObject.isNull(name)) {
Class<?> parameterType = method.getParameterTypes()[0];
if (parameterType.isPrimitive()) {
if (parameterType.isAssignableFrom(boolean.class)) {
method.invoke(object, jsonObject.getBoolean(name));
} else if (parameterType.isAssignableFrom(int.class)) {
method.invoke(object, jsonObject.getInt(name));
} else if (parameterType.isAssignableFrom(long.class)) {
method.invoke(object, jsonObject.getLong(name));
} else if (parameterType.isAssignableFrom(double.class)) {
method.invoke(object, jsonObject.getDouble(name));
}
} else if (parameterType.isAssignableFrom(String.class)) {
method.invoke(object, jsonObject.getString(name));
} else if (parameterType.isAssignableFrom(Double.class)) {
method.invoke(object, jsonObject.getDouble(name));
} else if (parameterType.isAssignableFrom(Integer.class)) {
method.invoke(object, jsonObject.getInt(name));
} else if (parameterType.isAssignableFrom(Long.class)) {
method.invoke(object, jsonObject.getLong(name));
} else if (parameterType.isAssignableFrom(Boolean.class)) {
method.invoke(object, jsonObject.getBoolean(name));
} else if (parameterType.isArray()) {
method.invoke(object, parse(object, jsonObject.getJSONArray(name), parameterType));
// name as-is
methodCache.put(originalName, method);
// lowercased name
methodCache.put(originalName.toLowerCase(), method);
// not found as-is or lowercased? try replacing camel case with underscore
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that it is safe to assume that a private method will be inlined by the javac compiler with proper given optimization, so for code clarity I think that would be better it this snippet that gives snakecase names was removed to a proper method

Also, I think that getABCD will yield as a result a_bc_d. I have no idea if it is intended to behave like this, but I find it odd and surprises me.

* originally done using regex, but totalcross implementation has some bugs and
* until they are fixed this is done looping through the characters
* originalName.replaceAll("(.)(\\p{Upper})", "$1_$2").toLowerCase();
*/
StringBuilder sb = new StringBuilder();
boolean lastWasUnderscored = false;
for (int i = 0; i < originalName.length(); i++) {
char c = originalName.charAt(i);
if (!lastWasUnderscored && Character.isUpperCase(c)) {
lastWasUnderscored = true;
sb.append('_');
} else {
method.invoke(object, parse(object, jsonObject.getJSONObject(name), parameterType));
lastWasUnderscored = false;
}
sb.append(Character.toLowerCase(c));
}
final String underscoredName = sb.toString();
methodCache.put(underscoredName, method);
}
}
return object;
return methodCache;
}
}
1 change: 1 addition & 0 deletions TotalCrossSDK/src/main/java/totalcross/lang/Class4D.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public final class Class4D<T> {
Object nativeStruct; // TClass
String targetName; // java.lang.String
String ncached, cached;
Method[] methods;

/** The TotalCross deployer can find classes that are instantiated using Class.forName if, and only if, they are
* String constants. If you build the className dynamically, then you must include the file passing it to the tc.Deploy
Expand Down
4 changes: 0 additions & 4 deletions TotalCrossVM/src/nm/instancefields.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@
#define StringBuffer_charsStart(o) ((JCharP)(ARRAYOBJ_START(StringBuffer_chars(o))))
#define StringBuffer_count(o) FIELD_I32(o, 0)

// java.lang.Class
#define Class_nativeStruct(o) FIELD_OBJ(o, OBJ_CLASS(o), 0)
#define Class_targetName(o) FIELD_OBJ(o, OBJ_CLASS(o), 1)

// java.lang.reflect.Field
#define Field_index(o) FIELD_I32(o, 0)
#define Field_mod(o) FIELD_I32(o, 1)
Expand Down
39 changes: 31 additions & 8 deletions TotalCrossVM/src/nm/lang/Class.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@



#include "tcvm.h"
#include "Class.h"

TC_API void jlC_forName_s(NMParams p);

Expand Down Expand Up @@ -129,20 +129,23 @@ static void createMethodObject(Context currentContext, Method m, TCClass declari
setObjectLock(Method_name(*ret) = createStringObjectFromCharP(currentContext,isConstructor ? declaringClass->name : m->name,-1),UNLOCKED);
createClassObject(currentContext, declaringClass->name, Type_Null, &Method_declaringClass(*ret),null);
// parameters and exceptions
Method_parameterTypes(*ret) = createArrayObject(currentContext, "[java.lang.Class", n = m->paramCount);
if (Method_parameterTypes(*ret) && n > 0)
ptrObj = createArrayObject(currentContext, "[java.lang.Class", n = m->paramCount);
if (ptrObj && n > 0)
{
TCObject* oa = (TCObject*)ARRAYOBJ_START(Method_parameterTypes(*ret));
TCObject* oa = (TCObject*)ARRAYOBJ_START(ptrObj);
for (i=0; i < n; i++)
createClassObject(currentContext, declaringClass->cp->cls[m->cpParams[i]], m->cpParams[i] < Type_Object ? m->cpParams[i] : Type_Null, oa++, null);
}
Method_exceptionTypes(*ret) = createArrayObject(currentContext, "[java.lang.Class", n = 0); // thrown exceptions is not stored in TCClass!
if (Method_exceptionTypes(*ret) && n > 0)
setObjectLock(Method_parameterTypes(*ret) = ptrObj, UNLOCKED);

ptrObj = createArrayObject(currentContext, "[java.lang.Class", n = 0); // thrown exceptions is not stored in TCClass!
if (ptrObj && n > 0)
{
TCObject* oa = (TCObject*)ARRAYOBJ_START(Method_exceptionTypes(*ret));
TCObject* oa = (TCObject*)ARRAYOBJ_START(ptrObj);
for (i=0; i < n; i++)
createClassObject(currentContext, m->exceptionHandlers[i].className, Type_Null, oa++, null);
}
setObjectLock(Method_exceptionTypes(*ret) = ptrObj, UNLOCKED);

// return and type
if (!isConstructor)
Expand Down Expand Up @@ -566,7 +569,27 @@ TC_API void jlC_getFields(NMParams p) // java/lang/Class public native java.lang
//////////////////////////////////////////////////////////////////////////
TC_API void jlC_getMethods(NMParams p) // java/lang/Class public native java.lang.reflect.Method[] getMethods() throws SecurityException;
{
getMCarray(p,false,true);
TCObject this_ = p->obj[0];
TCObject methods = Class_methods(this_);
TCObject ret;

if (methods == null)
{
getMCarray(p,false,true);
Class_methods(this_) = methods = p->retO;
}

ret = createArrayObject(p->currentContext, "[java.lang.reflect.Method", ARRAYOBJ_LEN(methods));
if (ret)
{
int32 length = ARRAYOBJ_LEN(methods);
TCObjectArray psrc = (TCObjectArray) ARRAYOBJ_START(methods);
TCObjectArray pdst = (TCObjectArray) ARRAYOBJ_START(ret);

while (length-- >= 0)
*pdst++ = *psrc++;
}
setObjectLock(p->retO = ret, UNLOCKED);
}
//////////////////////////////////////////////////////////////////////////
TC_API void jlC_getConstructors(NMParams p) // java/lang/Class public native java.lang.reflect.Constructor[] getConstructors() throws SecurityException;
Expand Down
15 changes: 15 additions & 0 deletions TotalCrossVM/src/nm/lang/Class.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright (C) 2021 TotalCross Global Mobile Platform Ltda.
//
// SPDX-License-Identifier: LGPL-2.1-only

#ifndef Class_h
#define Class_h

#include "tcvm.h"

// java.lang.Class
#define Class_nativeStruct(o) FIELD_OBJ(o, OBJ_CLASS(o), 0)
#define Class_targetName(o) FIELD_OBJ(o, OBJ_CLASS(o), 1)
#define Class_methods(o) FIELD_OBJ(o, OBJ_CLASS(o), 4)

#endif
Loading