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

Investigate adding automatic bind in default dispatchers. #36

Closed
glung opened this issue Mar 16, 2016 · 12 comments
Closed

Investigate adding automatic bind in default dispatchers. #36

glung opened this issue Mar 16, 2016 · 12 comments

Comments

@glung
Copy link
Contributor

glung commented Mar 16, 2016

This could be done like we do w/ base activities and fragments.

  • The cons being it will trigger more binding attempt which has a cost.

On the example we would go from binding:

com.soundcloud.lightcycle.sample.real_world.HomeActivity
com.soundcloud.lightcycle.sample.real_world.HomePresenter

to binding:

com.soundcloud.lightcycle.sample.real_world.HomeActivity
com.soundcloud.lightcycle.ActivityLightCycleDispatcher
java.lang.Object
com.soundcloud.lightcycle.sample.real_world.HomePresenter
  • The pro is that then, we would not need to care about binding anymore when using dispatchers.
@NikoYuwono
Copy link
Contributor

So continuing from conversation in #48 I think we can also consider Bytecode Manipulation like in Retrolambda or Realm.

Pro

  • Cheap cost
  • No need to modify current API

Cons

  • Need to write plugin which means more dependency to the user too (e.g. Realm case)
  • Harder to debug
  • Don't work with Jack

Usage in other project

The other thing that need to consider is, if we use this, we need define our own rule when the binding will happen (Currently it's always in onCreate).
Which can be a good thing to prevent user mistake to call Lightcycles.bind() in wrong place or maybe forgot to call Lightcycles.bind() at all.

@glung
Copy link
Contributor Author

glung commented Nov 25, 2016

Interesting, idea.

This makes me think of a 3rd idea. With v2 we may have an annotation like @LightCycleHost or @LightCycleBaseClass so potentially we would not provide base classes but generate those (cc @michaelengland). Eventually, we could have @LightCycleHost(autobind = true/false) or bind only when the processor sees a lightcycle.

Pros

  • Parameterizable per class (potentially, no extra binding)
  • One processor to generates all the boiler place

Cons

  • Implementation may be complex

Open question

@glung
Copy link
Contributor Author

glung commented Nov 25, 2016

Ultimately, I am wondering what is the real cost to bind all the time ...
How do you think we could measure that ?

Maybe we take example on RxJava - since they have a focus on performances - https://github.com/ReactiveX/RxJava/blob/1.x/src/perf/java/rx/SubscribingPerf.java http://openjdk.java.net/projects/code-tools/jmh/ .

@NikoYuwono
Copy link
Contributor

I like your 3rd idea! But we must be careful when design it. I think we should do that after API v2 design completed. What do you think?

Hmmm I think it's better to use microbenchmark like what you said above with JMH or Google's Caliper which also have Android fork named Spanner.

@glung
Copy link
Contributor Author

glung commented Nov 28, 2016

I like your 3rd idea! But we must be careful when design it. I think we should do that after API v2 design completed. What do you think?

Makes sense

Hmmm I think it's better to use microbenchmark like what you said above with JMH or Google's Caliper which also have Android fork named Spanner.

I don't know about microbenchmark, I just read this answer on stack overflow.

Can you elaborate a bit ?
Do you have examples ?

@NikoYuwono
Copy link
Contributor

To put it simple, microbenchmark is benchmarking a little piece of code (Performance depends on how those few lines are compiled). Because microbenchmark is like a magnifying glass, even if we can find a problem with it, it doesn't mean that problem is the main cause of performance problem of our app/library.

On why I think microbenchmark can be used in our case because you asked about the performance of the binding, rather than the whole program. In this case, using profiler would be overkill in my opinion.

For examples, I think the RxJava links that you shared before is a collection of microbenchmark.

And by the way, I think this question and the most voted answer is really good reference for microbenchmark
http://stackoverflow.com/questions/504103/how-do-i-write-a-correct-micro-benchmark-in-java

@glung
Copy link
Contributor Author

glung commented Dec 5, 2016

I had another look at it, and I am under the impression we are overthinking it.

For context :

  • LightCycles.bind(LightCycleDispatcher) uses reflexions to find the MyClass$LightCycleBinder.
  • MyClass$LightCycleBinder does not.

So far, we have bindings to both Fragment and Activity classes per default. If we add it to the default dispatchers, i.e. SupportFragmentLightCycleDispatcher, FragmentLightCycleDispatcher, and ActivityLightCycleDispatcher it means we will call LightCycles.bind(LightCycleDispatcher) all the time.

Does it mean that it only penalizes Dispatchers w/o @LighCycle ? I think it is....

Now, I believe it is actually more harmful to have an inconsistent API (autobinding for Activity and Fragment class but not the other dispatchers) than the performance cost actually is.

TBH, I think this issue was created in the early days and we never took the time to look back at it.

Thoughts ?

@NikoYuwono
Copy link
Contributor

I had another look at it, and I am under the impression we are overthinking it.

Agreed

Now, I believe it is actually more harmful to have an inconsistent API (autobinding for Activity and Fragment class but not the other dispatchers) than the performance cost actually is.

Agreed, like I wrote above currently there are no official rule for when to do LightCycles.bind().
In Default Activity and Fragment that have autobinding, the call to LightCycles.bind() is located in onCreate() but if the user didn't take a look into that and use his own dispatcher and put LightCycles.bind() inside onResume(), this can also be a source of problem.

I have two idea :

First

  • Keep the auto-binding for Activity and Fragment
  • Add rule for when the LightCycles.bind() need to be called in Dispatchers and add them to Readme (Possibly in Built-in dispatcher section)

Pros

  • No Breaking Changes

Cons

  • Incosistent API

Second

  • Remove the auto-binding for Activity and Fragment
  • Add rule for when the LightCycles.bind() need to be called and add them to Readme

Pros

  • Consistent API

Cons

  • Breaking Changes

In my opinion, we can also think LightCycleActivity and LightCycleFragment as different thing from the dispatchers and think it like this

LightCycleActivity & LightCycleFragment

  • Not a Lightcycle
  • Owner of Views
  • Owner of Lightcycles (including Dispatchers)

Dispatcher

  • Is a Lightcycle
  • Owner of other Lightcycles (including Dispatchers)

With this argument, if we want to avoid breaking changes, we can stick with the first idea.

What do you think about this?

@glung
Copy link
Contributor Author

glung commented Dec 6, 2016

I think option 1 is better for the users for the lib and if we have perf issues we will address them later. I am confident.

About your last point, I think we ca refer to #48, unless you think it does not capture your thoughts :

LightCycle : a representation of the Activity or Fragment's lifecycle.
Host : the instance of the Activity or Fragment's lifecycle as seen by the LightCycle.
LifeCycle : an Activity or a Fragment

@NikoYuwono
Copy link
Contributor

OK then! I'll create a pull request to make the Readme clearer and let's discuss about it there!
After that maybe we can close this issue?

@glung
Copy link
Contributor Author

glung commented Dec 9, 2016

No problem! Can you review this PR #82 ?

@NikoYuwono
Copy link
Contributor

OK! Will review it!

@glung glung closed this as completed in 265478d Dec 10, 2016
glung added a commit that referenced this issue Dec 15, 2016
Dispatchers rely on equals to deduce the list of attached light cycles. Lifted light cycles must implement it.
Related to #36
glung added a commit that referenced this issue Dec 15, 2016
Bind the fragment only on the first onAttach call.
Related to #36
glung added a commit that referenced this issue Dec 16, 2016
* Lifted LightCycles of the same class are equals.
* Fix fragment dispatchers bound multiple times.

Related to #36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants