New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NewThreadWorker.tryEnableCancelPolicy doing costly reflection on Android #3119

Closed
digitalbuddha opened this Issue Jul 29, 2015 · 13 comments

Comments

Projects
None yet
4 participants
@digitalbuddha

digitalbuddha commented Jul 29, 2015

I was analyzing startup time in the NY Times Android app and started method profiling on startup using Android Device Monitor. Total time from the beginning of the application class to end of onCreate for first activity is roughly 2.2seconds. Diving deeper I was able to observe that NewThreadWorker.tryEnableCancelPolicy
was taking 1200ms to execute with the offending line being
for (Method m : exec.getClass().getMethods()) (1017ms)
Diving deeper shows a call to
CollectionUtils.removeDuplicates (992 ms)
which will call
collection.sort (719ms)
&
reflect.compare(259ms).

tryEnableCancelPolicy has the following comment:

     * Tries to enable the Java 7+ setRemoveOnCancelPolicy.
     * <p>{@code public} visibility reason: called from other package(s) within RxJava.
     * If the method returns false, the {@link #registerExecutor(ScheduledThreadPoolExecutor)} may
     * be called to enable the backup option of purging the executors.
     * @param exec the executor to call setRemoveOnCaneclPolicy if available.
     * @return true if the policy was successfully enabled 
     */ 

I tried creating a Scheduler from an Executor but still hit the offending code. Is there a way to avoid this code or fix the large performance hit that it is causing?

@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Jul 29, 2015

Member

Woah. If the signature of the to-be-reflected method is known why is a direct lookup not being done?

Member

JakeWharton commented Jul 29, 2015

Woah. If the signature of the to-be-reflected method is known why is a direct lookup not being done?

@akarnokd

This comment has been minimized.

Show comment
Hide comment
@akarnokd

akarnokd Jul 29, 2015

Member

@digitalbuddha You can use the system property "rx.scheduler.jdk6.purge-force" set to "true" to avoid the loop. The reason for the loop is to avoid NoSuchMethodException being thrown on JDK 6 which is more costly than looping through ~70 methods. Although I admit evaluating that all the time is unnecessary as Executors.newScheduledExecutor() won't change is ability during runtime.

Member

akarnokd commented Jul 29, 2015

@digitalbuddha You can use the system property "rx.scheduler.jdk6.purge-force" set to "true" to avoid the loop. The reason for the loop is to avoid NoSuchMethodException being thrown on JDK 6 which is more costly than looping through ~70 methods. Although I admit evaluating that all the time is unnecessary as Executors.newScheduledExecutor() won't change is ability during runtime.

@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Jul 29, 2015

Member

Thankfully that property just squeezes under Android's 31 character max at 29 chars!

Member

JakeWharton commented Jul 29, 2015

Thankfully that property just squeezes under Android's 31 character max at 29 chars!

@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Jul 29, 2015

Member

Can we use PlatformDependent.isAndroid() to default this to true?

Member

JakeWharton commented Jul 29, 2015

Can we use PlatformDependent.isAndroid() to default this to true?

@akarnokd

This comment has been minimized.

Show comment
Hide comment
@akarnokd

akarnokd Jul 29, 2015

Member

My google search indicates the method setRemoveOnCancelPolicy is supported from API level 22. If there could be a way to discover the API level programmatically and cross-platform safe then sure.

Member

akarnokd commented Jul 29, 2015

My google search indicates the method setRemoveOnCancelPolicy is supported from API level 22. If there could be a way to discover the API level programmatically and cross-platform safe then sure.

@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Jul 29, 2015

Member

The android.os.Build.VERSION class has an SDK_INT int constant which can be read. The presence of the class could replace the check for android.app.Application.

Member

JakeWharton commented Jul 29, 2015

The android.os.Build.VERSION class has an SDK_INT int constant which can be read. The presence of the class could replace the check for android.app.Application.

@artem-zinnatullin

This comment has been minimized.

Show comment
Hide comment
@artem-zinnatullin

artem-zinnatullin Jul 29, 2015

Contributor

@akarnokd what do you mean by

cross-platform safe

? android.os.Build.VERSION is safe to call for all Android versions.

Documentation: http://developer.android.com/reference/android/os/Build.VERSION.html#SDK_INT

Contributor

artem-zinnatullin commented Jul 29, 2015

@akarnokd what do you mean by

cross-platform safe

? android.os.Build.VERSION is safe to call for all Android versions.

Documentation: http://developer.android.com/reference/android/os/Build.VERSION.html#SDK_INT

@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Jul 29, 2015

Member

Android is one platform

Member

JakeWharton commented Jul 29, 2015

Android is one platform

@akarnokd

This comment has been minimized.

Show comment
Hide comment
@akarnokd

akarnokd Jul 29, 2015

Member

Great. Would you like to submit a PR?

Member

akarnokd commented Jul 29, 2015

Great. Would you like to submit a PR?

@artem-zinnatullin

This comment has been minimized.

Show comment
Hide comment
Contributor

artem-zinnatullin commented Jul 29, 2015

@artem-zinnatullin

This comment has been minimized.

Show comment
Hide comment
@artem-zinnatullin

artem-zinnatullin Jul 30, 2015

Contributor

Little Gist for those who want to fix this in Android app with RxJava: https://gist.github.com/artem-zinnatullin/51b6c6720ecb8a2a71eb

Contributor

artem-zinnatullin commented Jul 30, 2015

Little Gist for those who want to fix this in Android app with RxJava: https://gist.github.com/artem-zinnatullin/51b6c6720ecb8a2a71eb

@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Jul 30, 2015

Member

Your gist says "static initializer block" and then proceeds to use an instance initializer block.

Member

JakeWharton commented Jul 30, 2015

Your gist says "static initializer block" and then proceeds to use an instance initializer block.

@artem-zinnatullin

This comment has been minimized.

Show comment
Hide comment
@artem-zinnatullin

artem-zinnatullin Jul 30, 2015

Contributor

Such a stupid mistake…Uh. Thanks.

Contributor

artem-zinnatullin commented Jul 30, 2015

Such a stupid mistake…Uh. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment