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

fix: memory leaks from tablayout / recyclerview #494

Merged
merged 3 commits into from Jan 18, 2019

Conversation

Projects
None yet
3 participants
@verno3632
Copy link
Contributor

verno3632 commented Jan 15, 2019

Issue

Overview (Required)

This fixes 2 memory leaks

SessionPageFragment.sessionTabLayout

These 2 images show 1 leak

RecyclerView

RecyclerView should release adapter when the fragment destroy views.
https://stackoverflow.com/questions/35520946/leak-canary-recyclerview-leaking-madapter

The target is BottomSheetDaySessionsFragment and BottomSheetFavoriteSessionsFragment.

@jmatsu-bot

This comment has been minimized.

Copy link
Collaborator

jmatsu-bot commented Jan 15, 2019

@@ -89,6 +89,11 @@ class SessionPagesFragment : DaggerFragment() {
}
}

override fun onDestroyView(){

This comment has been minimized.

@jmatsu-bot

jmatsu-bot Jan 15, 2019

Collaborator
🚫 Missing spacing before “{“
@jmatsu
Copy link
Member

jmatsu left a comment

Thank you for your contribution. My comments do not require changes but investigation.

@@ -142,6 +142,11 @@ class BottomSheetDaySessionsFragment : DaggerFragment() {
}
}

override fun onDestroyView() {
binding.sessionsRecycler.adapter = null

This comment has been minimized.

@jmatsu

jmatsu Jan 16, 2019

Member

The root cause of these memory leaks seems that these Fragments have adapter instances with strong references. We may need to have a look at the inside of adapters as well.
So, does this work properly even if this Fragment is re-attached? If no, we should instantiate the adapter every onViewCreated instead.

This comment has been minimized.

@verno3632

This comment has been minimized.

@jmatsu

jmatsu Jan 18, 2019

Member

Gotcha. Thanks. 👍

@jmatsu-bot

This comment has been minimized.

Copy link
Collaborator

jmatsu-bot commented Jan 16, 2019

@jmatsu-bot

This comment has been minimized.

Copy link
Collaborator

jmatsu-bot commented Jan 16, 2019

Apk comparision results

Property Summary
New File Size 13791727 Bytes. (13.15 MB)
File Size Change -9154 Bytes. (-8.94 KB)
Download Size Change -4620 Bytes. (-4.51 KB)
New Method Reference Count 153801
Method Reference Count Change 41
New Number of dex file(s) 4
Number of dex file(s) Change 0

Generated by 🚫 Danger

@jmatsu-bot

This comment has been minimized.

Copy link
Collaborator

jmatsu-bot commented Jan 16, 2019

Asserted successfully. 💯

Generated by 🚫 Danger

@jmatsu

jmatsu approved these changes Jan 18, 2019

Copy link
Member

jmatsu left a comment

LGTM. Thank you so much!

@jmatsu jmatsu merged commit 6805e8d into DroidKaigi:master Jan 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.