-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
845d360
to
9634210
Compare
@jfirebaugh I'm having some issues compiling this locally. Tried with ndk 16.1.4479499 and 17.1.4828580, but get the below error (for arm-v7 and v8a). Verified that master does compile.
|
I had just rebased the branch and there were some commits that conflicted. Try now. |
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.
👍 Cleans up nicely.
I couldn't verify the changes on older hardware due to #12762.
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.
Hacking around the issue in #12762 it seems that the new SeizeLocal
is not working out quite like the former DeleteLocalRef
approach.
08-29 11:50:06.913 8331-8387/com.mapbox.mapboxsdk.testapp E/dalvikvm: JNI ERROR (app bug): local reference table overflow (max=512)
08-29 11:50:06.913 8331-8387/com.mapbox.mapboxsdk.testapp W/dalvikvm: JNI local reference table (0x60d43ec8) dump:
Last 10 entries (of 512):
511: 0x417df2a0 java.lang.Class<android.util.Log>
510: 0x425c5bc8 java.lang.String "{Thread-575}[Ope... (38 chars)
509: 0x425c5b88 java.lang.String "Mbgl"
508: 0x425c5af0 java.lang.String "{Thread-575}[Ope... (38 chars)
507: 0x425c5ab0 java.lang.String "Mbgl"
506: 0x425c5a18 java.lang.String "{Thread-575}[Ope... (38 chars)
505: 0x425c59d8 java.lang.String "Mbgl"
504: 0x425c5940 java.lang.String "{Thread-575}[Ope... (38 chars)
503: 0x425c5900 java.lang.String "Mbgl"
502: 0x425c5868 java.lang.String "{Thread-575}[Ope... (38 chars)
Summary:
1 of java.lang.Class
510 of java.lang.String (510 unique instances)
1 of com.mapbox.mapboxsdk.maps.MapView$5
@jfirebaugh Above error is reproducible on a Galaxy Nexus with Android 4.3. This device has the bare minimum for reference tables. |
Thanks @ivovandongen, I'll take a look. Same repro steps as in #12762? |
Looks like that overflow is caused by d297bf1#diff-f0e8cf5a32c5a4047cdc6945b9fab894, rather than by these changes. Given how easy it is to leak local refs, I think we should go ahead with mapbox/jni.hpp#32. I'll look at doing this before the jni.hpp 4.0.0 final release. |
This method seems to be called in a loop, leading to a local reference table overflow if not explicitly deleted. See #12716 (review)
This method seems to be called in a loop, leading to a local reference table overflow if not explicitly deleted. See #12716 (review)
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.
loving the new syntax, less error prone, lgtm
d53f1eb
to
4272c5b
Compare
4272c5b
to
9cd50a8
Compare
Issues addressed and Ivo's on vacation.
Hi, I got urgent might solved by this PR. Since it is already merged wondering when it will be released? |
@lekaha This change will be part of the next release, which is tentatively scheduled for mid-October. |
Upgrade to jni.hpp 4.0.0-rc8. The main changes were:
jni::DeleteLocalRef
with use ofjni::Local<T>
/jni::SeizeLocal
.jni::Class<T>::Singleton()
, replacingjni::Class<T>::Find(env).NewGlobalRef(env).release()
.jni::Class
is also no longer reassignable, soSingleton
gets used heavily. One gotcha is that we still need to make sure it's called in the main thread during registration, becauseFindClass
will fail if run from a background thread.jni::Cast
.Binary size increases slightly, but I'm working on followups to gain this back.
I ran all the instrumented tests locally. Let me know if there are additional tests I should run.
Fixes #9688 -- we never attempt to delete a non-local reference
Fixes #11556 -- not using weak global references anymore
Fixes #11510