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

chore(Easings): Refactor #5108

Open
wants to merge 3 commits into
base: nextgen
Choose a base branch
from

Conversation

sqlerrorthing
Copy link
Contributor

No description provided.

Copy link
Contributor

@ccetl ccetl left a comment

Choose a reason for hiding this comment

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

I fail to see how this refactor is beneficial for readability or speed.

@sqlerrorthing
Copy link
Contributor Author

sqlerrorthing commented Dec 30, 2024

I fail to see how this refactor is beneficial for readability or speed.

in my opinion, this code is more readable than before. speed is not affected & override fun transform(x: Float) is not relevant here at all.

@MukjepScarlet
Copy link
Contributor

(Float) -> Number will create packed types of number, which will cause memory waste

@sqlerrorthing
Copy link
Contributor Author

sqlerrorthing commented Dec 30, 2024

(Float) -> Number will create packed types of number, which will cause memory waste

whydm?
resolved

@MukjepScarlet
Copy link
Contributor

(Float) -> Number will create packed types of number, which will cause memory waste

whydm?

java.lang.Float <-> float
The first one is an Object, which consumes more memory. The same goes for Number. You should take a look of Kotlin bytecode of this file.

@sqlerrorthing
Copy link
Contributor Author

(Float) -> Number will create packed types of number, which will cause memory waste

whydm?

java.lang.Float <-> float The first one is an Object, which consumes more memory. The same goes for Number. You should take a look of Kotlin bytecode of this file.

sry, my fuckup

@MukjepScarlet
Copy link
Contributor

MukjepScarlet commented Dec 30, 2024

If you want a direct double result, create an inline extension function of this class.

inline fun Easing.transform(x: Double) = transform(x.toFloat()).toDouble()

@ccetl
Copy link
Contributor

ccetl commented Dec 30, 2024

I fail to see how this refactor is beneficial for readability or speed.

in my opinion, this code is more readable than before. speed is not affected & override fun transform(x: Float) is not relevant here at all.

The speed is affected, the class is now ~95% slower.

Benchmark                     Mode  Cnt   Score   Error  Units
d.c.lambda.Transform.bench    avgt    8  27,581 ± 0,700  ns/op
d.c.override.Transform.bench  avgt    8  14,146 ± 1,719  ns/op
My setup was, with `EasingX` exchanged for the individual Easing class:
var i = 0f
var function = 0

@Benchmark
fun bench(blackhole: Blackhole) {
    i += 1.3f
    function++
    if (function == 7) {
        function = 0
    }
    blackhole.consume(EasingX.entries[function].transform(i))
}

WARMUP_ITERATIONS = 4
WARMUP_TIME = 5
WARMUP_FORKS = 1

MEASUREMENT_ITERATIONS = 8
MEASUREMENT_TIME = 5
MEASUREMENT_FORKS = 1

Hardware:
AMD Ryzen 7 3700U

@ccetl
Copy link
Contributor

ccetl commented Dec 30, 2024

Actually this class was already refactored to be not like this in #4115 (the class used to be named Curves.kt).
But the comment got lost in one of my commits.

@superblaubeere27
Copy link
Contributor

@ccetl I think the benchmark is flawed. 4 warmup iterations is too few. The JIT compiler didn't even have a chance to compile the code. You probably tested the unoptimized, interpreted version. Those versions should be equivalent, otherwise there is a serious problem in the JIT implementation.

Function pointers (() -> Int) are (AFAIK) implemented using interfaces while abstract classes use vtables.

@MukjepScarlet
Copy link
Contributor

@superblaubeere27 I think the biggest problem is that Kotlin function interfaces will return Object instead of primitives (like float/double in this case). This results in increased execution time and memory usage.

@ccetl
Copy link
Contributor

ccetl commented Dec 31, 2024

@superblaubeere27 I think the biggest problem is that Kotlin function interfaces will return Object instead of primitives (like float/double in this case). This results in increased execution time and memory usage.

You're right, but not only does it return an object at will also take an object that has to be covertet into primitives first before passing it down to the actual logic.

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

Successfully merging this pull request may close these issues.

4 participants