Use valueOf() to box primitive values instead of creating new objects every time #88
Conversation
src/main/c/jpy_jtype.c
Outdated
| Py_BEGIN_ALLOW_THREADS; \ | ||
| *JOBJECT_PTR = (*jenv)->CallStaticObjectMethod(jenv, TARGET_TYPE, STATIC_METHOD_ID, VALUE); \ | ||
| Py_END_ALLOW_THREADS; \ |
There was a problem hiding this comment.
It's not clear to me that we should / want to drop and then re-acquire the GIL (even thought the previous code to this was). In the other cases of CallStaticObjectMethod (and NewObject, etc) we are usually calling arbitrary pieces of code - at least in the context of this patch, we are only calling short valueOf methods.
@niloc132 @jmao-denver may have perspective as well.
There was a problem hiding this comment.
Any Java allocation could theoretically lead to a lengthy GC so worth keeping that in mind
Is there any performance testing around jpy/any idea what the cost of dropping/reacquiring the GIL is?
There was a problem hiding this comment.
It's an interesting question - are allocation points also "safepoints"? I think "yes", but the real answer may be more nuanced than that.
That said, you are probably right and the way we do it now is probably the safest option.
src/main/c/jpy_jtype.c
Outdated
| } else { | ||
| return JType_PythonToJavaConversionError(type, pyArg); | ||
| } | ||
| return JType_CreateJavaObject(jenv, type, pyArg, JPy_Boolean_JClass, JPy_Boolean_Init_MID, value, objectRef); | ||
| JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Boolean_JClass, JPy_Boolean_ValueOf_SMID, value, objectRef); |
There was a problem hiding this comment.
Extra ; (also in other places) given the macro defines it as well.
There was a problem hiding this comment.
I can change it, but I figured better this way because it's nice to "pretend it's a regular function" and because this is how other similar macros are used (e.g. JPy_DELETE_LOCAL_REF)
src/main/c/jpy_jtype.c
Outdated
| // always create new objects for long/int/short; any values assigned | ||
| // assigned to those types will be outside the default JVM cache | ||
| // range of [-128, 127]. | ||
| if (i != j) { | ||
| value.j = j; | ||
| return JType_CreateJavaObject(jenv, type, pyArg, JPy_Long_JClass, JPy_Long_Init_MID, value, objectRef); | ||
| JType_CREATE_JAVA_OBJECT_1_AND_RETURN(JPy_Long_JClass, JPy_Long_Init_MID, j, objectRef); |
There was a problem hiding this comment.
So, it's not clear to me there will be any negligible performance difference if we keep doing valueOf. In fact, these constructors are deprecated. Not that I think they'll ever go away, but it might be worthwhile to stick w/ valueOf.
There was a problem hiding this comment.
I also don't know if it makes a difference, but valueOf is HotSpotIntrinsicCandidate (I don't know if constructors are or can be?).
There was a problem hiding this comment.
Ok, thanks, good point. I changed it back. Doesn't make sense to use a deprecated code for performance reasons if there's no evidence it helps performance
…tors are deprecated.)
No description provided.