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

[AC-3673] Android: Optimize removeAllChildren #8026

Closed
wants to merge 4 commits into from
Closed

[AC-3673] Android: Optimize removeAllChildren #8026

wants to merge 4 commits into from

Conversation

m1ga
Copy link
Contributor

@m1ga m1ga commented May 27, 2016

@m1ga m1ga changed the title [AC-3673] Optimize removeAllChildren [work in progress] [AC-3673] Android: Optimize removeAllChildren [work in progress] May 27, 2016
@sgtcoolguy sgtcoolguy modified the milestone: 6.0.0 Jun 17, 2016
@sgtcoolguy sgtcoolguy removed the 6.0.0 label Jun 17, 2016
@m1ga m1ga changed the title [AC-3673] Android: Optimize removeAllChildren [work in progress] [AC-3673] Android: Optimize removeAllChildren Jun 20, 2016
@sgtcoolguy
Copy link
Contributor

@m1ga Is this now ready for review? cc @ashcoding

@build
Copy link
Contributor

build commented Jun 23, 2016

Can one of the admins verify this patch?

@sgtcoolguy
Copy link
Contributor

ok to test

@ashcoding
Copy link
Contributor

@m1ga if it's good to test, I'll give it a look.

@m1ga
Copy link
Contributor Author

m1ga commented Jun 24, 2016

Yes,please test it. I left some comments in the jira ticket about the listview

@ashcoding
Copy link
Contributor

Functionally tested it out.
Before:
4026ms
4293ms
3910ms
4207ms
3979ms

After:
264ms
342ms
338ms

if (nv instanceof ViewGroup) {
ViewGroup vg = (ViewGroup) nv;
if (TiApplication.isUIThread()) {
vg.removeAllViews();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code uses the method remove(child) to remove the TiViewProxy child view. This in turns call handleRemove().

In the handleRemove, the TiViewProxy is released by the method releaseViews(). This is not done here. Would there be a leak caused from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do any memory checks but according to http://stackoverflow.com/a/29942391/5193915 it will null all child views but it won't call release() on all children, which is the other part of handleRemove(). I'll try to see if I can do some memory testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashcoding
Copy link
Contributor

I tried this with 500 views.

Prior to this, I get 77,300~ objects after GC (before GC, it was around 121,000). Although it takes a lot of time.

With this PR, I get 121,404~ objects after GC. Although it is very fast.

In short, the objects are not being released. 😭

@m1ga
Copy link
Contributor Author

m1ga commented Jul 13, 2016

😢 ok, perhaps there is another way of releasing the objects and still keep the removeAllViews(). Just strange that in my test run the objects went down again. But I'm not that familiar with memory testing so I might have done something wrong

@ashcoding
Copy link
Contributor

I'll give this a look again when I have time if I can.

@ashcoding
Copy link
Contributor

If you found time to look at this more, do let me know.

@hansemannn hansemannn modified the milestones: 6.1.0, 6.0.0 Aug 7, 2016
@hansemannn
Copy link
Collaborator

@m1ga any progress here?

@m1ga
Copy link
Contributor Author

m1ga commented Oct 15, 2016

@hansemannn sorry, no. It looks like handleRemove() has to be called on all objects before removing them and then you have to do the loop over all objects again (like it is now). Since there is a JIRA ticket you might close this PR. Perhaps somebody else comes up with a clever idea to speed up removing of objects

@hansemannn hansemannn closed this Oct 15, 2016
@m1ga m1ga deleted the removeall branch December 10, 2016 14:20
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

5 participants