varabyte / kobweb

A modern framework for full stack web apps in Kotlin, built upon Compose HTML
https://kobweb.varabyte.com
Apache License 2.0
1.46k stars 65 forks source link

Kobweb components are recomposing too much #157

Closed bitspittle closed 1 year ago

bitspittle commented 2 years ago

Thanks a ton to a user on my discord:

Whenever you pass in a modifier directly to a composable, like
ParentComposable(text: String) {
  ChildComposable(
    Modifier...
  )
}

I've noticed that everytime ParentComposable()  recomposes, ChildComposable() recomposes too, even if nothing changes about the Modifier being passed in. Is this intentional?
Because on Android, ChildComposable() wouldn't recompose if the modifier didn't change.
Similarly, if you do something using the attrs parameter like
ParentComposable(text: String) {
  ChildComposable(
    attrs = {
      ...
    }
  )
}

ChildComposable() doesn't recompose if the attrs parameter doesn't change

To avoid the unnecessary recomposition of ChildComposable(), I've found you need to do something like
val modifier = remember { Modifier... }
ParentComposable(text: String) {
  ChildComposable(
    modifier
  )
}

I did some more digging into this, and this seems to happen because your Modifier/ChainedModifier doesn't override hashCode(), which is what's used to check whether or not it should recompose
bitspittle commented 2 years ago

More:

You can see in the files here for example that basically all of the modifier extension functions are associated with a class that overrides the hashCode
https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/foundation/foundation-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/

which all get used in the CombinedModifier/ChainedModifier
override fun hashCode(): Int = outer.hashCode() + 31 * inner.hashCode()
bitspittle commented 1 year ago

Actually, reopening. As pointed out, this probably isn't fixed yet (since Style and Attr Modifier classes themselves don't have equals / hashcode implementations)

Concretely, I need to do this:

class AttrsModifier(internal val attrs: (AttrsScope<*>.() -> Unit)) : Modifier {
   override fun equals(other) { ??? }  
   override fun hashcode() { ??? }
}

class StyleModifier(internal val styles: (StyleScope.() -> Unit)) : Modifier {
   override fun equals(other) { ??? }  
   override fun hashcode() { ??? }
}

But that's not currently possible because even if I have an AttrsScope or StyleScope, I can't compare them to each other.

class AttrsModifier(internal val attrs: (ComparableAttrsScope<*>.() -> Unit)) : Modifier {
   override fun equals(other) = attrs() == other.attrs()
   override fun hashcode() = attrs().hashcode()
}
bitspittle commented 1 year ago

Here's a simple repro page to showcase the issue:

package helloworld.pages

import androidx.compose.runtime.*
import com.varabyte.kobweb.compose.foundation.layout.Column
import com.varabyte.kobweb.compose.foundation.layout.Row
import com.varabyte.kobweb.compose.ui.Modifier
import com.varabyte.kobweb.compose.ui.asAttributesBuilder
import com.varabyte.kobweb.compose.ui.modifiers.backgroundColor
import com.varabyte.kobweb.compose.ui.modifiers.fontSize
import com.varabyte.kobweb.compose.ui.modifiers.padding
import com.varabyte.kobweb.core.Page
import com.varabyte.kobweb.silk.components.text.SpanText
import helloworld.components.layouts.PageLayout
import org.jetbrains.compose.web.attributes.AttrsScopeBuilder
import org.jetbrains.compose.web.attributes.InputType
import org.jetbrains.compose.web.css.Color
import org.jetbrains.compose.web.css.fontSize
import org.jetbrains.compose.web.css.padding
import org.jetbrains.compose.web.css.px
import org.jetbrains.compose.web.dom.*
import org.w3c.dom.HTMLSpanElement

@Page
@Composable
fun HomePage() {
    PageLayout("Welcome to Kobweb!") {
        var count by remember { mutableStateOf(0) }
        var colorToggle by remember { mutableStateOf(false) }
        Div({ style { padding(25.px) } }) {
            Row {
                Button(attrs = {
                    onClick { count -= 1 }
                }) {
                    Text("-")
                }

                Button(attrs = {
                    onClick { count += 1 }
                }) {
                    Text("+")
                }

                Button(attrs = {
                    onClick { colorToggle = !colorToggle }
                }) {
                    Text("red/green")
                }
            }

            TextList(count, Modifier.backgroundColor(if (colorToggle) Color.red else Color.green).padding(15.px))
        }
    }
}

const val USE_KOBWEB_MODIFIERS = true

@Composable
fun TextList(count: Int, modifier: Modifier) {
    println("Recomposing TextList")
    Column(modifier) {
        for (i in 0..count) {
            if (USE_KOBWEB_MODIFIERS) {
                TextEntry("Test ${i + 1}", Modifier.fontSize(20.px))
            } else {
                TextEntryAlt("Test ${i + 1}", attrs = { style { fontSize(20.px) } })
            }
        }
    }
}

@Composable
fun TextEntry(text: String, modifier: Modifier) {
    println("Recomposing TextEntry")
    SpanText(text, modifier)
}

@Composable
fun TextEntryAlt(text: String, attrs: AttrBuilderContext<HTMLSpanElement>) {
    println("Recomposing TextEntryAlt")
    Span(attrs) {
        Text(text)
    }
}

Run this sample and open up a developer console to see output get printed there.

Basically, each time you press the "toggle color" button, only the outer composible (TextList) should recompose. The inner elements (TextEntry) should not change.

There's a constant in the middle of the code sample called USE_KOBWEB_MODIFIERS. If you change it to false, it uses the Web Compose attrs approach and that works fine. However, when using Kobweb modifiers, the inner composables always recompose.

bitspittle commented 1 year ago

Should be fixed in 0.9.14

bitspittle commented 1 year ago

Looks like this may have broken with Compose 1.2.1

bitspittle commented 1 year ago

Turns out chasing this uncovered an upstream bug: https://github.com/JetBrains/compose-jb/issues/2539

Sadly this issue will keep happening until we can migrate Kobweb past (it seems) Kotlin/JS 1.7.20.

I'll close this bug when I can do that, but my hands are basically tied for now. I have to imagine this will be a high priority issue for JB, as breaking Compose performance for all Kotlin/JS Compose libraries seems to me like it would be a Big Deal (TM).

bitspittle commented 1 year ago

OK, closing this again. This is working once more after Kobweb v0.11.4 went out.