idanatz / OneAdapter

A Viewholderless Adapter for RecyclerView, who supports builtin diffing, states (paging, empty...), events (clicking, swiping...), and more.
MIT License
470 stars 45 forks source link

SetItems very slow #21

Closed NoahReyson closed 4 years ago

NoahReyson commented 4 years ago

Disclaimer: I measured the execution time of this function by displaying timestamp just before and just after call. Apparently, it only takes 1ms to execute. If this function does not use different threads, the problem may not come from it.

I am trying to display some objects but it seems to be taking much longer than expected. The GUI freezes for 6~7s so I am sure it's a process executed on the Main Thread.

Here my Diffable :

class ExpansionDiffable(val expansion: Expansion): Diffable {

    override fun areContentTheSame(other: Any): Boolean {
        return other is ExpansionDiffable
                && other.expansion.count == expansion.count
                && other.expansion.countOwned == expansion.countOwned
                && other.expansion.price == expansion.price
                && other.expansion.priceOwned == expansion.priceOwned
                && other.expansion.body == expansion.body
    }

    override fun getUniqueIdentifier(): Long {
        return expansion.idExpansion.toLong()
    }
} 

And here my Module

class ExpansionModule: ItemModule<ExpansionDiffable>() {
    override fun onBind(model: ExpansionDiffable, viewBinder: ViewBinder) {
        val name: TextView = viewBinder.findViewById(R.id.name)
        val progress: CircleProgressView = viewBinder.findViewById(R.id.progress)
        val abbreviation: TextView = viewBinder.findViewById(R.id.abbreviation)
        val count: TextView = viewBinder.findViewById(R.id.count)
        val worth: TextView = viewBinder.findViewById(R.id.worth)
        val releaseDate: TextView = viewBinder.findViewById(R.id.releaseDate)

        name.text = model.expansion.name
        progress.set(model.expansion.countRatio)
        abbreviation.text = model.expansion.abbreviation
        releaseDate.text = model.expansion.releaseDateLimited

        if (!model.expansion.isReleased)
            releaseDate.textColor = context.getColor(R.color.red_A200)
        else
            releaseDate.textColor = context.getColor(R.color.md_black_1000)

        count.text = "${model.expansion.countOwned}/${model.expansion.count}"
        worth.text = model.expansion.priceOwned.toPriceString()
    }

    override fun provideModuleConfig(): ItemModuleConfig = object: ItemModuleConfig() {
        override fun withLayoutResource(): Int = R.layout.expansion_module
    }
}

For information, there are around 500 Expansions to display. Do you think that the slowdown observed could come from your library?

idanatz commented 4 years ago

Hi NoahReyson,

SetItems return in 1ms because it's spinning a background thread to do the diffing and calculations and returns.

I've tried the sample project using the complete example, generated 1000 items bonded to multiple modules and even inserting to RoomDB and observing with RxJava2 (which obviously slow down things instead of straight from memory) and it took about 1-1.5 second. Please try it as well.

Can you give me more information? what is your XML? what is the code that calling the adapter? how long the data take to get to the adapter? can you provide a minimal project where this issue reproduces?

NoahReyson commented 4 years ago

I can give you some more info

Here the Module layout

<?xml version="1.0" encoding="utf-8"?>
<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:app="http://schemas.android.com/apk/res-auto"
    android:id="@+id/background"
    android:layout_width="match_parent"
    android:layout_height="wrap_content"
    android:orientation="horizontal">

    <at.grabner.circleprogress.CircleProgressView
        android:id="@+id/progress"
        android:layout_width="80dp"
        android:layout_height="80dp"
        android:layout_margin="10dp"
        app:cpv_autoTextColor="true"
        app:cpv_autoTextSize="true"
        app:cpv_barStrokeCap="Butt"
        app:cpv_innerContourSize="0dp"
        app:cpv_outerContourSize="0dp"
        app:cpv_showUnit="true"
        app:cpv_startAngle="90"
        app:cpv_text="33"
        app:cpv_unit="%"
        app:cpv_unitPosition="right_top"
        app:cpv_value="33" />

    <TextView
        android:id="@+id/name"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:layout_alignParentTop="true"
        android:layout_marginTop="5dp"
        android:layout_marginEnd="15dp"
        android:layout_marginBottom="5dp"
        android:layout_toEndOf="@+id/progress"
        android:minHeight="40dp"
        android:text="Metal Raiders"
        android:textColor="#222222"
        android:textSize="16sp"
        android:textStyle="bold" />

    <LinearLayout
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:layout_below="@+id/name"
        android:layout_marginEnd="15dp"
        android:layout_toEndOf="@+id/progress"
        android:orientation="horizontal">

        <LinearLayout
            android:layout_width="0dp"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:orientation="vertical">

            <TextView
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:text="@string/ABBR"
                android:textSize="10sp" />

            <TextView
                android:id="@+id/abbreviation"
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:text="MRD"
                android:textColor="@color/md_black_1000"
                android:textSize="12sp" />

        </LinearLayout>

        <LinearLayout
            android:layout_width="0dp"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:orientation="vertical">

            <TextView
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:text="@string/COUNT"
                android:textSize="10sp" />

            <TextView
                android:id="@+id/count"
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:text="12/30"
                android:textColor="@color/md_black_1000"
                android:textSize="12sp" />

        </LinearLayout>

        <LinearLayout
            android:layout_width="0dp"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:orientation="vertical">

            <TextView
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:text="@string/WORTH"
                android:textSize="10sp" />

            <TextView
                android:id="@+id/worth"
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:text="31.20€"
                android:textColor="@color/md_black_1000"
                android:textSize="12sp" />

        </LinearLayout>

        <LinearLayout
            android:layout_width="0dp"
            android:layout_height="wrap_content"
            android:layout_weight="1.5"
            android:orientation="vertical">

            <TextView
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:text="@string/DATE"
                android:textSize="10sp" />

            <TextView
                android:id="@+id/releaseDate"
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:text="10 octobre 2019"
                android:textColor="@color/md_black_1000"
                android:textSize="12sp" />

        </LinearLayout>

    </LinearLayout>

</RelativeLayout>

And here my Expansion class

class Expansion(json: String) {
    constructor(cursor: Cursor): this(cursor.getString(cursor.getColumnIndex(MKMDatabaseExpansionsHelper.COLUMN_CONTENT))) {
        updateDate = Date(cursor.getLong(cursor.getColumnIndex(MKMDatabaseExpansionsHelper.COLUMN_UPDATE_DATE)))
        count = cursor.getInt(cursor.getColumnIndex("count"))
        countOwned = cursor.getInt(cursor.getColumnIndex("countOwned"))
        price = cursor.getDouble(cursor.getColumnIndex("price"))
        priceOwned = cursor.getDouble(cursor.getColumnIndex("priceOwned"))
    }

    val body: JSONObject = JSONObject(json)

    val idExpansion: Int
        get() {
            return body.getInt("idExpansion")
        }
    val name: String
        get() {
            return name(Settings.language) ?: body.getString("enName")
        }
    val abbreviation: String
        get() {
            return body.getString("abbreviation")
        }
    val releaseDateString: String
        get() = body.getString("releaseDate")

    val releaseDate: Date
        get() = releaseDateString.toDate()!!

    val releaseDateLimited: String
        get() = releaseDate.format(Date(946684800000), null, "Unknown date", "dd/MM/yyyy")

    val isReleased: Boolean
        get() {
            return releaseDate < Date()
        }

    var count: Int = 0
    var countOwned: Int = 0
    val countRatio: Double
        get() = if (count == 0) 0.0 else (countOwned.toDouble() / count.toDouble())

    var price: Double = 0.0
    var priceOwned: Double = 0.0

    var updateDate: Date = Date()

    fun name(language: Language = Language.FRENCH): String? {
        val locs: JSONArray = body.getJSONArray("localization")
        for (i in 0 until locs.length()) {
            val loc = locs[i] as JSONObject
            if (language.id == loc.getString("idLanguage").toInt())
                return loc.getString("name")
        }
        return null
    }

    override fun toString(): String {
        return body.toString(2)
    }

}

I can't give you a sample project because it requires some database base etc. I can tell you that the Expansions are loaded from a SQLite database but I'm sure it loads in 5ms so the problem is not here. There really is a 6-7s lapse between the call of SetItems and the GUI refresh during which it's frozen.

What really bugs me is that I use your lib in others activities where it loads 300 items in less than a seconde.

idanatz commented 4 years ago

Ok from what you attached I believe the slowing down will occur even with Valina adapter and it has nothing to do with the library. here are some advises:

  1. your XML layout is pretty nested... all of this inner linear layouts have performance implications, try to move to a more flat hierarchy with constraint layout.

  2. more important: your Expansion model is parsing some of its members at construct time (like count and countOwned) and some at runtime (everywhere you are using a get() function). why is that? each time you call a get function you are searching inside the JSON which takes time even if the JSON is not that big. instead of : val abbreviation: String get() { return body.getInt("abbreviation") } write: val abbreviation: String = return body.getInt("abbreviation")

this way the value is calculated to the member-only once, it's possible because you are getting the JSON in the constructor, there is no need for a getter function in some of your scenarios.

NoahReyson commented 4 years ago

Ok so I created a sample project with only the problematic activity. The freeze seems a bit shorter than on the real app but still longer than expected (about 4-5s on a Pixel 3a). I load the data from a local ressource file, not from a SQLite database, but it makes no diference. The main layout is a bit messy because I deleted some other irrelevant elements for this sample but it loads without issue.

If you have any idea...

SampleApp.zip

idanatz commented 4 years ago

I found your issue. The Recycler View is going crazy in your layout, due to the fact that it is declared as wrap_content. If you will put a log in the onBInd method you will see that it is being called for each item on the list, not only for the visible ones. This is why the adapter is slow, it's rendering ALL your item at the same time instead of just the visible once (basically acting like ListView and not RecyclerView) Once I set the ReyclerView to a fixed hight everything worked as should, its not an adapter issue.

one last thing, you are working with 1.2.0, the last version is 1.5.1 (there will be some breaking API changes)

NoahReyson commented 4 years ago

OK, thanks.

But if I set the height of the Recycler to a fixed value, it will not fill the entire cardview. Moreover, it will scroll independently of the cardview (even with nestedScrollingEnabled="true" in the cardview).

If I set it to match_parent, the original issue remains the same.

I will make some tests to find a solution.

Thanks for your help.

NoahReyson commented 4 years ago

Ok, after thinking about it for a while, I think it's obvious the CardView cannot calculate its own size without knowing the total size of the RecyclerView, and thus calculating all of its elements (the issue).

Unless you have an idea, I think I will look for another design without a RecyclerView inside a CardView.

idanatz commented 4 years ago

The question is what UX you are trying to achieve, and why a Recycler needs to be inside a nested scroll view. (which causes all the issues here, without it you can use wrap content with no issues)

I would search StackOverflow for the combination of Recycler & NestedScrollView, there is something there that you need to figure out. Good luck.

NoahReyson commented 4 years ago

I am trying to make a list of CardView (there is only one in the sample I sent but imagine that there could be several like Google News). Except that one CardView contains a recycler with 500 elements. What scrolls is therefore not the Recycler itself but all of the CardViews. The height of the CardView containing the Recycler is therefore equal to the total height of the Recycler with all of its elements.

I send you again the sample project just to visualize my idea. Notice that I reduced to 10 the number of elements in the Recycler to avoid the freeze but it's still present.

SampleApp.zip

idanatz commented 4 years ago

The problem is that you are not actually using the recycle view here... you are just loading all the items on startup with not recycling abilities. Your card is the size of all the items and the adapter onBind calls for all items on startup. With this case you can just remove the recycler... it doesn't do anything its just a ListView.

My suggestion (and it is now fully worked out of course) is that the size of the recycler view should not the size of all the items it holds. Recycle view height can be at most the height of the screen and holds the number of items the can fit the screen, all the other items should be recycled when you scroll. if you know you will have 500 items I wound measure the screen hight at runtime and set the size of the recycler view to this size.

The things that you need to figure out is how the scroll should behave between the outer scroll view and the inner recycler view. once the recycler takes the full screen of your list the scroll should move to the recycler and then it will render only the visible items and not all the items at once.