This is an automated email from the ASF dual-hosted git repository.
viirya pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion-comet.git
The following commit(s) were added to refs/heads/main by this push:
new 9f44a74a refactor: Remove method get_global_jclass (#580)
9f44a74a is described below
commit 9f44a74acee322122d4587422deff6526bfc9d92
Author: Emil Ejbyfeldt <[email protected]>
AuthorDate: Fri Jun 21 08:35:16 2024 +0200
refactor: Remove method get_global_jclass (#580)
* refactor: Remove method get_global_jclass
The method uses unsafe code to cast a value to a static lifetime using
std::mem::transmute. But none of users of the method actually seems to
depend on this lifetime extention. So to me it seems better to just call
the underlying `find_class` method and delete the unsafe code.
* Apply suggestions from code review
Remove no longer needed comments.
Co-authored-by: Liang-Chi Hsieh <[email protected]>
---------
Co-authored-by: Liang-Chi Hsieh <[email protected]>
---
core/src/jvm_bridge/batch_iterator.rs | 4 +---
core/src/jvm_bridge/comet_exec.rs | 5 +----
core/src/jvm_bridge/comet_metric_node.rs | 5 +----
core/src/jvm_bridge/comet_task_memory_manager.rs | 4 +---
core/src/jvm_bridge/mod.rs | 18 ++----------------
5 files changed, 6 insertions(+), 30 deletions(-)
diff --git a/core/src/jvm_bridge/batch_iterator.rs
b/core/src/jvm_bridge/batch_iterator.rs
index 474e4fdc..f6f5e06f 100644
--- a/core/src/jvm_bridge/batch_iterator.rs
+++ b/core/src/jvm_bridge/batch_iterator.rs
@@ -15,7 +15,6 @@
// specific language governing permissions and limitations
// under the License.
-use super::get_global_jclass;
use jni::{
errors::Result as JniResult,
objects::{JClass, JMethodID},
@@ -34,8 +33,7 @@ impl<'a> CometBatchIterator<'a> {
pub const JVM_CLASS: &'static str = "org/apache/comet/CometBatchIterator";
pub fn new(env: &mut JNIEnv<'a>) -> JniResult<CometBatchIterator<'a>> {
- // Get the global class reference
- let class = get_global_jclass(env, Self::JVM_CLASS)?;
+ let class = env.find_class(Self::JVM_CLASS)?;
Ok(CometBatchIterator {
class,
diff --git a/core/src/jvm_bridge/comet_exec.rs
b/core/src/jvm_bridge/comet_exec.rs
index 6b6652eb..9d4a76eb 100644
--- a/core/src/jvm_bridge/comet_exec.rs
+++ b/core/src/jvm_bridge/comet_exec.rs
@@ -22,8 +22,6 @@ use jni::{
JNIEnv,
};
-use super::get_global_jclass;
-
/// A struct that holds all the JNI methods and fields for JVM CometExec
object.
pub struct CometExec<'a> {
pub class: JClass<'a>,
@@ -55,8 +53,7 @@ impl<'a> CometExec<'a> {
pub const JVM_CLASS: &'static str =
"org/apache/spark/sql/comet/CometScalarSubquery";
pub fn new(env: &mut JNIEnv<'a>) -> JniResult<CometExec<'a>> {
- // Get the global class reference
- let class = get_global_jclass(env, Self::JVM_CLASS)?;
+ let class = env.find_class(Self::JVM_CLASS)?;
Ok(CometExec {
method_get_bool: env
diff --git a/core/src/jvm_bridge/comet_metric_node.rs
b/core/src/jvm_bridge/comet_metric_node.rs
index d0176f42..821dc3ff 100644
--- a/core/src/jvm_bridge/comet_metric_node.rs
+++ b/core/src/jvm_bridge/comet_metric_node.rs
@@ -22,8 +22,6 @@ use jni::{
JNIEnv,
};
-use super::get_global_jclass;
-
/// A struct that holds all the JNI methods and fields for JVM CometMetricNode
class.
pub struct CometMetricNode<'a> {
pub class: JClass<'a>,
@@ -37,8 +35,7 @@ impl<'a> CometMetricNode<'a> {
pub const JVM_CLASS: &'static str =
"org/apache/spark/sql/comet/CometMetricNode";
pub fn new(env: &mut JNIEnv<'a>) -> JniResult<CometMetricNode<'a>> {
- // Get the global class reference
- let class = get_global_jclass(env, Self::JVM_CLASS)?;
+ let class = env.find_class(Self::JVM_CLASS)?;
Ok(CometMetricNode {
method_get_child_node: env
diff --git a/core/src/jvm_bridge/comet_task_memory_manager.rs
b/core/src/jvm_bridge/comet_task_memory_manager.rs
index a79a5b67..97d1bf3a 100644
--- a/core/src/jvm_bridge/comet_task_memory_manager.rs
+++ b/core/src/jvm_bridge/comet_task_memory_manager.rs
@@ -22,8 +22,6 @@ use jni::{
JNIEnv,
};
-use crate::jvm_bridge::get_global_jclass;
-
/// A wrapper which delegate acquire/release memory calls to the
/// JVM side `CometTaskMemoryManager`.
#[derive(Debug)]
@@ -40,7 +38,7 @@ impl<'a> CometTaskMemoryManager<'a> {
pub const JVM_CLASS: &'static str =
"org/apache/spark/CometTaskMemoryManager";
pub fn new(env: &mut JNIEnv<'a>) -> JniResult<CometTaskMemoryManager<'a>> {
- let class = get_global_jclass(env, Self::JVM_CLASS)?;
+ let class = env.find_class(Self::JVM_CLASS)?;
let result = CometTaskMemoryManager {
class,
diff --git a/core/src/jvm_bridge/mod.rs b/core/src/jvm_bridge/mod.rs
index 3f61c032..5d810edb 100644
--- a/core/src/jvm_bridge/mod.rs
+++ b/core/src/jvm_bridge/mod.rs
@@ -20,8 +20,8 @@
use crate::errors::CometResult;
use jni::{
- errors::{Error, Result as JniResult},
- objects::{JClass, JMethodID, JObject, JString, JThrowable, JValueGen,
JValueOwned},
+ errors::Error,
+ objects::{JMethodID, JObject, JString, JThrowable, JValueGen, JValueOwned},
signature::ReturnType,
AttachGuard, JNIEnv,
};
@@ -176,20 +176,6 @@ pub(crate) use jni_new_string;
pub(crate) use jni_static_call;
pub(crate) use jvalues;
-/// Gets a global reference to a Java class.
-pub fn get_global_jclass(env: &mut JNIEnv, cls: &str) ->
JniResult<JClass<'static>> {
- let local_jclass = env.find_class(cls)?;
- let global = env.new_global_ref::<JObject>(local_jclass.into())?;
-
- // A hack to make the `JObject` static. This is safe because the global
reference is never
- // gc-ed by the JVM before dropping the global reference.
- let global_obj = unsafe { std::mem::transmute::<_,
JObject<'static>>(global.as_obj()) };
- // Prevent the global reference from being dropped.
- let _ = std::mem::ManuallyDrop::new(global);
-
- Ok(JClass::from(global_obj))
-}
-
mod comet_exec;
pub use comet_exec::*;
mod batch_iterator;
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
