Skip to content
This repository has been archived by the owner on Aug 6, 2023. It is now read-only.

potential performance issue in List #9

Closed
cardoe opened this issue Sep 2, 2017 · 3 comments
Closed

potential performance issue in List #9

cardoe opened this issue Sep 2, 2017 · 3 comments
Assignees
Milestone

Comments

@cardoe
Copy link

cardoe commented Sep 2, 2017

So I'm following your examples and I notice in your main loop you create a new Group with new List's for each iteration. Looking at the code behind this at https://github.com/fdehau/tui-rs/blob/master/src/widgets/list.rs#L14 shows that List has a Vec inside of it. While all the items are references you'll still be allocating a Vec for the entire thing. Looking at how its then used is to iterate over the items so you could probably come up with a custom iterator type that could take in. Looking further at the List example, you collect() to a Vec and then pass the slice in since List.items() takes a slice. It then turns around an collect()'s to a Vec again so you're taking the allocation performance hit twice.

Another idea would be to take the style independently of the data, both as IntoIterator Then you could use Iterator.zip() to combine the two. You could alternatively create a type like

pub enum Item<'s> {
    Data(::std::fmt::Display), // allows any type that can be displayed to be used as input (so `String` and `&str` still work)
    StyledData(::std::fmt::Display, &'s ::tui::style::Style),
}

Then List.items() could have a signature like:

pub fn items<L: IntoIterator<Item=Item>(&'a mut self, items: L) -> &mut List<'a>

and you could just store the iterator inside of List instead collecting to a Vec. Then when draw() ran it would just work with that iterator.

@cardoe
Copy link
Author

cardoe commented Sep 2, 2017

I should mention that Iterator.zip() doesn't work if the style iterator has less items than the data iterator because zip() stops when any of the iterators run out of data. So people would be confused why their data was being truncated.

@fdehau
Copy link
Owner

fdehau commented Sep 3, 2017

Hi, there are several valid points here. First of all, there are certainly a lot of places in the code where there is room for improvements regarding both the API exposed to the caller and the performance.

I took the liberty to implement your ideas in a dedicated branch #11. The major change required for this to work is to update the Widget signature. Indeed, until now the draw call was done using a simple & to the widget. Consuming the iterator inside the draw call would then require &mut if I'm not mistaken. This change the behavior of the widget that should now be seen as a "consumable" object. However, this does not appear to me as problem since it was one of the goals of the library anyway.

I would appreciate your feedback on this if you have some time :)

@fdehau fdehau self-assigned this Sep 3, 2017
@fdehau fdehau added this to the 0.1.4 milestone Sep 3, 2017
@fdehau fdehau closed this as completed Sep 10, 2017
@cardoe
Copy link
Author

cardoe commented Sep 11, 2017

Looks good. Sorry for the delay in looking at it. There were some other changes to the API so that delayed me a bit from trying it out. I like it.

oyvindberg added a commit to oyvindberg/tui-rs that referenced this issue Dec 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants