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

Use valueOf() to box primitive values instead of creating new objects every time #88

Merged
merged 7 commits into from
Sep 27, 2022
Merged

Use valueOf() to box primitive values instead of creating new objects every time #88

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d97f4a7
Use valueOf() to box primitive values instead of creating new objects…
rbasralian Sep 23, 2022
f3c0b4e
fix inconsistent spacing
rbasralian Sep 23, 2022
64ae0f3
pass args by value, not by reference. avoid `jvalue` where possible.
rbasralian Sep 23, 2022
f6ea799
revert accidental commit (change is in jpy_type.c)
rbasralian Sep 23, 2022
aa31470
rename macro. remove superfluous jvalue.
rbasralian Sep 24, 2022
9c0f459
don't use valueOf for values that will never be cached. rename macro …
rbasralian Sep 25, 2022
dd60b8a
revert back to always using valueOf when boxing. (boxed type construc…
rbasralian Sep 26, 2022
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
73 changes: 41 additions & 32 deletions src/main/c/jpy_jtype.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,48 +401,59 @@ int JType_CreateJavaObject_2(JNIEnv* jenv, JPy_JType* type, PyObject* pyArg, jcl
return 0;
}

#define JType_CallStaticObjectMethodPrimitiveArgAndReturn(TARGET_TYPE, STATIC_METHOD_ID, VALUE, JOBJECT_PTR) \
rbasralian marked this conversation as resolved.
Show resolved Hide resolved
Py_BEGIN_ALLOW_THREADS; \
*JOBJECT_PTR = (*jenv)->CallStaticObjectMethod(jenv, TARGET_TYPE, STATIC_METHOD_ID, VALUE); \
Py_END_ALLOW_THREADS; \
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

if (*objectRef == NULL) { \
PyErr_NoMemory(); \
return -1; \
} \
JPy_ON_JAVA_EXCEPTION_RETURN(-1); \
return 0;

int JType_CreateJavaBooleanObject(JNIEnv* jenv, JPy_JType* type, PyObject* pyArg, jobject* objectRef)
{
jvalue value;
jboolean value;
if (PyBool_Check(pyArg) || JPy_IS_CLONG(pyArg)) {
value.z = JPy_AS_JBOOLEAN(pyArg);
value = JPy_AS_JBOOLEAN(pyArg);
} 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);
Copy link
Member

Choose a reason for hiding this comment

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

Extra ; (also in other places) given the macro defines it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

}

int JType_CreateJavaCharacterObject(JNIEnv* jenv, JPy_JType* type, PyObject* pyArg, jobject* objectRef)
{
jvalue value;
jchar value;
if (JPy_IS_CLONG(pyArg)) {
value.c = JPy_AS_JCHAR(pyArg);
value = JPy_AS_JCHAR(pyArg);
} else {
return JType_PythonToJavaConversionError(type, pyArg);
}
return JType_CreateJavaObject(jenv, type, pyArg, JPy_Character_JClass, JPy_Character_Init_MID, value, objectRef);
JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Character_JClass, JPy_Character_ValueOf_SMID, value, objectRef);
}

int JType_CreateJavaByteObject(JNIEnv* jenv, JPy_JType* type, PyObject* pyArg, jobject* objectRef)
{
jvalue value;
jbyte value;
if (JPy_IS_CLONG(pyArg)) {
value.b = JPy_AS_JBYTE(pyArg);
value = JPy_AS_JBYTE(pyArg);
} else {
return JType_PythonToJavaConversionError(type, pyArg);
}
return JType_CreateJavaObject(jenv, type, pyArg, JPy_Byte_JClass, JPy_Byte_Init_MID, value, objectRef);
JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Byte_JClass, JPy_Byte_ValueOf_SMID, value, objectRef);
}

int JType_CreateJavaShortObject(JNIEnv* jenv, JPy_JType* type, PyObject* pyArg, jobject* objectRef)
{
jvalue value;
jshort value;
if (JPy_IS_CLONG(pyArg)) {
value.s = JPy_AS_JSHORT(pyArg);
value = JPy_AS_JSHORT(pyArg);
} else {
return JType_PythonToJavaConversionError(type, pyArg);
}
return JType_CreateJavaObject(jenv, type, pyArg, JPy_Short_JClass, JPy_Short_Init_MID, value, objectRef);
JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Short_JClass, JPy_Short_ValueOf_SMID, value, objectRef);
}

int JType_CreateJavaNumberFromPythonInt(JNIEnv* jenv, JPy_JType* type, PyObject* pyArg, jobject* objectRef)
Expand All @@ -459,68 +470,66 @@ int JType_CreateJavaNumberFromPythonInt(JNIEnv* jenv, JPy_JType* type, PyObject*

if (i != j) {
value.j = j;
return JType_CreateJavaObject(jenv, type, pyArg, JPy_Long_JClass, JPy_Long_Init_MID, value, objectRef);
JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Long_JClass, JPy_Long_ValueOf_SMID, value.j, objectRef);
rbasralian marked this conversation as resolved.
Show resolved Hide resolved
}
if (s != i) {
value.i = i;
return JType_CreateJavaObject(jenv, type, pyArg, JPy_Integer_JClass, JPy_Integer_Init_MID, value,
objectRef);
JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Integer_JClass, JPy_Integer_ValueOf_SMID, value.i, objectRef);
}
if (b != s) {
value.s = s;
return JType_CreateJavaObject(jenv, type, pyArg, JPy_Short_JClass, JPy_Short_Init_MID, value,
objectRef);
JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Short_JClass, JPy_Short_ValueOf_SMID, value.s, objectRef);
}
value.b = b;
return JType_CreateJavaObject(jenv, type, pyArg, JPy_Byte_JClass, JPy_Byte_Init_MID, value, objectRef);
JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Byte_JClass, JPy_Byte_ValueOf_SMID, value.b, objectRef);
}

int JType_CreateJavaIntegerObject(JNIEnv* jenv, JPy_JType* type, PyObject* pyArg, jobject* objectRef)
{
jvalue value;
jint value;
if (JPy_IS_CLONG(pyArg)) {
value.i = JPy_AS_JINT(pyArg);
value = JPy_AS_JINT(pyArg);
} else {
return JType_PythonToJavaConversionError(type, pyArg);
}
return JType_CreateJavaObject(jenv, type, pyArg, JPy_Integer_JClass, JPy_Integer_Init_MID, value, objectRef);
JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Integer_JClass, JPy_Integer_ValueOf_SMID, value, objectRef);
}

int JType_CreateJavaLongObject(JNIEnv* jenv, JPy_JType* type, PyObject* pyArg, jobject* objectRef)
{
jvalue value;
jlong value;
if (JPy_IS_CLONG(pyArg)) {
value.j = JPy_AS_JLONG(pyArg);
value = JPy_AS_JLONG(pyArg);
} else {
return JType_PythonToJavaConversionError(type, pyArg);
}
return JType_CreateJavaObject(jenv, type, pyArg, JPy_Long_JClass, JPy_Long_Init_MID, value, objectRef);
JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Long_JClass, JPy_Long_ValueOf_SMID, value, objectRef);
}

int JType_CreateJavaFloatObject(JNIEnv* jenv, JPy_JType* type, PyObject* pyArg, jobject* objectRef)
{
jvalue value;
jfloat value;
if (JPy_IS_CLONG(pyArg)) {
value.f = (jfloat) JPy_AS_JLONG(pyArg);
value = (jfloat) JPy_AS_JLONG(pyArg);
} else if (PyFloat_Check(pyArg)) {
value.f = JPy_AS_JFLOAT(pyArg);
value = JPy_AS_JFLOAT(pyArg);
} else {
return JType_PythonToJavaConversionError(type, pyArg);
}
return JType_CreateJavaObject(jenv, type, pyArg, JPy_Float_JClass, JPy_Float_Init_MID, value, objectRef);
JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Float_JClass, JPy_Float_ValueOf_SMID, value, objectRef);
}

int JType_CreateJavaDoubleObject(JNIEnv* jenv, JPy_JType* type, PyObject* pyArg, jobject* objectRef)
{
jvalue value;
jdouble value;
if (JPy_IS_CLONG(pyArg)) {
value.d = (jdouble) JPy_AS_JLONG(pyArg);
value = (jdouble) JPy_AS_JLONG(pyArg);
} else if (PyFloat_Check(pyArg)) {
value.d = JPy_AS_JDOUBLE(pyArg);
value = JPy_AS_JDOUBLE(pyArg);
} else {
return JType_PythonToJavaConversionError(type, pyArg);
}
return JType_CreateJavaObject(jenv, type, pyArg, JPy_Double_JClass, JPy_Double_Init_MID, value, objectRef);
JType_CallStaticObjectMethodPrimitiveArgAndReturn(JPy_Double_JClass, JPy_Double_ValueOf_SMID, value, objectRef);
}

int JType_CreateJavaPyObject(JNIEnv* jenv, JPy_JType* type, PyObject* pyArg, jobject* objectRef)
Expand Down
24 changes: 24 additions & 0 deletions src/main/c/jpy_module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,29 +194,37 @@ jclass JPy_StopIteration_JClass = NULL;
// java.lang.Boolean
jclass JPy_Boolean_JClass = NULL;
jmethodID JPy_Boolean_Init_MID = NULL;
jmethodID JPy_Boolean_ValueOf_SMID = NULL;
jmethodID JPy_Boolean_BooleanValue_MID = NULL;

jclass JPy_Character_JClass = NULL;
jmethodID JPy_Character_Init_MID;
jmethodID JPy_Character_ValueOf_SMID;
jmethodID JPy_Character_CharValue_MID = NULL;

jclass JPy_Byte_JClass = NULL;
jmethodID JPy_Byte_Init_MID = NULL;
jmethodID JPy_Byte_ValueOf_SMID = NULL;

jclass JPy_Short_JClass = NULL;
jmethodID JPy_Short_Init_MID = NULL;
jmethodID JPy_Short_ValueOf_SMID = NULL;

jclass JPy_Integer_JClass = NULL;
jmethodID JPy_Integer_Init_MID = NULL;
jmethodID JPy_Integer_ValueOf_SMID = NULL;

jclass JPy_Long_JClass = NULL;
jmethodID JPy_Long_Init_MID = NULL;
jmethodID JPy_Long_ValueOf_SMID = NULL;

jclass JPy_Float_JClass = NULL;
jmethodID JPy_Float_Init_MID = NULL;
jmethodID JPy_Float_ValueOf_SMID = NULL;

jclass JPy_Double_JClass = NULL;
jmethodID JPy_Double_Init_MID = NULL;
jmethodID JPy_Double_ValueOf_SMID = NULL;

// java.lang.Number
jclass JPy_Number_JClass = NULL;
Expand Down Expand Up @@ -891,31 +899,39 @@ int JPy_InitGlobalVars(JNIEnv* jenv)

DEFINE_CLASS(JPy_Boolean_JClass, "java/lang/Boolean");
DEFINE_METHOD(JPy_Boolean_Init_MID, JPy_Boolean_JClass, "<init>", "(Z)V");
DEFINE_STATIC_METHOD(JPy_Boolean_ValueOf_SMID, JPy_Boolean_JClass, "valueOf", "(Z)Ljava/lang/Boolean;");
DEFINE_METHOD(JPy_Boolean_BooleanValue_MID, JPy_Boolean_JClass, "booleanValue", "()Z");

DEFINE_CLASS(JPy_Character_JClass, "java/lang/Character");
DEFINE_METHOD(JPy_Character_Init_MID, JPy_Character_JClass, "<init>", "(C)V");
DEFINE_STATIC_METHOD(JPy_Character_ValueOf_SMID, JPy_Character_JClass, "valueOf", "(C)Ljava/lang/Character;");
DEFINE_METHOD(JPy_Character_CharValue_MID, JPy_Character_JClass, "charValue", "()C");

DEFINE_CLASS(JPy_Number_JClass, "java/lang/Number");

DEFINE_CLASS(JPy_Byte_JClass, "java/lang/Byte");
DEFINE_METHOD(JPy_Byte_Init_MID, JPy_Byte_JClass, "<init>", "(B)V");
DEFINE_STATIC_METHOD(JPy_Byte_ValueOf_SMID, JPy_Byte_JClass, "valueOf", "(B)Ljava/lang/Byte;");

DEFINE_CLASS(JPy_Short_JClass, "java/lang/Short");
DEFINE_METHOD(JPy_Short_Init_MID, JPy_Short_JClass, "<init>", "(S)V");
DEFINE_STATIC_METHOD(JPy_Short_ValueOf_SMID, JPy_Short_JClass, "valueOf", "(S)Ljava/lang/Short;");

DEFINE_CLASS(JPy_Integer_JClass, "java/lang/Integer");
DEFINE_METHOD(JPy_Integer_Init_MID, JPy_Integer_JClass, "<init>", "(I)V");
DEFINE_STATIC_METHOD(JPy_Integer_ValueOf_SMID, JPy_Integer_JClass, "valueOf", "(I)Ljava/lang/Integer;");

DEFINE_CLASS(JPy_Long_JClass, "java/lang/Long");
DEFINE_METHOD(JPy_Long_Init_MID, JPy_Long_JClass, "<init>", "(J)V");
DEFINE_STATIC_METHOD(JPy_Long_ValueOf_SMID, JPy_Long_JClass, "valueOf", "(J)Ljava/lang/Long;");

DEFINE_CLASS(JPy_Float_JClass, "java/lang/Float");
DEFINE_METHOD(JPy_Float_Init_MID, JPy_Float_JClass, "<init>", "(F)V");
DEFINE_STATIC_METHOD(JPy_Float_ValueOf_SMID, JPy_Float_JClass, "valueOf", "(F)Ljava/lang/Float;");

DEFINE_CLASS(JPy_Double_JClass, "java/lang/Double");
DEFINE_METHOD(JPy_Double_Init_MID, JPy_Double_JClass, "<init>", "(D)V");
DEFINE_STATIC_METHOD(JPy_Double_ValueOf_SMID, JPy_Double_JClass, "valueOf", "(D)Ljava/lang/Double;");

DEFINE_CLASS(JPy_Number_JClass, "java/lang/Number");
DEFINE_METHOD(JPy_Number_IntValue_MID, JPy_Number_JClass, "intValue", "()I");
Expand Down Expand Up @@ -1037,15 +1053,23 @@ void JPy_ClearGlobalVars(JNIEnv* jenv)
JPy_Field_GetModifiers_MID = NULL;
JPy_Field_GetType_MID = NULL;
JPy_Boolean_Init_MID = NULL;
JPy_Boolean_ValueOf_SMID = NULL;
JPy_Boolean_BooleanValue_MID = NULL;
JPy_Character_Init_MID = NULL;
JPy_Character_ValueOf_SMID = NULL;
JPy_Character_CharValue_MID = NULL;
JPy_Byte_Init_MID = NULL;
JPy_Short_Init_MID = NULL;
JPy_Integer_Init_MID = NULL;
JPy_Long_Init_MID = NULL;
JPy_Float_Init_MID = NULL;
JPy_Double_Init_MID = NULL;
JPy_Byte_ValueOf_SMID = NULL;
JPy_Short_ValueOf_SMID = NULL;
JPy_Integer_ValueOf_SMID = NULL;
JPy_Long_ValueOf_SMID = NULL;
JPy_Float_ValueOf_SMID = NULL;
JPy_Double_ValueOf_SMID = NULL;
JPy_Number_IntValue_MID = NULL;
JPy_Number_LongValue_MID = NULL;
JPy_Number_DoubleValue_MID = NULL;
Expand Down
8 changes: 8 additions & 0 deletions src/main/c/jpy_module.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,31 +216,39 @@ extern jclass JPy_StopIteration_JClass;

extern jclass JPy_Boolean_JClass;
extern jmethodID JPy_Boolean_Init_MID;
extern jmethodID JPy_Boolean_ValueOf_SMID;
extern jmethodID JPy_Boolean_BooleanValue_MID;

extern jclass JPy_Character_JClass;
extern jmethodID JPy_Character_Init_MID;
extern jmethodID JPy_Character_ValueOf_SMID;
extern jmethodID JPy_Character_CharValue_MID;

extern jclass JPy_Number_JClass;

extern jclass JPy_Byte_JClass;
extern jmethodID JPy_Byte_Init_MID;
extern jmethodID JPy_Byte_ValueOf_SMID;

extern jclass JPy_Short_JClass;
extern jmethodID JPy_Short_Init_MID;
extern jmethodID JPy_Short_ValueOf_SMID;

extern jclass JPy_Integer_JClass;
extern jmethodID JPy_Integer_Init_MID;
extern jmethodID JPy_Integer_ValueOf_SMID;

extern jclass JPy_Long_JClass;
extern jmethodID JPy_Long_Init_MID;
extern jmethodID JPy_Long_ValueOf_SMID;

extern jclass JPy_Float_JClass;
extern jmethodID JPy_Float_Init_MID;
extern jmethodID JPy_Float_ValueOf_SMID;

extern jclass JPy_Double_JClass;
extern jmethodID JPy_Double_Init_MID;
extern jmethodID JPy_Double_ValueOf_SMID;

extern jclass JPy_Number_JClass;
extern jmethodID JPy_Number_IntValue_MID;
Expand Down