Reimplement `GlobalRef` #67

alexander-irbis · 2018-01-08T14:23:32Z

Description of the problem: #66

This PR depends on invocation_api branch.

The current implementation assumes the need to maintain the previous implementation, but introduces breaking changes: old GlobalRef is renamed to AttachedGlobalRef, old module global_ref is renamed to global_ref_ad, and old JNIEnv::new_global_ref() is renamed to JNIEnv::new_global_ref_attached(). I have no answer so far whether it is necessary. It seems to me that it is better to completely remove AttachedGlobalRef.

The current JNIEnv implementation assumes only a hierarchical execution order, and this also imposes some restrictions on the new implementation of GlobalRef. But this issue can be solved separately in a subsequent PR and after discussion.

What other tests to add? Ideas are welcome.

export LD_LIBRARY_PATH=`java -XshowSettings:properties -version 2>&1 > /dev/null | grep 'java.home' | sed 's/^.*= //'`/lib/server/:$LD_LIBRARY_PATH

And then

cargo test --features="invocation"

or

cargo test --all-features

alexander-irbis · 2018-01-08T14:49:58Z

src/wrapper/jnienv.rs

    /// Turns an object into a global ref. This has the benefit of removing the
    /// lifetime bounds since it's guaranteed to not get GC'd by java. It
    /// releases the GC pin upon being dropped.
    pub fn new_global_ref(&self, obj: JObject) -> Result<GlobalRef> {
        non_null!(obj, "new_global_ref obj argument");
        let new_ref: JObject = jni_call!(self.internal, NewGlobalRef, obj.into_inner());
-        let global = unsafe { GlobalRef::new(self.internal, new_ref.into_inner()) };
+        let global = unsafe { GlobalRef::new(self.get_java_vm()?, new_ref.into_inner()) };


Now, a JNI Call is made here. But is it necessary? Perhaps in the future JNIEnv will just have a cached reference to JavaVM

I would assume that an underlying JNIEnv has a pointer to JavaVM, therefore, the cost is likely to be negligible.

In practice, this call is executed one and a half times longer than creating + deleting a copy of Arc. We can really neglect it for now.

test tests::jni_get_java_vm ... bench: 1,789 ns/iter (+/- 431) test tests::native_arc ... bench: 1,138 ns/iter (+/- 269)

dmitry-timofeev · 2018-01-08T15:34:10Z

src/wrapper/jnienv.rs

    /// Turns an object into a global ref. This has the benefit of removing the
    /// lifetime bounds since it's guaranteed to not get GC'd by java. It
    /// releases the GC pin upon being dropped.
    pub fn new_global_ref(&self, obj: JObject) -> Result<GlobalRef> {
        non_null!(obj, "new_global_ref obj argument");
        let new_ref: JObject = jni_call!(self.internal, NewGlobalRef, obj.into_inner());
-        let global = unsafe { GlobalRef::new(self.internal, new_ref.into_inner()) };
+        let global = unsafe { GlobalRef::new(self.get_java_vm()?, new_ref.into_inner()) };


I would assume that an underlying JNIEnv has a pointer to JavaVM, therefore, the cost is likely to be negligible.

dmitry-timofeev · 2018-01-08T15:37:54Z

src/wrapper/objects/global_ref.rs

-    pub unsafe fn new(env: *mut sys::JNIEnv, obj: jobject) -> Self {
+    /// Creates a new global reference. This assumes that `NewGlobalRef`
+    /// has already been called.
+    pub unsafe fn new(vm: JavaVM, obj: sys::jobject) -> Self {


Am I right to assume this method is not supposed to be used by the library clients? If so, is it possible in Rust to make it available only inside the library (= hide from the users)?

I agree with this, we can make such methods visible only for this crate itself.

dmitry-timofeev · 2018-01-08T15:52:38Z

tests/jni_global_refs.rs

+            barrier.wait();
+            for _ in 0..10000 {
+                unwrap(&env, unwrap(&env, env.call_method(
+                    atomic_integer.as_obj(), "addAndGet", "(I)I", &[JValue::from(-1)])).i());


decrementAndGet here, and incrementAndGet below?

dmitry-timofeev · 2018-01-08T16:01:02Z

tests/jni_global_refs.rs

+
+    let global_ref_1 = unwrap(&env, env.new_global_ref_attached(local_ref.as_obj()));
+
+    {


Since there are multiple asserts and conversions between various reference types, I'd add comments clarifying what you are trying to test, e.g.

// Test several global refs to the same object work … // Test detached & re-attached global ref works … // Test the first global ref unaffected by another gr to the same object detached

It seemed to me that this was too obvious, but ok, let it be

dmitry-timofeev · 2018-01-08T16:02:21Z

tests/jni_global_refs.rs

+}
+
+#[test]
+pub fn global_ref_works_in_other_threads() {


I'd simplify the test to make it easier to understand by reviewers & future maintainers:

Use incrementAndGet in both threads.

Extract a magic 10000 in a constant (e.g., incrementsInEachThread, or numIncrements).

Use the same constant in both loops (no 10000 + 1, because that is likely to confuse readers).

assert that the resulting value is equal to the number of threads (2) multiplied by the number of increments.

Everything went to this, but I somehow forgot to finish it.

dmitry-timofeev · 2018-01-08T16:13:27Z

I wonder what is the deprecation policy:

Shall the old GlobalRef simply go away?
Shall it be deprecated first with a #[deprecated(since = "x.y.z", note="Use NewGlobalRef")] and retain its original name so that the existing users are not affected, and then be turned into NewGlobalRef (old GlobalRef removed + NewGlobalRef renamed into GlobalRef)?
Shall it be deprecated but get a new name (like in this PR)?

@jechase, @alexander-irbis, @DarkEld3r , @fpoli, what do you think? I think the (2) is the most user-friendly, but requires a little bit more work, the (1) is the easiest.

Restricted the visibility of `GlobalRef::new()`. Updated tests. Added comments.

dmitry-timofeev

LGTM.

Regarding some extra tests, I can think of tests verifying that drop works as expected:

When the current native thread is attached, it remains attached after GlobalRef#drop finishes, and other references to the same Java object (e.g., local or system) are not usable. AFAIK, that will require little hacks to suppress lifetime checker and make JObject outlive GlobalRef.
When the current native thread is detached, it remains detached, the reference becomes unusable.

dmitry-timofeev · 2018-01-09T08:45:22Z

src/wrapper/objects/global_ref.rs

+        let res = match self.vm.get_env() {
+            Ok(env) => drop_impl(&env, self.as_obj()),
+            Err(_) => self.vm
+                .attach_current_thread()


Please consider #69

dmitry-timofeev · 2018-01-09T08:45:55Z

src/wrapper/objects/global_ref_ad.rs

+            }
+            Err(Error(ErrorKind::ThreadDetached, _)) => {
+                let env = self.vm.attach_current_thread()?;
+                let _ = self.attach_impl(&env);


Please consider #69

fpoli · 2018-01-09T09:30:57Z

I wonder what is the deprecation policy:

Shall the old GlobalRef simply go away?

Shall it be deprecated first with a #[deprecated(since = "x.y.z", note="Use NewGlobalRef")] and retain its original name so that the existing users are not affected, and then be turned into NewGlobalRef (old GlobalRef removed + NewGlobalRef renamed into GlobalRef)?

Shall it be deprecated but get a new name (like in this PR)?

@jechase, @alexander-irbis, @DarkEld3r , @fpoli, what do you think? I think the (2) is the most user-friendly, but requires a little bit more work, the (1) is the easiest.

I agree. In my opinion it's ok to just go with (1) and a (major?) version change.

alexander-irbis · 2018-01-09T11:35:12Z

When the current native thread is detached, it remains detached, the reference becomes unusable.

@dmitry-timofeev currently, references unusable without JNIEnv object. This object keeps a thread attachment and provides access to JNI calls. So, it's not possible to use references without an attachment. In other words: it is tested by a type system.

…ni-rs#69

dmitry-timofeev · 2018-01-09T13:20:23Z

… In other words: it is tested by a type system.

You are right, the second test doesn't make much sense with the second reference being a local one.

stanislav-tkach · 2018-01-09T13:58:13Z

src/wrapper/objects/mod.rs

@@ -36,6 +36,10 @@ pub use self::jbytebuffer::*;
 mod global_ref;
 pub use self::global_ref::*;

+// For when you want to store a reference to a java object


"For storing a reference to a java object" if you want the same comment style as below. I'm not sure if it is needed at all.

dmitry-timofeev · 2018-01-09T14:11:39Z

src/wrapper/objects/global_ref.rs

+        let res = match self.vm.get_env() {
+            Ok(env) => drop_impl(&env, self.as_obj()),
+            Err(_) => {
+                warn!("`GlobalRef` attaches JNI-thread while dropping its instance.");


Since this message is for (potentially) ignorant users, I'd be more verbose and suggest what they shall do:
Dropping a GlobalRef in a detached thread. Fix your code if this message appears frequently (see the GlobalRef docs)

Currently it's unclear from the message whether it shall concern a user at all.

dmitry-timofeev · 2018-01-09T14:21:06Z

src/wrapper/objects/global_ref.rs

-/// since it requires a pointer to the `JNIEnv` to do anything useful with it.
+/// outlive the `JNIEnv` that it came from and can be used in other threads.
+///
+/// Note that the native thread must be `attach`ed to the jni thread (this is so when you have


It is _recommended_ that a native thread that drops the global reference is attached to the Java thread (i.e., has an instance of `JNIEnv`). If the native thread is *not* attached, the `GlobalRef#drop` will print a warning and implicitly `attach` and `detach` it, which significantly affects performance.
?

jrobsonchase · 2018-01-09T21:15:35Z

I'm good with removing the old GlobalRef and making a new semver-incompatible version. It was going to need to happen for the invocation api branch anyway.

jrobsonchase · 2018-01-09T21:27:31Z

If this is ready to go in, I can go ahead and merge it and the invocation api branch, assuming there's nothing major that I've overlooked.

Reimplemented GlobalRef (jni-rs#66)

0a22954

alexander-irbis changed the base branch from master to invocation_api January 8, 2018 14:35

alexander-irbis commented Jan 8, 2018

View reviewed changes

dmitry-timofeev requested changes Jan 8, 2018

View reviewed changes

Fixed review issues.

7210a5f

Restricted the visibility of `GlobalRef::new()`. Updated tests. Added comments.

dmitry-timofeev approved these changes Jan 9, 2018

View reviewed changes

dmitry-timofeev mentioned this pull request Jan 9, 2018

Warn a user when a GlobalRef is dropped with no thread attached #69

Closed

dmitry-timofeev reviewed Jan 9, 2018

View reviewed changes

Added a warning on dropping GlobalRef in a detached state. Closes j…

95cd0a5

…ni-rs#69

stanislav-tkach reviewed Jan 9, 2018

View reviewed changes

dmitry-timofeev reviewed Jan 9, 2018

View reviewed changes

Fixed comments, warnings, and docs.

890ae00

dmitry-timofeev approved these changes Jan 9, 2018

View reviewed changes

stanislav-tkach approved these changes Jan 9, 2018

View reviewed changes

Removed AttachedGlobalRef and DetachedGlobalRef

ee94216

dmitry-timofeev approved these changes Jan 10, 2018

View reviewed changes

jrobsonchase merged commit 95214d5 into jni-rs:invocation_api Jan 10, 2018

alexander-irbis changed the title ~~[WIP] Reimplement GlobalRef~~ Reimplement GlobalRef Jan 10, 2018

alexander-irbis mentioned this pull request Jan 10, 2018

A test to ensure DeleteGlobalRef is called. #70

Merged

nbaksalyar mentioned this pull request Mar 13, 2018

Fix DetachGlobalRef usage in the async example #85

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement `GlobalRef` #67

Reimplement `GlobalRef` #67

alexander-irbis commented Jan 8, 2018 •

edited

alexander-irbis Jan 8, 2018

dmitry-timofeev Jan 8, 2018

alexander-irbis Jan 8, 2018

dmitry-timofeev Jan 8, 2018

dmitry-timofeev Jan 8, 2018

alexander-irbis Jan 8, 2018

dmitry-timofeev Jan 8, 2018

dmitry-timofeev Jan 8, 2018

alexander-irbis Jan 8, 2018

dmitry-timofeev Jan 8, 2018

alexander-irbis Jan 8, 2018

dmitry-timofeev commented Jan 8, 2018

dmitry-timofeev left a comment

dmitry-timofeev Jan 9, 2018

dmitry-timofeev Jan 9, 2018

fpoli commented Jan 9, 2018 •

edited

alexander-irbis commented Jan 9, 2018 •

edited

dmitry-timofeev commented Jan 9, 2018

stanislav-tkach Jan 9, 2018

dmitry-timofeev Jan 9, 2018

dmitry-timofeev Jan 9, 2018

jrobsonchase commented Jan 9, 2018

jrobsonchase commented Jan 9, 2018


		let global_ref_1 = unwrap(&env, env.new_global_ref_attached(local_ref.as_obj()));

		{

Reimplement GlobalRef #67

Reimplement GlobalRef #67

Conversation

alexander-irbis commented Jan 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-timofeev commented Jan 8, 2018

dmitry-timofeev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpoli commented Jan 9, 2018 • edited

alexander-irbis commented Jan 9, 2018 • edited

dmitry-timofeev commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrobsonchase commented Jan 9, 2018

jrobsonchase commented Jan 9, 2018

Reimplement `GlobalRef` #67

Reimplement `GlobalRef` #67

alexander-irbis commented Jan 8, 2018 •

edited

fpoli commented Jan 9, 2018 •

edited

alexander-irbis commented Jan 9, 2018 •

edited