CompositeAndroid icon indicating copy to clipboard operation
CompositeAndroid copied to clipboard

Implementation seems very heavy

Open ScottPierce opened this issue 8 years ago • 13 comments

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?

ScottPierce avatar May 14 '16 06:05 ScottPierce

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.

passsy avatar May 16 '16 22:05 passsy

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

darrillaga avatar May 25 '16 17:05 darrillaga

Next release will focus on performance

passsy avatar Jul 11 '16 09:07 passsy

Part 1 of the performance improvements: #23

passsy avatar Jul 22 '16 15:07 passsy

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

ScottPierce avatar Jan 25 '17 17:01 ScottPierce

End of February I'll continue bringing this project forward

passsy avatar Jan 25 '17 19:01 passsy

@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?

ScottPierce avatar Jan 25 '17 19:01 ScottPierce

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

passsy avatar Jan 25 '17 19:01 passsy

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?

yiyocx avatar Feb 28 '17 05:02 yiyocx

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

Rainer-Lang avatar Aug 30 '17 14:08 Rainer-Lang

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?

markusressel avatar Feb 16 '18 13:02 markusressel

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.

passsy avatar Feb 16 '18 19:02 passsy

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.

markusressel avatar Feb 16 '18 20:02 markusressel