Improve performance of NewThreadWorker, disable search for setRemoveOnCancelPolicy() on Android API < 21 #3121
Conversation
| @@ -50,6 +53,16 @@ | |||
| PURGE = new AtomicReference<ScheduledExecutorService>(); | |||
| PURGE_FORCE = Boolean.getBoolean(PURGE_FORCE_KEY); | |||
| PURGE_FREQUENCY = Integer.getInteger(FREQUENCY_KEY, 1000); | |||
|
|
|||
| // Cached to prevent method call | |||
| IS_ANDROID = PlatformDependent.isAndroid(); | |||
There was a problem hiding this comment.
This is redundant with the other checks
There was a problem hiding this comment.
I don't think so, we're trying to enable cancel policy if current platform is not Android or if it's Android and API >= 21, that's why I have 2 separate booleans.
Or maybe I just don't see easier way.
There was a problem hiding this comment.
You are checking if the API is null which is equivalent (and thus redundant) with this boolean.
There was a problem hiding this comment.
Yep, but then I use IS_ANDROID on line 127. I've improved IS_ANDROID initialization.
|
Checked on JVM (Java 8), Android 4.1.2 (API 16), Android 5.1.1 (API 22). |
artem-zinnatullin
force-pushed
the
android-setRemoveOnCancelPolicy
branch
2 times, most recently
from
85d2e83 to
1cd7141
Compare
| private static final boolean IS_ANDROID = isAndroid0(); | ||
| // Will be null if current platform is not Android | ||
| // Or if Android API Version can not be resolved. | ||
| private static final Integer ANDROID_API_VERSION = resolveAndroidApiVersion(); |
There was a problem hiding this comment.
Is there an API version 0? If not, we could use int here so there is no need for unboxing and null checking elsewhere.
There was a problem hiding this comment.
Nope, there is no API version 0 as far as I know, we can also use -1 as Not-Android-Platform value and even create a constant.
There was a problem hiding this comment.
Nope. Great idea! RxJava doesn't support anything below API 9 anyway.
artem-zinnatullin
force-pushed
the
android-setRemoveOnCancelPolicy
branch
from
1cd7141 to
5c914e4
Compare
| final Integer androidApiVersion = PlatformDependent.getAndroidApiVersion(); | ||
|
|
||
| // Cached to prevent method call | ||
| IS_ANDROID = androidApiVersion != null; |
There was a problem hiding this comment.
You can still eliminate this variable altogether. The same check is included in the other.
artem-zinnatullin
force-pushed
the
android-setRemoveOnCancelPolicy
branch
from
5c914e4 to
47c063d
Compare
artem-zinnatullin
changed the title
| private static final boolean IS_ANDROID = isAndroid0(); | ||
| /** | ||
| * Constant value of Android API Version which means that current platform is not Android. | ||
| * We could use {@link Integer} and {@code null}, but we want to avoid boxing/unboxing. |
There was a problem hiding this comment.
Second sentence is not useful to retain.
| } | ||
|
|
||
| @Test | ||
| public void tryEnableCancelPolicyShouldBeSkipped() throws NoSuchFieldException, IllegalAccessException { |
There was a problem hiding this comment.
This looks rather dangerous. What I'd do is to have a package-private method tryEnableCancelPolicy(ScheduledExecutor executor, boolean tryEnable) so the test can inject the necessary value.
There was a problem hiding this comment.
Hm, a separate method just for injecting value which can completely disable it… Not sure I am following this (on Android we have some limits on the quantity of methods in the app, this will increment it just for test purposes), let's just remove this test for now?
Or I can remove private modifier from SHOULD_TRY_ENABLE_CANCEL_POLICY and make this test more "static", reflection will only change the value of this field.
There was a problem hiding this comment.
I'd rather not have this test for now. All other changes appear to be okay. If you update the PR, I'll merge it.
artem-zinnatullin
force-pushed
the
android-setRemoveOnCancelPolicy
branch
from
47c063d to
81b1951
Compare
artem-zinnatullin
changed the title
artem-zinnatullin
force-pushed
the
android-setRemoveOnCancelPolicy
branch
from
81b1951 to
750d5ef
Compare
|
Fixed all comments, removed test with reflection, added more javadoc and comments, tests are now compilable on JDK 6 (didn't check but they should work fine). @akarnokd, @JakeWharton PTAL |
| /** | ||
| * Resolves version of Android API. | ||
| * | ||
| * @return version of Android API or {@code null} if version can not be resolved. |
There was a problem hiding this comment.
This no longer returns null but ANDROID_API_VERSION_IS_NOT_ANDROID.
Disable search for ScheduledExecutorService.setRemoveOnCancelPolicy() on Android API < 21
artem-zinnatullin
force-pushed
the
android-setRemoveOnCancelPolicy
branch
from
750d5ef to
dd5c646
Compare
|
Great, thanks! |
…ncelPolicy Improve performance of NewThreadWorker, disable search for setRemoveOnCancelPolicy() on Android API < 21
Resolves #3119.
This PR adds methods for detecting Android API Version and disables
NewThreadWorker.tryEnableCancelPolicy()on Android API < 21 which has significant performance cost on Android.PR also improves performance of
NewThreadWorker.tryEnableCancelPolicy()via caching Reflection.