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

fix(Layout): layout cache does not grow without bounds #487

Merged
merged 4 commits into from
Sep 14, 2023

Conversation

marianomarciello
Copy link
Contributor

This pull request avoids the growth of Layout cache without any bounds.
The Layout cache is now using an LruCache (doc).
The current default cache size is set to 16. It seems reasonable for me to keep this value low.
A user can set the cache size during the Layout instantiation.

fix #402

@codecov
Copy link

codecov bot commented Sep 10, 2023

Codecov Report

Merging #487 (064cf8e) into main (d4976d4) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
+ Coverage   89.89%   89.93%   +0.04%     
==========================================
  Files          41       41              
  Lines       11286    11334      +48     
==========================================
+ Hits        10145    10193      +48     
  Misses       1141     1141              
Files Changed Coverage Δ
src/layout.rs 98.37% <100.00%> (+0.07%) ⬆️

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

LGTM - happy to merge this with or without the suggestions.

Edit: I just noticed the lint check - can you please run cargo make format and bump this.

src/layout.rs Outdated Show resolved Hide resolved
src/layout.rs Outdated Show resolved Hide resolved
Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Code is great.
Docs look good - thanks for adding these.

A couple of minor changes to make the docs render the DEFAULT_CACHE_SIZE. You might also need to rewrap the doc text if these changes are made. Check the VSCode Rewrap extension or configure your IDE / editor to wrap at 100 chars.

src/layout.rs Outdated Show resolved Hide resolved
src/layout.rs Outdated Show resolved Hide resolved
src/layout.rs Outdated Show resolved Hide resolved
Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribution.

@joshka joshka force-pushed the layout-cache-fix-402 branch from 70eaff6 to 064cf8e Compare September 14, 2023 22:13
@joshka joshka merged commit 638d596 into ratatui:main Sep 14, 2023
33 checks passed
@joshka
Copy link
Member

joshka commented Sep 14, 2023

I rebased on main, squashed and updated the commit message so it would look nice in the Changelog and merged this.

fix(Layout): use LruCache for layout cache (#487)

The layout cache now uses a LruCache with default size set to 16 entries.
Previously the cache was backed by a HashMap, and was able to grow
without bounds as a new entry was added for every new combination of
layout parameters.

- Added a new method (`layout::init_cache(usize)`) that allows the cache
size to be changed if necessary. This will only have an effect if it is called
prior to any calls to `layout::split()` as the cache is wrapped in a `OnceLock`

Thanks for submitting this PR. We appreciate your contribution.

@marianomarciello marianomarciello deleted the layout-cache-fix-402 branch September 17, 2023 13:53
@a-kenji a-kenji mentioned this pull request Oct 25, 2023
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.

Ensure that layout cache does not grow without bounds
4 participants