Skip to content
This repository has been archived by the owner on Apr 20, 2021. It is now read-only.

Implementation seems very heavy #13

Open
ScottPierce opened this issue May 14, 2016 · 13 comments
Open

Implementation seems very heavy #13

ScottPierce opened this issue May 14, 2016 · 13 comments

Comments

@ScottPierce
Copy link

The implementation seems very heavy when I look at the source code. I stopped being as worried about garbage collection with ART, but it seems like each stage of the Activity or Fragment lifecycle is creating at least 1, if not more objects:

return callFunction("getContext()", new PluginCall<FragmentPlugin, Context>() {
            @Override
            public Context call(final NamedSuperCall<Context> superCall,
                    final FragmentPlugin plugin, final Object... args) {

                return plugin.getContext(superCall);

            }
        }, new SuperCall<Context>() {
            @Override
            public Context call(final Object... args) {
                return getFragment().getContext__super();
            }
        });

I have no measured data on this, but couldn't this be easily implemented without the overhead of multiple function objects created at each method call?

@passsy
Copy link
Owner

passsy commented May 16, 2016

One of the reasons, why this lib is at version 0.1.X instead of 1.X 😉

Right now, two Objects are created for each method call, one for the method call and one for the super call. I agree that this is not ideal. I did a few tests around memory when writing this lib and saw the memory going up due to those objects. I was expecting this. When triggering the GC everything was perfectly freed. I had bigger problems to fight these days.

I already played around with code generation to reduce the allocation of new objects. I came up with a solution which inlines the callFunction() method. It removed one Object allocation but introduced a new method for each method which would double the already high method count of this lib.

Right now I prefer a solution which lazy initializes and caches those two objects as member variables.

    private PluginCallVoid<ActivityPlugin> mAddContentViewMethodCall;

    private SuperCallVoid mAddContentViewSuperCall;

    public void addContentView(final View view, final ViewGroup.LayoutParams params) {
        if (mPlugins.isEmpty()) {
            return;
        }
        if (mAddContentViewSuperCall == null) {
            mAddContentViewSuperCall = new SuperCallVoid() {
                @Override
                public void call(final Object... args) {
                    getCompositeActivity().addContentView__super((View) args[0],
                            (ViewGroup.LayoutParams) args[1]);
                }
            };
        }

        if (mAddContentViewMethodCall == null) {
            mAddContentViewMethodCall = new PluginCallVoid<ActivityPlugin>() {
                @Override
                public void call(final NamedSuperCall<Void> superCall,
                        final ActivityPlugin plugin, final Object... args) {
                    plugin.addContentView(superCall, (View) args[0],
                            (ViewGroup.LayoutParams) args[1]);
                }
            };
        }

        callHook("addContentView(View, ViewGroup.LayoutParams)",
                mAddContentViewMethodCall, mAddContentViewSuperCall, view, params);
    }


    private SuperCall<Boolean> mBindServiceActivitySuper;

    private PluginCall<ActivityPlugin, Boolean> mBindServiceMethodCall;

    public boolean bindService(final Intent service, final ServiceConnection conn,
            final int flags) {
        if (mPlugins.isEmpty()) {
            return getCompositeActivity().bindService__super(service, conn, flags);
        }

        if (mBindServiceMethodCall != null) {
            mBindServiceMethodCall = new PluginCall<ActivityPlugin, Boolean>() {
                @Override
                public Boolean call(final NamedSuperCall<Boolean> superCall,
                        final ActivityPlugin plugin, final Object... args) {
                    return plugin.bindService(superCall, (Intent) args[0],
                            (ServiceConnection) args[1], (int) args[2]);

                }
            };
        }
        if (mBindServiceActivitySuper != null) {
            mBindServiceActivitySuper = new SuperCall<Boolean>() {
                @Override
                public Boolean call(final Object... args) {
                    return getCompositeActivity()
                            .bindService__super((Intent) args[0], (ServiceConnection) args[1],
                                    (int) args[2]);
                }
            };
        }
        return callFunction("bindService(Intent, ServiceConnection, int)",
                mBindServiceMethodCall, mBindServiceActivitySuper, service, conn, flags);
    }

I'm not sure if caching the call functions with a SoftReference instead of a strong one would be better.

I appreciate tips or ideas for better solutions.

@darrillaga
Copy link

darrillaga commented May 25, 2016

Hi, having like a general Map with name -> [PluginCall, SuperCall] could be a posible solution, what do you think?

I think I can dedicate some time to do that

@passsy
Copy link
Owner

passsy commented Jul 11, 2016

Next release will focus on performance

@passsy
Copy link
Owner

passsy commented Jul 22, 2016

Part 1 of the performance improvements: #23

@ScottPierce
Copy link
Author

@passsy any idea when part 2 performance improvements will be coming?

@passsy
Copy link
Owner

passsy commented Jan 25, 2017

End of February I'll continue bringing this project forward

@ScottPierce
Copy link
Author

@passsy You mentioned in your above PR

I'd call this desirable because you have full control from a plugin. You could write a plugin which reimplements setContentView inflating the layouts not into the root container but inside a layout of your choice (and do not call super).

What if 2 plugins both override setContentView, how is it handled?

@passsy
Copy link
Owner

passsy commented Jan 25, 2017

Like inheriitance, the last layer wins. In terms of plugins, the last plugin which was added. See the wiki for more details: https://github.com/passsy/CompositeAndroid/wiki/Ordering-of-plugins-and-the-result-on-the-call-order

@yiyocx
Copy link

yiyocx commented Feb 28, 2017

You're doing a great job with the performance improvements. I just have a question: How are you attaching the plugin with the activity lifecycle events in the internal implementation?

@Rainer-Lang
Copy link

@passsy Any news on this? Will the performance 2 PR be merged soon?

@markusressel
Copy link

Although I don't seem to have performance problems in my app I would also like to see those improvements make it into a new release. @passsy What's holding you back?

@passsy
Copy link
Owner

passsy commented Feb 16, 2018

I'm not happy with a reflection based solution.

  1. it might break
  2. it's costly, especially because most work happens at initialization
  3. it's hard to measure and balance if it's cheaper to do the reflection call or just call the method 100 times

This project was an experiment which was very valuable the time I wrote it. But due to lifecycle listeners in the support library for Activity and Fragments it lost most of its value. Only a few methods can't be intercepted in the support lib and it's questionable if those should be interceptable.

@markusressel
Copy link

markusressel commented Feb 16, 2018

Well you may be right, Android lifecycle listeners provide a decent way to interact with the default lifecycle methods of an activity or fragment. I'm still in the process of understanding them though so I'm not able to do a fully qualified comparison with CompositeAndroid.

In my case I was able to write an Activity Plugin with your library that intercepts the "setContentView()" method and creates a wrapper layout of the parent layout to add it's own stuff (specifically a lock screen in my case). That would not have been possible with lifecycle listeners although there may still be a way to achieve what I was trying to do.

I'm a little sad hearing that the support for this project is now limited.
I will investigate further into android lifecycle listeners and see what they can do instead.

Thx again for the quick response and the hard work you put into this library.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants