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

[TIMOB-23715] Android: Fix TiBaseActivity and TiUIView memory leaks #8616

Merged
merged 1 commit into from Dec 7, 2016

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Nov 16, 2016

  • Fix memory leaks from various object references in TiListView TiBaseActivity and TiUIView
TEST CASE
var w = Ti.UI.createWindow({layout: 'vertical'}),
    a = Ti.UI.createButton({title: 'Ti.UI.View', width: '90%'}),
    b = Ti.UI.createButton({title: 'Ti.UI.ListView', width: '90%'}),
    iterations = 5;

a.addEventListener('click', function() {
    for (var i = 0; i < iterations; i++) {
        setTimeout(function () {
            var w = Ti.UI.createWindow(),
                a = Ti.UI.createView({
                    width: Ti.UI.FILL,
                    height: Ti.UI.FILL,
                    backgroundColor: 'red'
                });
            w.addEventListener('open', function() {
                w.close();
            });
            w.add(a);
            w.open();
        }, 250*i);
    }
});

b.addEventListener('click', function() {
    for (var i = 0; i < iterations; i++) {
        setTimeout(function () {
            var w = Ti.UI.createWindow(),
                a = Ti.UI.createListView();
            w.addEventListener('open', function() {
                w.close();
            });
            w.add(a);
            w.open();
        }, 250*i);
    }
});

w.add(a);
w.add(b);
w.open();
  • Use Android Monitor to execute GC and monitor memory allocation

JIRA Ticket

@garymathews garymathews added this to the 6.1.0 milestone Nov 16, 2016
@garymathews
Copy link
Contributor Author

garymathews commented Nov 17, 2016

test this please

@garymathews
Copy link
Contributor Author

test this please

@frankieandone
Copy link
Contributor

Running your test case and the one reported in the ticket. I still get leaked activities from both test cases. Seems there are still contexts leaking in TiCompositeLayout for one example.

@garymathews
Copy link
Contributor Author

@fmerzadyan Updated PR

@garymathews
Copy link
Contributor Author

test this please

@frankieandone
Copy link
Contributor

frankieandone commented Nov 29, 2016

The leaks are whittled down 👍 but a few still remain. Add this snippet to TiListView.release().

if (inflater != null) {
    inflater = null;
}

After adding this your test case has no leaks but there are leaks in original test case. Maybe populate your list view and inspect list view items: TiBaseListViewItem?

@garymathews
Copy link
Contributor Author

garymathews commented Dec 5, 2016

@fmerzadyan The inflater is static so that should be fine. If everything else is okay it should be good to merge 👍

@frankieandone
Copy link
Contributor

frankieandone commented Dec 7, 2016

Mutable static variables are not garbage collected. Developers must explicitly nullify these variables. Plus it was flagged up when profiling Java heap.

@garymathews
Copy link
Contributor Author

You're right, but it's done purposefully to reuse the inflater object again when a new ListView is created.

Which is why this is in the constructor, to not recreate the inflater object again:

if (inflater == null) {
	inflater = (LayoutInflater)activity.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
}

@frankieandone
Copy link
Contributor

CR && FT PASS

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

Successfully merging this pull request may close these issues.

None yet

2 participants