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

Table: Make UpdateViewport() have constant runtime, not depending on … #284

Merged
merged 3 commits into from
Dec 1, 2022

Conversation

pja237
Copy link

@pja237 pja237 commented Nov 4, 2022

…number of rows in a table.

Related issue: #276

Since there was no feedback on the issue, i tried the following approach to solve the problem of table becoming pretty much useless for a large number of rows.

Instead of rendering the whole table, i limited the rendering to only rows from: m.cursor-m.viewport.Height to: m.cursor+m.viewport.Height. This approach gives the constant runtime (see below) making table act snappy and responsive independent on the number of rows in it.

Results, timed by measuring time to execute a MoveDown(1):

		// DOWN
		case key.Matches(msg, keybindings.DefaultKeyMap.Down):
			t := time.Now()
			m.Log.Printf("Update: Move down\n")
			activeTable.MoveDown(1)
			m.Log.Printf("Update: Move down finished in: %.3f sec\n", time.Since(t).Seconds())
Rows: 36932


SC: 14:34:19.346611 update.go:396: Update: Move down
debug 2022/11/04 14:34:19 rows: 36932 start: 0 end: 51 cursor: 1 height: 50 range: 51
SC: 14:34:19.351719 update.go:398: Update: Move down finished in: 0.005 sec

SC: 14:34:19.507794 update.go:396: Update: Move down
debug 2022/11/04 14:34:19 rows: 36932 start: 0 end: 52 cursor: 2 height: 50 range: 52
SC: 14:34:19.511653 update.go:398: Update: Move down finished in: 0.004 sec

SC: 14:34:19.675428 update.go:396: Update: Move down
debug 2022/11/04 14:34:19 rows: 36932 start: 0 end: 53 cursor: 3 height: 50 range: 53
SC: 14:34:19.679369 update.go:398: Update: Move down finished in: 0.004 sec


Rows: 12

SC: 14:34:31.174864 update.go:396: Update: Move down
debug 2022/11/04 14:34:31 rows: 12 start: 0 end: 12 cursor: 1 height: 50 range: 12
SC: 14:34:31.176839 update.go:398: Update: Move down finished in: 0.002 sec

SC: 14:34:31.344683 update.go:396: Update: Move down
debug 2022/11/04 14:34:31 rows: 12 start: 0 end: 12 cursor: 2 height: 50 range: 12
SC: 14:34:31.345428 update.go:398: Update: Move down finished in: 0.001 sec

SC: 14:34:31.514059 update.go:396: Update: Move down
debug 2022/11/04 14:34:31 rows: 12 start: 0 end: 12 cursor: 3 height: 50 range: 12
SC: 14:34:31.514577 update.go:398: Update: Move down finished in: 0.001 sec

This code works fine in my app , although i'm not sure if it might have any adverse effect in some other table code-paths?
Can't see any for now. If you spot anything, let me know! 😺

@maaslalani
Copy link
Contributor

Thank you @pja237, this is super appreciated. Nice work!

@maaslalani
Copy link
Contributor

One thing I noticed when playing around with this is that the cursor is not working properly when scrolling. The cursor should behave like the first table where it is on the last / first row depending on whether it is scrolling down / up.

Screen.Recording.2022-11-04.at.10.26.08.AM.mov

@pja237
Copy link
Author

pja237 commented Nov 4, 2022

👍 haven't noticed that, thanks!
It's back to the drawing board, maths is hard 😄

@maaslalani
Copy link
Contributor

Thanks! This is a great improvement, really looking forward to see it merged!

@pja237
Copy link
Author

pja237 commented Nov 5, 2022

Hey @maaslalani ,
sorry again for the sloppy original PR, got excited by "how easy it was" 😆
Now, i've tried to cover the corner cases that arise from this approach (bending table to misuse viewport), and it seems better. Please give it a try and let me know what you think.

table/table.go Outdated
Comment on lines 24 to 28
renderedLines
}

type renderedLines struct {
start, end int
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can get rid of this renderedLines struct and replace it as end and start on the model. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Absolutely fine with me, was just a distinction between original table struct and added values, pushed a commit with change

@maaslalani
Copy link
Contributor

Tested this out and it works great! Thank you so much @pja237. Great work!

@pja237
Copy link
Author

pja237 commented Nov 5, 2022

You're very welcome!
👍 for your amazing responsiveness! 🥇

@muesli
Copy link
Contributor

muesli commented Nov 18, 2022

@maaslalani Anything blocking this or can we merge?

@pja237
Copy link
Author

pja237 commented Dec 1, 2022

To keep this alive and let you know, i've been in contact with @maaslalani on discord regarding this pr and we concluded that it's good to go, but of course, we should wait for him on this. Just posting this so it doesn't fizzle out, would love to see this merged

Copy link
Contributor

@maaslalani maaslalani left a comment

Choose a reason for hiding this comment

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

Yep, this one is good to go!

@maaslalani maaslalani merged commit 94252ef into charmbracelet:master Dec 1, 2022
@maaslalani
Copy link
Contributor

Thanks @pja237 for the awesome work ❤️

@pja237 pja237 deleted the table_performance branch December 14, 2022 21:24
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.

3 participants