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

List optimization - handle SetItemHeight with default height as a special case #4841

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

dweymouth
Copy link
Contributor

Description:

Handles the case of SetItemHeight(h) where h is equal to the default item height specially. Instead of storing it in the itemHeights map, delete the entry in the map if it exists for the given item, since list operations are more efficient with fewer entries in the itemHeights map.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@coveralls
Copy link

Coverage Status

coverage: 65.061% (-0.009%) from 65.07%
when pulling 8216c18 on dweymouth:list-itemheight-optimize
into 54154f9 on fyne-io:develop.

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Thanks. This seems very sensible but I think we might want a benchmark to back this up?

@dweymouth
Copy link
Contributor Author

What would we want to benchmark in this case? Setup by setting a custom height for every row and then unsetting it, then benchmarking how long contentMinSize takes?

@Jacalz
Copy link
Member

Jacalz commented May 18, 2024

No offence, but I just find it hard to grasp or understand why this is an obvious performance improvement and without any benchmark to back it up, I have a hard time motivating an approval.

@dweymouth
Copy link
Contributor Author

It's an improvement since list calculations are more expensive with a lot of custom item heights (especially now that #4835 is in making size O(n) in custom heights rather than O(n) in total items). So I guess the appropriate benchmark will be to set and unset a bunch of item heights in setup, and then benchmark that contentMinSize is faster with these changes. I'll look into adding one

@Jacalz
Copy link
Member

Jacalz commented May 30, 2024

Just a friendly reminder to add a benchmark. Would be nice to have this in 2.5 :)

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