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

Changed library to use annotations #130

Merged
merged 3 commits into from Feb 4, 2016

Conversation

Projects
None yet
6 participants
@rjbrock
Contributor

rjbrock commented Nov 15, 2014

The library now requires the use of annotations in order for a method to be subscribed to an event.

@Subscribe
public void mySubscribingMethod(MyEvent event)

rjbrock added some commits Nov 14, 2014

Changed method subscription to use Annotations
 - Removed use of onEvent, onEventThreadName
 - Added @subscribe annotation with an optional threadMode arg
@danielbenzvi

This comment has been minimized.

Show comment
Hide comment
@danielbenzvi

danielbenzvi commented Nov 15, 2014

Why?

@rjbrock

This comment has been minimized.

Show comment
Hide comment
@rjbrock

rjbrock Nov 15, 2014

Contributor

@danielbenzvi Because annotations have become the standard way to use reflection in android. This is also a cleaner solution which allows you to name your methods in a way that makes more sense to your app. In addition, we can allow more customization via annotation arguments. We had a conversation in an issue (#126)

Contributor

rjbrock commented Nov 15, 2014

@danielbenzvi Because annotations have become the standard way to use reflection in android. This is also a cleaner solution which allows you to name your methods in a way that makes more sense to your app. In addition, we can allow more customization via annotation arguments. We had a conversation in an issue (#126)

@dcow

This comment has been minimized.

Show comment
Hide comment
@dcow

dcow commented Nov 17, 2014

+1

@rjbrock

This comment has been minimized.

Show comment
Hide comment
@rjbrock

rjbrock Nov 17, 2014

Contributor

@greenrobot CI failed due to a timeout in downloading gradle artifacts. I can't retrigger the build, I dont think that I have the proper permissions

Contributor

rjbrock commented Nov 17, 2014

@greenrobot CI failed due to a timeout in downloading gradle artifacts. I can't retrigger the build, I dont think that I have the proper permissions

@greenrobot

This comment has been minimized.

Show comment
Hide comment
@greenrobot

greenrobot Nov 17, 2014

Owner

Done. I'd be curious about registration performance. Could you compare results with the performance app? Especially 1st time subscribers test. Should not make a big difference...

Owner

greenrobot commented Nov 17, 2014

Done. I'd be curious about registration performance. Could you compare results with the performance app? Especially 1st time subscribers test. Should not make a big difference...

@rjbrock

This comment has been minimized.

Show comment
Hide comment
@rjbrock

rjbrock Nov 17, 2014

Contributor

Right now, it looks like the first time subscriber takes ~1760 microseconds on my nexus 5. Which gives roughly 450/s.

A subsequent run gave 720 microseconds and 1400/second

Contributor

rjbrock commented Nov 17, 2014

Right now, it looks like the first time subscriber takes ~1760 microseconds on my nexus 5. Which gives roughly 450/s.

A subsequent run gave 720 microseconds and 1400/second

@greenrobot

This comment has been minimized.

Show comment
Hide comment
@greenrobot

greenrobot Nov 17, 2014

Owner

Which Android version? What's the value in EventBus 2.4.0 without annotations? It might be a good idea to increase the subscriber amount to get more stable results.
PS.: Anyone else wants to join for giving that branch a try for performance testing?

Owner

greenrobot commented Nov 17, 2014

Which Android version? What's the value in EventBus 2.4.0 without annotations? It might be a good idea to increase the subscriber amount to get more stable results.
PS.: Anyone else wants to join for giving that branch a try for performance testing?

@rjbrock

This comment has been minimized.

Show comment
Hide comment
@rjbrock

rjbrock Nov 17, 2014

Contributor

That was in android 4.4.4

Contributor

rjbrock commented Nov 17, 2014

That was in android 4.4.4

Added logging and sped up method finding
 - Moved check for bridge and synthetic methods up to
   rule them out asap
 - Reorganized method finding to be more clear
 - Added comments
 - Added logging to method finding if the user enables it
@rjbrock

This comment has been minimized.

Show comment
Hide comment
@rjbrock

rjbrock Nov 17, 2014

Contributor

I modified the SubscriberMethodFinder and I am now getting 3700/s roughly in android 4.4.4

Contributor

rjbrock commented Nov 17, 2014

I modified the SubscriberMethodFinder and I am now getting 3700/s roughly in android 4.4.4

@greenrobot

This comment has been minimized.

Show comment
Hide comment
@greenrobot

greenrobot Nov 18, 2014

Owner

On my Nexus 5 with Android 5.0, I get >26,000/s on EventBus 2.4 with 100,000 subscriber count.

Owner

greenrobot commented Nov 18, 2014

On my Nexus 5 with Android 5.0, I get >26,000/s on EventBus 2.4 with 100,000 subscriber count.

@rjbrock

This comment has been minimized.

Show comment
Hide comment
@rjbrock

rjbrock Nov 18, 2014

Contributor

@greenrobot ah ok. I was doing 1st subscribe with 100 set

Contributor

rjbrock commented Nov 18, 2014

@greenrobot ah ok. I was doing 1st subscribe with 100 set

@rjbrock

This comment has been minimized.

Show comment
Hide comment
@rjbrock

rjbrock Nov 21, 2014

Contributor

@greenrobot I was wondering if you have had any time to check out these changes. We would like to start testing internally with a jar that I can build, but I want to make sure the syntax would remain pretty much how it is in this pull request.

Contributor

rjbrock commented Nov 21, 2014

@greenrobot I was wondering if you have had any time to check out these changes. We would like to start testing internally with a jar that I can build, but I want to make sure the syntax would remain pretty much how it is in this pull request.

@MariusVolkhart MariusVolkhart referenced this pull request Dec 29, 2014

Closed

Annotation Support #126

@rjbrock

This comment has been minimized.

Show comment
Hide comment
@rjbrock

rjbrock Jan 5, 2015

Contributor

@greenrobot Any updates?

Contributor

rjbrock commented Jan 5, 2015

@greenrobot Any updates?

@greenrobot

This comment has been minimized.

Show comment
Hide comment
@greenrobot

greenrobot Jan 7, 2015

Owner

Did not have the chance have a closer look yet.

Owner

greenrobot commented Jan 7, 2015

Did not have the chance have a closer look yet.

@rjbrock

This comment has been minimized.

Show comment
Hide comment
@rjbrock

rjbrock Jan 8, 2015

Contributor

@greenrobot do you know when you will have a chance to take a deeper look? I am hoping to use this library in our application, but I am holding out until annotations are a part

Contributor

rjbrock commented Jan 8, 2015

@greenrobot do you know when you will have a chance to take a deeper look? I am hoping to use this library in our application, but I am holding out until annotations are a part

@dodgex dodgex referenced this pull request Feb 3, 2015

Closed

Integrate EventBus to AA? #1237

@jromero

This comment has been minimized.

Show comment
Hide comment
@jromero

jromero Feb 23, 2015

@greenrobot We are also waiting for this feature...

jromero commented Feb 23, 2015

@greenrobot We are also waiting for this feature...

@andreas-

This comment has been minimized.

Show comment
Hide comment
@andreas-

andreas- commented Jan 26, 2016

+1

@greenrobot greenrobot merged commit df91aeb into greenrobot:master Feb 4, 2016

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@rjbrock

This comment has been minimized.

Show comment
Hide comment
@rjbrock

rjbrock Feb 4, 2016

Contributor

Thanks!

Contributor

rjbrock commented Feb 4, 2016

Thanks!

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