Skip to content
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

Use bounded ThreadPoolExecutor for async notify calls #145

Merged
merged 3 commits into from
May 2, 2017

Conversation

felipecsl
Copy link
Contributor

We noticed performance issues when making several calls to Bugsnag#notify in a short period of time. It turns out that it's because it is using Executors#newCachedThreadPool which creates as many new threads as needed. This can quickly overload the system, cause lots of GC overhead and even ANRs.
Just used the AsyncTask#THREAD_POOL_EXECUTOR settings here and made it a tiny bit more conservative.
Also updated the dependencies while I was at it for convenience, but if you don't want that in this PR, I'd be happy to revert them.

@kattrali
Copy link
Contributor

kattrali commented Apr 25, 2017

Thank you for the contribution, @felipecsl. I'm investigating what's happening with the tests now, as the tasks which start runnables now encounter a VerifyError during initialization.

@kattrali
Copy link
Contributor

This seems to be an issue with calling allowCoreThreadTimeOut(boolean), despite the code being gated by SDK version. Without it, the tests then pass as expected on API 4 and newer.

At runtime, the verifier rejects the Async class because the method cannot be found:

E/dalvikvm( 1091): Could not find method java.util.concurrent.ThreadPoolExecutor.allowCoreThreadTimeOut, referenced from method com.bugsnag.android.Async.<clinit>
W/dalvikvm( 1091): VFY: unable to resolve virtual method 717: Ljava/util/concurrent/ThreadPoolExecutor;.allowCoreThreadTimeOut (Z)V
W/dalvikvm( 1091): VFY:  rejecting opcode 0x6e at 0x0049
W/dalvikvm( 1091): VFY:  rejected Lcom/bugsnag/android/Async;.<clinit> ()V
W/dalvikvm( 1091): Verifier rejected class Lcom/bugsnag/android/Async;

Did you see similar improvements to performance even without timing out the core threads? There could potentially be less core threads while keeping the maximum as well, though that would be more aggressive.

@felipecsl
Copy link
Contributor Author

felipecsl commented Apr 27, 2017

Yeah it should be fine to remove that since it's a minor optimization.
I'll update the PR

@felipecsl
Copy link
Contributor Author

PR updated, @kattrali PTAL

@kattrali kattrali merged commit dca8cc7 into bugsnag:master May 2, 2017
@felipecsl felipecsl deleted the felipe/async-executor branch May 2, 2017 21:18
rich-bugsnag pushed a commit that referenced this pull request Sep 3, 2021
Update Android example code to use AAR rather than plain Java file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants