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

Speed up SetContent by checking length of combining characters before reflect.DeepEqual #744

Merged
merged 8 commits into from
Oct 7, 2024

Conversation

kivattt
Copy link
Contributor

@kivattt kivattt commented Jul 26, 2024

Adding these length checks before the reflect.DeepEqual speeds up SetContent when not drawing Unicode combining characters

With xset r rate 300 255 (key repeating 255 times per second), I recorded myself scrolling through 2000 files in my file manager project where I'm using tview with and without (left) this change:

tcell_change.mp4

(6x sped up)

image

I believe the performance difference is a lot less dramatic for most projects, since in this old version of my file manager the screen was always unnecessarily being cleared with Box.DrawForSubclass() in tview

Co-authored-by: Garrett D'Amore <garrett@damore.org>
@kivattt
Copy link
Contributor Author

kivattt commented Oct 7, 2024

With the way I'm testing it, I see no difference with your change, but it's certainly easier to read and I'm not that familiar with the problem

@kivattt
Copy link
Contributor Author

kivattt commented Oct 7, 2024

Nevermind, I can see a stuck character on the screen when I use your change, I'll send a video demonstrating it

@gdamore
Copy link
Owner

gdamore commented Oct 7, 2024 via email

@kivattt
Copy link
Contributor Author

kivattt commented Oct 7, 2024

On second glance, I think you just had the ! in the wrong place, this new commit fixes the stuck character issue I noticed

@kivattt
Copy link
Contributor Author

kivattt commented Oct 7, 2024

Nevermind, my commit changes the behaviour, so I think your change is the more correct one and the "stuck" character I'm seeing is not an issue with tcell

@kivattt
Copy link
Contributor Author

kivattt commented Oct 7, 2024

I use this to bruteforce check if something changed

package main

import (
        "fmt"
        "math/rand"
        "os"
        "reflect"
        "strconv"
)

func oldOne(c cell, mainc rune, combc []rune) bool {
        return (c.width > 0) && (mainc != c.currMain || !reflect.DeepEqual(combc, c.currComb))
}

func newOne(c cell, mainc rune, combc []rune) bool {
        return (c.width > 0) && (mainc != c.currMain || len(combc) != len(c.currComb) || (len(combc) > 0 && !reflect.DeepEqual(combc, c.currComb)))
}

func randRune() rune {
        return rune(rand.Intn(10)) // Low enough so 2 random runes can match
}

func randRuneSlice() []rune {
        var ret []rune
        upper := rand.Intn(5)
        for i := 1; i < upper; i++ {
                ret = append(ret, randRune())
        }
        return ret
}

type cell struct {
        currMain rune
        currComb []rune
        width    int
}

func main() {
        for i := 0; i < 10000000; i++ {
                c := cell{
                        currMain: randRune(),
                        currComb: randRuneSlice(),
                        width:    rand.Intn(5),
                }
                mainc := randRune()
                combc := randRuneSlice()

                if oldOne(c, mainc, combc) != newOne(c, mainc, combc) {
                        fmt.Println("oldOne and newOne differ!")
                        fmt.Println("c.currMain:", c.currMain)
                        fmt.Println("c.currComb:", c.currComb)
                        fmt.Println("c.width:", c.width)
                        fmt.Println("mainc: " + strconv.Itoa(int(mainc)))
                        fmt.Println("combc:", combc)
                        os.Exit(1)
                }
        }

        fmt.Println("oldOne and newOne match!")
}

@gdamore gdamore merged commit c0abe11 into gdamore:main Oct 7, 2024
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.

None yet

2 participants