-
Notifications
You must be signed in to change notification settings - Fork 40
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.
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
base: master
Are you sure you want to change the base?
Bugfix/leaks and performance loss observed using jsonfactory #361
Conversation
TODO: more info and maybe rename the tag
The items of the array are locked, so the leak itself wasn't that big.
Always use size_t for addresses!
Changing instancefields.h is a PITA because it triggers a full build. Moving the related declarations to its own header is better.
It makes sense to cache Class and Method references within JSONFactory, because it will most likely be used for the same class several times. Especially when considering that classes are locked objects and their methods cached after the first getMethods call.
0f3876d
to
3dd1c54
Compare
*/ | ||
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<>(); |
There was a problem hiding this comment.
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
Map<String, Method> methodCache = classes.get(classOfT); | ||
if (methodCache == null) { | ||
methodCache = mapClassMethodsToJsonNames(classOfT); | ||
classes.put(classOfT, methodCache); | ||
} |
There was a problem hiding this comment.
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?
// lowercased name | ||
methodCache.put(originalName.toLowerCase(), method); | ||
// not found as-is or lowercased? try replacing camel case with underscore | ||
/* |
There was a problem hiding this comment.
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.
if (classOfT.isArray()) { | ||
throw new IllegalArgumentException(); | ||
} | ||
T object = null; | ||
try { | ||
object = classOfT.newInstance(); | ||
object = classOfT.newInstance(); |
There was a problem hiding this comment.
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
@@ -764,6 +765,11 @@ TC_API TValue executeMethod(Context context, Method method, ...) | |||
if (c == null) | |||
goto throwClassNotFoundException; | |||
} | |||
if (thisClass != c) | |||
{ | |||
thisClass = c; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thisClass
the last time I saw it was OBJ_CLASS(regO[code->mtd.this_])
. I had not seen it being updated. And c
is OBJ_CLASS(regO[code->mtd.this_])
I didn't see any changes in regO
nor in mtd.this_
.
The only change I see is in line -763/+764 that c = OBJ_CLASS(params.retO);
(or could goto throwClassNotFoundException
, but then it would be out of this here context). Maybe this jump should be just below said line?
Fixes leaks observed while using JSONFactory and adds some improvements for reflection and JSONFactory performance.
Marked as DRAFT because I'd like some input in how to improve commit messages and documentation for this PR.