Issue 29345737: Issue 4146 - Fix failing UpdateCheckTest.testApplicationUpdateAvailable test in libadblockplus-andr… (Closed)
|
Created:
June 10, 2016, 1:33 p.m. by anton Modified:
Sept. 16, 2016, 11:06 a.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 4146 - Fix failing UpdateCheckTest.testApplicationUpdateAvailable test in libadblockplus-andr…
#depends on https://codereview.adblockplus.org/29344967/
Patch Set 1 #Patch Set 2 : moved entry points to JniLibrary.cpp #
Total comments: 11
Patch Set 3 : codestyle, moved unload #
Total comments: 2
Patch Set 4 : using jni version define from Utils.h #
Total comments: 1
MessagesTotal messages: 14
This happens because new thread is created and it's neither attached not jclass references are precached. This is clearly explained on android jni tips page: https://developer.android.com/training/articles/perf-jni.html#faq_FindClass Now introducing precaching of JsVale class reference as global reference which leads to: 1) improved performance while creating JsValue instance (no need to find class and ctor every time) 2) avoiding threading issues like this But we have to use JNI_OnLoad and JNI_OnUnload which can be not convenient (since it's only 1 function for the whole .so library) and if we need to precache something else we will have not good design here. tested with this fix and confirmed it's working: --- 06-10 09:16:11.066 7409-7409/org.adblockplus.libadblockplus.tests I/System.out﹕ EMMA: collecting runtime coverage data ... 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Trying to load lib /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so 0xb10427c8 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Added shared lib /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so 0xb10427c8 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Trying to load lib /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so 0xb10427c8 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Shared lib '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' already loaded in same CL 0xb10427c8 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests I/dalvikvm﹕ threadid=1: recursive native library load attempt (/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so) 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Trying to load lib /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so 0xb10427c8 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Shared lib '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' already loaded in same CL 0xb10427c8 06-10 09:16:11.076 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ started: testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Trying to load lib /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so 0xb10427c8 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Shared lib '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' already loaded in same CL 0xb10427c8 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Trying to load lib /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so 0xb10427c8 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Shared lib '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' already loaded in same CL 0xb10427c8 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Trying to load lib /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so 0xb10427c8 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Shared lib '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' already loaded in same CL 0xb10427c8 06-10 09:16:11.096 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Trying to load lib /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so 0xb10427c8 06-10 09:16:11.096 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ Shared lib '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' already loaded in same CL 0xb10427c8 06-10 09:16:11.176 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ finished: testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:11.176 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ passed: testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:11.186 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ started: testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ finished: testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ passed: testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ started: testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ finished: testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ passed: testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ started: testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ finished: testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ passed: testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ started: testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ finished: testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ passed: testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ started: testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:15.766 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ finished: testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:15.766 7409-7423/org.adblockplus.libadblockplus.tests I/TestRunner﹕ passed: testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) 06-10 09:16:15.766 7398-7398/? D/AndroidRuntime﹕ Shutting down VM
On 2016/06/10 13:44:42, anton wrote: > This happens because new thread is created and it's neither attached not jclass > references are precached. This is clearly explained on android jni tips page: > https://developer.android.com/training/articles/perf-jni.html#faq_FindClass > > Now introducing precaching of JsVale class reference as global reference which > leads to: > 1) improved performance while creating JsValue instance (no need to find class > and ctor every time) > 2) avoiding threading issues like this > > But we have to use JNI_OnLoad and JNI_OnUnload which can be not convenient > (since it's only 1 function for the whole .so library) and if we need to > precache something else we will have not good design here. > > tested with this fix and confirmed it's working: > --- > > 06-10 09:16:11.066 7409-7409/org.adblockplus.libadblockplus.tests > I/System.out﹕ EMMA: collecting runtime coverage data ... > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Trying to load lib > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > 0xb10427c8 > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Added shared lib > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > 0xb10427c8 > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Trying to load lib > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > 0xb10427c8 > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Shared lib > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > already loaded in same CL 0xb10427c8 > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests I/dalvikvm﹕ > threadid=1: recursive native library load attempt > (/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so) > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Trying to load lib > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > 0xb10427c8 > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Shared lib > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > already loaded in same CL 0xb10427c8 > 06-10 09:16:11.076 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ started: > testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Trying to load lib > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > 0xb10427c8 > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Shared lib > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > already loaded in same CL 0xb10427c8 > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Trying to load lib > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > 0xb10427c8 > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Shared lib > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > already loaded in same CL 0xb10427c8 > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Trying to load lib > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > 0xb10427c8 > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Shared lib > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > already loaded in same CL 0xb10427c8 > 06-10 09:16:11.096 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Trying to load lib > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > 0xb10427c8 > 06-10 09:16:11.096 7409-7423/org.adblockplus.libadblockplus.tests D/dalvikvm﹕ > Shared lib > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > already loaded in same CL 0xb10427c8 > 06-10 09:16:11.176 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ finished: > testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:11.176 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ passed: > testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:11.186 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ started: > testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ finished: > testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ passed: > testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ started: > testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ finished: > testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ passed: > testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ started: > testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ finished: > testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ passed: > testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ started: > testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ finished: > testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ passed: > testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ started: > testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:15.766 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ finished: > testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:15.766 7409-7423/org.adblockplus.libadblockplus.tests > I/TestRunner﹕ passed: > testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > 06-10 09:16:15.766 7398-7398/? D/AndroidRuntime﹕ Shutting down VM This solution is fine for me, but the cache logic and OnLoad/OnUnload functions should be moved to another file (e.g. JniLibrary) where it should keep the cached classes and all other logic that would be shared across the library
On 2016/09/08 19:39:40, diegocarloslima wrote: > On 2016/06/10 13:44:42, anton wrote: > > This happens because new thread is created and it's neither attached not > jclass > > references are precached. This is clearly explained on android jni tips page: > > https://developer.android.com/training/articles/perf-jni.html#faq_FindClass > > > > Now introducing precaching of JsVale class reference as global reference which > > leads to: > > 1) improved performance while creating JsValue instance (no need to find class > > and ctor every time) > > 2) avoiding threading issues like this > > > > But we have to use JNI_OnLoad and JNI_OnUnload which can be not convenient > > (since it's only 1 function for the whole .so library) and if we need to > > precache something else we will have not good design here. > > > > tested with this fix and confirmed it's working: > > --- > > > > 06-10 09:16:11.066 7409-7409/org.adblockplus.libadblockplus.tests > > I/System.out﹕ EMMA: collecting runtime coverage data ... > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Trying to load lib > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > 0xb10427c8 > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Added shared lib > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > 0xb10427c8 > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Trying to load lib > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > 0xb10427c8 > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Shared lib > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > already loaded in same CL 0xb10427c8 > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > I/dalvikvm﹕ > > threadid=1: recursive native library load attempt > > (/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so) > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Trying to load lib > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > 0xb10427c8 > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Shared lib > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > already loaded in same CL 0xb10427c8 > > 06-10 09:16:11.076 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ started: > > > testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Trying to load lib > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > 0xb10427c8 > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Shared lib > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > already loaded in same CL 0xb10427c8 > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Trying to load lib > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > 0xb10427c8 > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Shared lib > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > already loaded in same CL 0xb10427c8 > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Trying to load lib > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > 0xb10427c8 > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Shared lib > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > already loaded in same CL 0xb10427c8 > > 06-10 09:16:11.096 7409-7423/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Trying to load lib > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > 0xb10427c8 > > 06-10 09:16:11.096 7409-7423/org.adblockplus.libadblockplus.tests > D/dalvikvm﹕ > > Shared lib > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > already loaded in same CL 0xb10427c8 > > 06-10 09:16:11.176 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ finished: > > > testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:11.176 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ passed: > > > testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:11.186 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ started: > > > testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ finished: > > > testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ passed: > > > testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ started: > > testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ finished: > > testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ passed: > > testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ started: > > testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ finished: > > testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ passed: > > testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ started: > > testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ finished: > > testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ passed: > > testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ started: > > testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:15.766 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ finished: > > testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:15.766 7409-7423/org.adblockplus.libadblockplus.tests > > I/TestRunner﹕ passed: > > testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > 06-10 09:16:15.766 7398-7398/? D/AndroidRuntime﹕ Shutting down VM > > This solution is fine for me, but the cache logic and OnLoad/OnUnload functions > should be moved to another file (e.g. JniLibrary) where it should keep the > cached classes and all other logic that would be shared across the library added patch set #2. Moved global entry points ("JNI_OnLoad" and "JNI_OnUnload") to new file JniLibrary.cpp and referencing to needed function like "file_OnLoad" f.e. JniJsValue_OnLoad. All files which are interested in load/unload logics should provide methods and declare them in header.
On 2016/09/14 07:42:08, anton wrote: > On 2016/09/08 19:39:40, diegocarloslima wrote: > > On 2016/06/10 13:44:42, anton wrote: > > > This happens because new thread is created and it's neither attached not > > jclass > > > references are precached. This is clearly explained on android jni tips > page: > > > https://developer.android.com/training/articles/perf-jni.html#faq_FindClass > > > > > > Now introducing precaching of JsVale class reference as global reference > which > > > leads to: > > > 1) improved performance while creating JsValue instance (no need to find > class > > > and ctor every time) > > > 2) avoiding threading issues like this > > > > > > But we have to use JNI_OnLoad and JNI_OnUnload which can be not convenient > > > (since it's only 1 function for the whole .so library) and if we need to > > > precache something else we will have not good design here. > > > > > > tested with this fix and confirmed it's working: > > > --- > > > > > > 06-10 09:16:11.066 7409-7409/org.adblockplus.libadblockplus.tests > > > I/System.out﹕ EMMA: collecting runtime coverage data ... > > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Trying to load lib > > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > > 0xb10427c8 > > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Added shared lib > > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > > 0xb10427c8 > > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Trying to load lib > > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > > 0xb10427c8 > > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Shared lib > > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > > already loaded in same CL 0xb10427c8 > > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > > I/dalvikvm﹕ > > > threadid=1: recursive native library load attempt > > > (/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so) > > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Trying to load lib > > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > > 0xb10427c8 > > > 06-10 09:16:11.076 7409-7409/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Shared lib > > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > > already loaded in same CL 0xb10427c8 > > > 06-10 09:16:11.076 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ started: > > > > > > testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Trying to load lib > > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > > 0xb10427c8 > > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Shared lib > > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > > already loaded in same CL 0xb10427c8 > > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Trying to load lib > > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > > 0xb10427c8 > > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Shared lib > > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > > already loaded in same CL 0xb10427c8 > > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Trying to load lib > > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > > 0xb10427c8 > > > 06-10 09:16:11.086 7409-7423/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Shared lib > > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > > already loaded in same CL 0xb10427c8 > > > 06-10 09:16:11.096 7409-7423/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Trying to load lib > > > /data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so > > > 0xb10427c8 > > > 06-10 09:16:11.096 7409-7423/org.adblockplus.libadblockplus.tests > > D/dalvikvm﹕ > > > Shared lib > > > '/data/app-lib/org.adblockplus.libadblockplus.tests-2/libadblockplus-jni.so' > > > already loaded in same CL 0xb10427c8 > > > 06-10 09:16:11.176 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ finished: > > > > > > testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:11.176 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ passed: > > > > > > testAndroidTestCaseSetupProperly(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:11.186 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ started: > > > > > > testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ finished: > > > > > > testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ passed: > > > > > > testApplicationUpdateAvailable(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:12.316 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ started: > > > testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ finished: > > > testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ passed: > > > testRequestFailure(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:12.496 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ started: > > > testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ finished: > > > testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ passed: > > > testWrongApplication(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:13.576 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ started: > > > testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ finished: > > > testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ passed: > > > testWrongURL(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:14.686 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ started: > > > testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:15.766 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ finished: > > > testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:15.766 7409-7423/org.adblockplus.libadblockplus.tests > > > I/TestRunner﹕ passed: > > > testWrongVersion(org.adblockplus.libadblockplus.tests.UpdateCheckTest) > > > 06-10 09:16:15.766 7398-7398/? D/AndroidRuntime﹕ Shutting down VM > > > > This solution is fine for me, but the cache logic and OnLoad/OnUnload > functions > > should be moved to another file (e.g. JniLibrary) where it should keep the > > cached classes and all other logic that would be shared across the library > > added patch set #2. Moved global entry points ("JNI_OnLoad" and "JNI_OnUnload") > to new file JniLibrary.cpp and referencing to needed function like "file_OnLoad" > f.e. JniJsValue_OnLoad. All files which are interested in load/unload logics > should provide methods and declare them in header. LGTM
Looks good, I just have some nits and one question. And also a more general question: Shouldn't we precache ALL classes, not just JsValue? If so, it's fine by me if we just do it for JsValue now, but we should then create a followup issue for doing it for the other classes as well. https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... File libadblockplus-android/jni/JniJsValue.cpp (right): https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... libadblockplus-android/jni/JniJsValue.cpp:227: void JniJsValue_OnUnload(JavaVM *vm, JNIEnv *env, void *reserved) Nit: Asterisk should be next to the type according to our coding style (i.e. `JavaVM* vm` etc). https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... File libadblockplus-android/jni/JniJsValue.h (right): https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... libadblockplus-android/jni/JniJsValue.h:34: void JniJsValue_OnUnload(JavaVM *vm, JNIEnv *env, void *reserved); Nit: Maybe it's just me, but I think I'd find it easier if OnUnload comes right after OnLoad, similar to how destructors are typically listed right after constructors. https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... File libadblockplus-android/jni/JniLibrary.cpp (right): https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... libadblockplus-android/jni/JniLibrary.cpp:25: return JNI_ERR; Can we actually return JNI_ERR here? How I interpret the docs (https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/invocation.ht...), we should return JNI_VERSION_1_6 in either case, because JNI_OnLoad is supposed to return the required JNI version. https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... libadblockplus-android/jni/JniLibrary.cpp:33: void JNI_OnUnload(JavaVM *vm, void *reserved) Nit: Same issue with the asterisks again, should be `JavaVM* vm` etc. https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... libadblockplus-android/jni/JniLibrary.cpp:36: if (vm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6) != JNI_OK) Can we use a constant for the required JNI version, rather than duplicating it?
https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... File libadblockplus-android/jni/JniJsValue.cpp (right): https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... libadblockplus-android/jni/JniJsValue.cpp:227: void JniJsValue_OnUnload(JavaVM *vm, JNIEnv *env, void *reserved) On 2016/09/15 13:01:19, Felix Dahlke wrote: > Nit: Asterisk should be next to the type according to our coding style (i.e. > `JavaVM* vm` etc). Acknowledged. https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... File libadblockplus-android/jni/JniLibrary.cpp (right): https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... libadblockplus-android/jni/JniLibrary.cpp:25: return JNI_ERR; On 2016/09/15 13:01:19, Felix Dahlke wrote: > Can we actually return JNI_ERR here? How I interpret the docs > (https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/invocation.ht...), > we should return JNI_VERSION_1_6 in either case, because JNI_OnLoad is supposed > to return the required JNI version. As it's written here (https://developer.android.com/training/articles/perf-jni.html) --- jint JNI_OnLoad(JavaVM* vm, void* reserved) { JNIEnv* env; if (vm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6) != JNI_OK) { return -1; } // Get jclass with env->FindClass. // Register methods with env->RegisterNatives. return JNI_VERSION_1_6; } --- in case of error it returns "-1" which is actually "JNI_ERR": jni.h: #define JNI_ERR (-1) /* generic error */ in case of success it returns required java version https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... libadblockplus-android/jni/JniLibrary.cpp:33: void JNI_OnUnload(JavaVM *vm, void *reserved) On 2016/09/15 13:01:19, Felix Dahlke wrote: > Nit: Same issue with the asterisks again, should be `JavaVM* vm` etc. Acknowledged. https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... libadblockplus-android/jni/JniLibrary.cpp:33: void JNI_OnUnload(JavaVM *vm, void *reserved) On 2016/09/15 13:01:19, Felix Dahlke wrote: > Nit: Same issue with the asterisks again, should be `JavaVM* vm` etc. Acknowledged. https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... libadblockplus-android/jni/JniLibrary.cpp:36: if (vm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6) != JNI_OK) On 2016/09/15 13:01:19, Felix Dahlke wrote: > Can we use a constant for the required JNI version, rather than duplicating it? Acknowledged.
https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... File libadblockplus-android/jni/JniJsValue.h (right): https://codereview.adblockplus.org/29345737/diff/29352965/libadblockplus-andr... libadblockplus-android/jni/JniJsValue.h:34: void JniJsValue_OnUnload(JavaVM *vm, JNIEnv *env, void *reserved); On 2016/09/15 13:01:19, Felix Dahlke wrote: > Nit: Maybe it's just me, but I think I'd find it easier if OnUnload comes right > after OnLoad, similar to how destructors are typically listed right after > constructors. Acknowledged.
Just one thing now. But have you noticed my general question about whether we should do this for all classes and not just JsValue? https://codereview.adblockplus.org/29345737/diff/29353141/libadblockplus-andr... File libadblockplus-android/jni/JniLibrary.cpp (right): https://codereview.adblockplus.org/29345737/diff/29353141/libadblockplus-andr... libadblockplus-android/jni/JniLibrary.cpp:20: #define JNI_REQUIRED_VERSION (JNI_VERSION_1_6) I actually just noticed that there is already a define for this, it's ABP_JNI_VERSION from Utils.h - I guess we should use that one.
On 2016/09/15 13:01:20, Felix Dahlke wrote:
> Looks good, I just have some nits and one question.
>
> And also a more general question: Shouldn't we precache ALL classes, not just
> JsValue? If so, it's fine by me if we just do it for JsValue now, but we
should
> then create a followup issue for doing it for the other classes as well.
>
Rene wrote:
---
Normally I do have the pre-caching integrated in the Ctor of the class that uses
it, this is somehow an exception. So, either remove all the local caches and put
everything into Onload, or remove Onload and put the one into a local cache.
---
I've seen the question and i wrote to Rene to discuss it before i reply to you.
Here is my email with considerations to Rene:
---
Yes, i see it in JniLogSystemCallback f.e.:
JniLogSystemCallback::JniLogSystemCallback(JNIEnv* env, jobject callbackObject)
: JniCallbackBase(env, callbackObject), AdblockPlus::LogSystem(),
logLevelClass(new JniGlobalReference<jclass>(env,
env->FindClass(PKG("LogSystem$LogLevel"))))
{
}
...
private:
const JniGlobalReference<jclass>::Ptr logLevelClass;
};
I like this approach since the reference is hold more close (in object field)
not just in namespace.
The problem is that "FindClass" is working for every object ctor and in case of
JsValue it can be significant overhead.
another benefit is if we use load/unload some classes can skip searching some
classes because they are already found by another class.
F.e. is some class is required for 2 different classes we will have 1 findclass
invocation instead of 2 in 2 object constructors.
So from data-safe point of view i'd prefer your solution but from performance
point of view i'd prefer cache them in JNI_load/Unload.
---
https://codereview.adblockplus.org/29345737/diff/29353141/libadblockplus-andr... File libadblockplus-android/jni/JniLibrary.cpp (right): https://codereview.adblockplus.org/29345737/diff/29353141/libadblockplus-andr... libadblockplus-android/jni/JniLibrary.cpp:20: #define JNI_REQUIRED_VERSION (JNI_VERSION_1_6) On 2016/09/15 15:05:56, Felix Dahlke wrote: > I actually just noticed that there is already a define for this, it's > ABP_JNI_VERSION from Utils.h - I guess we should use that one. Acknowledged.
On 2016/09/16 05:48:35, anton wrote:
> On 2016/09/15 13:01:20, Felix Dahlke wrote:
> > Looks good, I just have some nits and one question.
> >
> > And also a more general question: Shouldn't we precache ALL classes, not
just
> > JsValue? If so, it's fine by me if we just do it for JsValue now, but we
> should
> > then create a followup issue for doing it for the other classes as well.
> >
>
> Rene wrote:
> ---
> Normally I do have the pre-caching integrated in the Ctor of the class that
uses
> it, this is somehow an exception. So, either remove all the local caches and
put
> everything into Onload, or remove Onload and put the one into a local cache.
> ---
>
> I've seen the question and i wrote to Rene to discuss it before i reply to
you.
> Here is my email with considerations to Rene:
> ---
> Yes, i see it in JniLogSystemCallback f.e.:
>
> JniLogSystemCallback::JniLogSystemCallback(JNIEnv* env, jobject
callbackObject)
> : JniCallbackBase(env, callbackObject), AdblockPlus::LogSystem(),
> logLevelClass(new JniGlobalReference<jclass>(env,
> env->FindClass(PKG("LogSystem$LogLevel"))))
> {
> }
>
> ...
>
> private:
> const JniGlobalReference<jclass>::Ptr logLevelClass;
> };
>
> I like this approach since the reference is hold more close (in object field)
> not just in namespace.
> The problem is that "FindClass" is working for every object ctor and in case
of
> JsValue it can be significant overhead.
> another benefit is if we use load/unload some classes can skip searching some
> classes because they are already found by another class.
> F.e. is some class is required for 2 different classes we will have 1
findclass
> invocation instead of 2 in 2 object constructors.
>
> So from data-safe point of view i'd prefer your solution but from performance
> point of view i'd prefer cache them in JNI_load/Unload.
> ---
So I guess that answers my question, René also thinks that we should do this
consistently, i.e. for all classes, not just JsValue: "So, either remove all the
local caches and put everything into Onload [...]".
Would you mind creating a follow-up issue for caching all classes in OnLoad? I
suppose there's no rush to do this, but we should still eventually do it for
performance reasons.
On 2016/09/16 06:12:52, Felix Dahlke wrote:
> On 2016/09/16 05:48:35, anton wrote:
> > On 2016/09/15 13:01:20, Felix Dahlke wrote:
> > > Looks good, I just have some nits and one question.
> > >
> > > And also a more general question: Shouldn't we precache ALL classes, not
> just
> > > JsValue? If so, it's fine by me if we just do it for JsValue now, but we
> > should
> > > then create a followup issue for doing it for the other classes as well.
> > >
> >
> > Rene wrote:
> > ---
> > Normally I do have the pre-caching integrated in the Ctor of the class that
> uses
> > it, this is somehow an exception. So, either remove all the local caches and
> put
> > everything into Onload, or remove Onload and put the one into a local cache.
> > ---
> >
> > I've seen the question and i wrote to Rene to discuss it before i reply to
> you.
> > Here is my email with considerations to Rene:
> > ---
> > Yes, i see it in JniLogSystemCallback f.e.:
> >
> > JniLogSystemCallback::JniLogSystemCallback(JNIEnv* env, jobject
> callbackObject)
> > : JniCallbackBase(env, callbackObject), AdblockPlus::LogSystem(),
> > logLevelClass(new JniGlobalReference<jclass>(env,
> > env->FindClass(PKG("LogSystem$LogLevel"))))
> > {
> > }
> >
> > ...
> >
> > private:
> > const JniGlobalReference<jclass>::Ptr logLevelClass;
> > };
> >
> > I like this approach since the reference is hold more close (in object
field)
> > not just in namespace.
> > The problem is that "FindClass" is working for every object ctor and in case
> of
> > JsValue it can be significant overhead.
> > another benefit is if we use load/unload some classes can skip searching
some
> > classes because they are already found by another class.
> > F.e. is some class is required for 2 different classes we will have 1
> findclass
> > invocation instead of 2 in 2 object constructors.
> >
> > So from data-safe point of view i'd prefer your solution but from
performance
> > point of view i'd prefer cache them in JNI_load/Unload.
> > ---
>
> So I guess that answers my question, René also thinks that we should do this
> consistently, i.e. for all classes, not just JsValue: "So, either remove all
the
> local caches and put everything into Onload [...]".
>
> Would you mind creating a follow-up issue for caching all classes in OnLoad? I
> suppose there's no rush to do this, but we should still eventually do it for
> performance reasons.
Created ticket https://issues.adblockplus.org/ticket/4442
Great, LGTM!
https://codereview.adblockplus.org/29345737/diff/29353257/libadblockplus-andr... File libadblockplus-android/jni/JniLibrary.cpp (right): https://codereview.adblockplus.org/29345737/diff/29353257/libadblockplus-andr... libadblockplus-android/jni/JniLibrary.cpp:19: added #include "Utils.h" for ABP_JNI_VERSION to be available in final commit 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
