Skip to content

Commit

Permalink
move tabbar lru_caches to instance level
Browse files Browse the repository at this point in the history
Flake8-bugbear correctly pointed out that TabBar instances would not be
reliably cleaned up because the `self` reference would be cached in the
lru_caches on a couple of the methods. And the caches are on the class
so they last for the whole lifetime of the process.

This commit move the caches to be created per instance, instead of on
the class.

Other options:

1. get rid of the caches

From running the benchmark tests (eg `python3 -m pytest --benchmark-columns Min,Max,Median,Rounds -k test_update_tab_titles_benchmark`)
it seems like the caches can still be helpful (even though when they
were introduced in #3122 we didn't have the config cache either)

2. use cachetools to manage our own cache instead of using lru_cache in
   a non-standard way

I don't feel like introducing a new dependency given this change didn't
end up being too offensive.

3. clear the cache whenever a window get closed

That would solve the "not getting deleted issue" but flake8 would still
complain :)

Possibly the cache size could be reduced now that there is going to be
one per window. But the aren't caching large objects anyway.

Flake8-bugbear change: PyCQA/flake8-bugbear#218
Video that pointed out this way of using lru_cache: https://youtu.be/sVjtp6tGo0g
  • Loading branch information
toofar committed Mar 26, 2022
1 parent 87b9dab commit b830c13
Showing 1 changed file with 13 additions and 7 deletions.
20 changes: 13 additions & 7 deletions qutebrowser/mainwindow/tabwidget.py
Expand Up @@ -410,6 +410,15 @@ def __init__(self, win_id, parent=None):
config.instance.changed.connect(self._on_config_changed)
self._set_icon_size()
QTimer.singleShot(0, self.maybe_hide)
self._minimum_tab_size_hint_helper = functools.lru_cache(maxsize=2**9)(
self._minimum_tab_size_hint_helper_uncached
)
debugcachestats.register(name=f'tab width cache (win_id={win_id})')(
self._minimum_tab_size_hint_helper
)
self._minimum_tab_height = functools.lru_cache(maxsize=1)(
self._minimum_tab_height_uncached
)

def __repr__(self):
return utils.get_repr(self, count=self.count())
Expand Down Expand Up @@ -575,11 +584,9 @@ def minimumTabSizeHint(self, index: int, ellipsis: bool = True) -> QSize:
icon_width, ellipsis,
pinned)

@debugcachestats.register(name='tab width cache')
@functools.lru_cache(maxsize=2**9)
def _minimum_tab_size_hint_helper(self, tab_text: str,
icon_width: int,
ellipsis: bool, pinned: bool) -> QSize:
def _minimum_tab_size_hint_helper_uncached(self, tab_text: str,
icon_width: int,
ellipsis: bool, pinned: bool) -> QSize:
"""Helper function to cache tab results.
Config values accessed in here should be added to _on_config_changed to
Expand Down Expand Up @@ -610,8 +617,7 @@ def _text_to_width(text):
width = max(min_width, width)
return QSize(width, height)

@functools.lru_cache(maxsize=1)
def _minimum_tab_height(self):
def _minimum_tab_height_uncached(self):
padding = config.cache['tabs.padding']
return self.fontMetrics().height() + padding.top + padding.bottom

Expand Down

0 comments on commit b830c13

Please sign in to comment.