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

Changed library to use annotations #130

Merged
merged 3 commits into from Feb 4, 2016

Conversation

rjbrock
Copy link
Contributor

@rjbrock 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)

Rick Brock added 2 commits November 14, 2014 15:40
 - Removed use of onEvent, onEventThreadName
 - Added @subscribe annotation with an optional threadMode arg
@danielbenzvi
Copy link

Why?

@rjbrock
Copy link
Contributor Author

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
Copy link

dcow commented Nov 17, 2014

+1

@rjbrock
Copy link
Contributor Author

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
Copy link
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...

@rjbrock
Copy link
Contributor Author

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
Copy link
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?

@rjbrock
Copy link
Contributor Author

rjbrock commented Nov 17, 2014

That was in android 4.4.4

 - 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
Copy link
Contributor Author

rjbrock commented Nov 17, 2014

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

@greenrobot
Copy link
Owner

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

@rjbrock
Copy link
Contributor Author

rjbrock commented Nov 18, 2014

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

@rjbrock
Copy link
Contributor Author

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.

@rjbrock
Copy link
Contributor Author

rjbrock commented Jan 5, 2015

@greenrobot Any updates?

@greenrobot
Copy link
Owner

Did not have the chance have a closer look yet.

@rjbrock
Copy link
Contributor Author

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

@jromero
Copy link

jromero commented Feb 23, 2015

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

@andreas-
Copy link

+1

@greenrobot greenrobot merged commit df91aeb into greenrobot:master Feb 4, 2016
@rjbrock
Copy link
Contributor Author

rjbrock commented Feb 4, 2016

Thanks!

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.

None yet

6 participants