Issue 29556626: Issue 5790 - Get ready for integration into Chromium (Closed)
|
Created:
Sept. 26, 2017, 11:29 a.m. by anton Modified:
Oct. 3, 2017, 6:48 a.m. CC:
René Jeschke, Felix Dahlke Visibility:
Public. |
DescriptionIssue 5790 - Get ready for integration into Chromium
Patch Set 1 #
Total comments: 6
Patch Set 2 : Renamed to getNativePtr() #
Total comments: 1
MessagesTotal messages: 11
It's not clear why this change is useful but in general good for me. https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java (right): https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:203: public long getFilterEnginePtr() { Maybe call it `getNativePtr`?
https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java (right): https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:203: public long getFilterEnginePtr() { On 2017/09/26 12:04:29, sergei wrote: > Maybe call it `getNativePtr`? Does not make sense for me, we can call it `getNativePtr `
https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:244: public synchronized boolean retain(boolean asynchronous) Could you please point to where and how it's actually used. I would like to check that the approach is not racy. https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java (right): https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:203: public long getFilterEnginePtr() { On 2017/09/26 12:18:40, anton wrote: > On 2017/09/26 12:04:29, sergei wrote: > > Maybe call it `getNativePtr`? > > Does not make sense for me, we can call it `getNativePtr` I don't insist, it's just my expectations of an API. This method is rather for some internal and very advanced usages and it's a common practice to have a convention that such methods like `getNative`, `getHandle`, etc, providing the access to an internal structure and they should not duplicate the name of the class. So if there is a need to expose the underlying handle from JsValue, Platform or some another class wrapping a native resource one can always get it through such method. http://en.cppreference.com/w/cpp/thread/thread/native_handle http://www.boost.org/doc/libs/1_64_0/doc/html/thread/thread_management.html#t...
Uploaded new patch set with renaming https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:244: public synchronized boolean retain(boolean asynchronous) On 2017/09/28 08:29:03, sergei wrote: > Could you please point to where and how it's actually used. I would like to > check that the approach is not racy. Java's `synchronized` token excludes mutiple thread to access the block concurrently. The second thread will be just blocked. Also `referenceCounter` is `Atomic` and it excludes race here too. https://bitbucket.org/4ntoine/chrome4sdp/commits/3bcbeadc50affdf2592d35fdb93c... ``` // synchronously (blocks the UI (providing bad UX) but allows to get pointer) // improvement: do it async and wait for engine to be created and passed when URL is about to load if (AdblockHelper.get().retain(false)) { // pass FilterEngine instance pointer to C++ side long ptr = AdblockHelper.get().getEngine().getFilterEngine().getFilterEnginePtr(); // not just getPointer(); ! Log.w(TAG, "Adblock: Notify C++ FilterEngine is created (passing pointer) " + ptr + " in thread " + Thread.currentThread()); sendFilterEnginePointer(ptr); } ```
https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29556626/diff/29556627/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:244: public synchronized boolean retain(boolean asynchronous) On 2017/09/28 09:08:39, anton wrote: > On 2017/09/28 08:29:03, sergei wrote: > > Could you please point to where and how it's actually used. I would like to > > check that the approach is not racy. > > Java's `synchronized` token excludes mutiple thread to access the block > concurrently. The second thread will be just blocked. > Also `referenceCounter` is `Atomic` and it excludes race here too. > > https://bitbucket.org/4ntoine/chrome4sdp/commits/3bcbeadc50affdf2592d35fdb93c... > > ``` > // synchronously (blocks the UI (providing bad UX) but allows to get > pointer) > // improvement: do it async and wait for engine to be created and passed > when URL is about to load > if (AdblockHelper.get().retain(false)) { > // pass FilterEngine instance pointer to C++ side > long ptr = > AdblockHelper.get().getEngine().getFilterEngine().getFilterEnginePtr(); // not > just getPointer(); ! > Log.w(TAG, "Adblock: Notify C++ FilterEngine is created (passing > pointer) " + ptr + " in thread " + Thread.currentThread()); > sendFilterEnginePointer(ptr); BTW, I plan to add an asynchronous version of creation of FilterEngine in Java because it's what should actually be used and it's already available in C++, will it be more convenient here? > } > ``` I cannot evaluate it now because it's not clear how the initAdblock and deinitAdblock are called. JIC, please take into account that if they are called from different threads then it's possible that between "Log.w(TAG, "Adblock: Notify C++ FilterEngine is created (passing pointer) " + ptr + " in thread " + Thread.currentThread());" and "sendFilterEnginePointer(ptr);" we execute `deinitAdblock` and afterwards continue with "sendFilterEnginePointer(ptr);". I just hope that initAdblock and deinitAdblock are called only from the same thread and, although it's not important maybe merely once at the start of the app and at the finishing of the app. I would like to come back to the topic later.
LGTM
https://codereview.adblockplus.org/29556626/diff/29558584/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java (right): https://codereview.adblockplus.org/29556626/diff/29558584/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:205: } Well, this ain't a good practice to expose/share native pointers through our Java API. But if it's currently the only way to integrate our lib into Chromium we can leave it and should revisit this in the future to see if something has changed, allowing us to integrate without exposing native pointers
On 2017/09/28 11:55:30, diegocarloslima wrote: > https://codereview.adblockplus.org/29556626/diff/29558584/libadblockplus-andr... > File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java > (right): > > https://codereview.adblockplus.org/29556626/diff/29558584/libadblockplus-andr... > libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:205: > } > Well, this ain't a good practice to expose/share native pointers through our > Java API. But if it's currently the only way to integrate our lib into Chromium > we can leave it and should revisit this in the future to see if something has > changed, allowing us to integrate without exposing native pointers Well our java classes are mostly wrappers to C++ code so we already working with native pointers (see `long ptr` members) in unsafe way. it's a long explanation required but i will try to keep it brief and clear. Since Chromium has huge c++ base and Java(Android) code we could integrate c++ but we need UI and it's in Android code only. We have to use Java's filter engine anyway for UI. So we have to integrate libadblockplus-android, not just libadblockplus. But filtering is done in C++ code (chrome_network_delegate.cc) we have to have C++ filter engine OR create JNI bridge to java and work with Java's filter engine instance. So we have to choose between less safety (working with raw C++ pointer retrieved from Java's FilterEngine with 'getNativePtr()') + better performance and more safety + much less performance. Imagine every filter engine call should be wrapped into Java call and arguments converting. So it's working in c++ in chromium and in c++ in libadblockplus. I don't think we should forward it to Java (create new JNI bridge) and then use libadblockplus-android (which is actually JNI bridge too) because of overhead but for better safety.
On 2017/09/28 12:37:33, anton wrote: .... > Since Chromium has huge c++ base and Java(Android) code we could integrate c++ > but we need UI and it's in Android code only. > We have to use Java's filter engine anyway for UI. So we have to integrate > libadblockplus-android, > not just libadblockplus. But filtering is done in C++ code > (chrome_network_delegate.cc) we have to have C++ filter engine OR create JNI > bridge to java and work with > Java's filter engine instance. > > So we have to choose between less safety (working with raw C++ pointer retrieved > from Java's FilterEngine with 'getNativePtr()') + better performance and more > safety + much less performance. > Imagine every filter engine call should be wrapped into Java call and arguments > converting. > > So it's working in c++ in chromium and in c++ in libadblockplus. > I don't think we should forward it to Java (create new JNI bridge) and then use > libadblockplus-android (which is actually JNI bridge too) because of overhead > but for better safety. Thanks for the explanation, I had similar questions too, now it's clearer and for me it's fine. It's actually a good point what should be the balance and how to organize these things, if you don't mind I would like to have it in some issue, so we can revisit it later and think about/discuss/work on it separately from this review.
On 2017/09/28 14:36:52, sergei wrote: > On 2017/09/28 12:37:33, anton wrote: > .... > > Since Chromium has huge c++ base and Java(Android) code we could integrate c++ > > but we need UI and it's in Android code only. > > We have to use Java's filter engine anyway for UI. So we have to integrate > > libadblockplus-android, > > not just libadblockplus. But filtering is done in C++ code > > (chrome_network_delegate.cc) we have to have C++ filter engine OR create JNI > > bridge to java and work with > > Java's filter engine instance. > > > > So we have to choose between less safety (working with raw C++ pointer > retrieved > > from Java's FilterEngine with 'getNativePtr()') + better performance and more > > safety + much less performance. > > Imagine every filter engine call should be wrapped into Java call and > arguments > > converting. > > > > So it's working in c++ in chromium and in c++ in libadblockplus. > > I don't think we should forward it to Java (create new JNI bridge) and then > use > > libadblockplus-android (which is actually JNI bridge too) because of overhead > > but for better safety. > > Thanks for the explanation, I had similar questions too, now it's clearer and > for me it's fine. It's actually a good point what should be the balance and how > to organize these things, if you don't mind I would like to have it in some > issue, so we can revisit it later and think about/discuss/work on it separately > from this review. LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
