Issue 4442 - Cache class and ctors references in JNI_Load/UnLoad

Issue 4442 - Cache class and ctors references in JNI_Load/UnLoad

[anton, 2016-09-23 13:05:26]

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
File libadblockplus-android/jni/JniCallbacks.h (left):

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
libadblockplus-android/jni/JniCallbacks.h:107: jobject NewTuple(JNIEnv* env,
const std::string& a, const std::string& b) const;
"NewTuple" is misleading in WebRequest context, "NewHeaderEntry" is more clear
IMHO

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
File libadblockplus-android/jni/JniWebRequest.cpp (left):

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
libadblockplus-android/jni/JniWebRequest.cpp:65: JniAddObjectToList(*env,
*arrayList, *tuple);
here "add" methods is found headers size times which is extremely bad for
performance!

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
File libadblockplus-android/jni/JniWebRequest.cpp (right):

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
libadblockplus-android/jni/JniWebRequest.cpp:82: JniLocalReference<jobject>
arrayList(*env, NewJniArrayList(*env));
Consider using LinkedList instead of ArrayList - for adding one-by-one use case
it's more efficient. Also since we know what list class we create, we can find
the method once during searching list class and skip using
"JniGetAddToListMethod" at all

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
libadblockplus-android/jni/JniWebRequest.cpp:83: jmethodID addMethod =
JniGetAddToListMethod(*env, *arrayList);
Now searching for the method once

[anton, 2016-09-23 13:07:09]

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
File libadblockplus-android/jni/JniFilter.h (left):

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
libadblockplus-android/jni/JniFilter.h:18: package
org.adblockplus.libadblockplus;
rietveld bug - the file did not exist before

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
File libadblockplus-android/jni/JniLogSystem.h (left):

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
libadblockplus-android/jni/JniLogSystem.h:20: public class NoOpUpdaterCallback
extends UpdateCheckDoneCallback
rietveld bug - the file did not exist before

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
File libadblockplus-android/jni/JniNotification.h (left):

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
libadblockplus-android/jni/JniNotification.h:22: public class LazyWebRequest
extends WebRequest
rietveld bug - the file did not exist before

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
File libadblockplus-android/jni/JniWebRequest.h (left):

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
libadblockplus-android/jni/JniWebRequest.h:20: public class NoOpUpdaterCallback
extends UpdateCheckDoneCallback
rietveld bug - the file did not exist before

[diegocarloslima, 2016-11-04 11:34:19]

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
File libadblockplus-android/jni/JniWebRequest.cpp (right):

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
libadblockplus-android/jni/JniWebRequest.cpp:82: JniLocalReference<jobject>
arrayList(*env, NewJniArrayList(*env));
On 2016/09/23 13:05:25, anton wrote:
> Consider using LinkedList instead of ArrayList - for adding one-by-one use
case
> it's more efficient. Also since we know what list class we create, we can find
> the method once during searching list class and skip using
> "JniGetAddToListMethod" at all 

Yeah, LinkedList are better for insertion while ArraysList are better for
retrieving elements. If we do more insertions that retrieval operations for this
list, I do agree that it makes more sense, but the performance gain is not
really relevant if the list don't get really big.

[diegocarloslima, 2016-11-04 11:35:00]

On 2016/11/04 11:34:19, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
> File libadblockplus-android/jni/JniWebRequest.cpp (right):
> 
>
https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
> libadblockplus-android/jni/JniWebRequest.cpp:82: JniLocalReference<jobject>
> arrayList(*env, NewJniArrayList(*env));
> On 2016/09/23 13:05:25, anton wrote:
> > Consider using LinkedList instead of ArrayList - for adding one-by-one use
> case
> > it's more efficient. Also since we know what list class we create, we can
find
> > the method once during searching list class and skip using
> > "JniGetAddToListMethod" at all 
> 
> Yeah, LinkedList are better for insertion while ArraysList are better for
> retrieving elements. If we do more insertions that retrieval operations for
this
> list, I do agree that it makes more sense, but the performance gain is not
> really relevant if the list don't get really big.

LGTM

[Felix Dahlke, 2016-11-18 10:42:23]

Looks pretty good to me, just one comment.

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
File libadblockplus-android/jni/JniCallbacks.h (left):

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
libadblockplus-android/jni/JniCallbacks.h:107: jobject NewTuple(JNIEnv* env,
const std::string& a, const std::string& b) const;
On 2016/09/23 13:05:25, anton wrote:
> "NewTuple" is misleading in WebRequest context, "NewHeaderEntry" is more clear
> IMHO

Pretty unrelated change though, hm? Given how many files are being changed here 
I think we should stick to what the issue says.

[anton, 2016-11-21 08:26:38]

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
File libadblockplus-android/jni/JniCallbacks.h (left):

https://codereview.adblockplus.org/29354787/diff/29354788/libadblockplus-andr...
libadblockplus-android/jni/JniCallbacks.h:107: jobject NewTuple(JNIEnv* env,
const std::string& a, const std::string& b) const;
On 2016/11/18 10:42:23, Felix Dahlke wrote:
> On 2016/09/23 13:05:25, anton wrote:
> > "NewTuple" is misleading in WebRequest context, "NewHeaderEntry" is more
clear
> > IMHO
> 
> Pretty unrelated change though, hm? Given how many files are being changed
here 
> I think we should stick to what the issue says.

Acknowledged. Fixed in patch set #2

[Felix Dahlke, 2016-11-21 08:50:04]

LGTM!