[PATCH 1/3] Use cache for base repo if possible
Samantha N. Bueno
sbueno+anaconda at redhat.com
Mon Oct 21 13:44:03 UTC 2013
On Wed, Oct 16, 2013 at 03:26:21PM +0200, Vratislav Podzimek wrote:
> The baseRepo property is tricky, because it needs yum_lock to be evaluated.
> However, in many cases, nothing changes in the configuration and we can simply
> return the same value as in the previous call. If there is some change that can
> potentially change the value, the cache needs to be made invalid and refreshed.
>
> This speeds things up and makes the getBaseRepo method cheap enough to be used
> in additional places that need it.
>
> Signed-off-by: Vratislav Podzimek <vpodzime at redhat.com>
> ---
> pyanaconda/constants.py | 1 +
> pyanaconda/packaging/__init__.py | 21 +++++++--
> pyanaconda/packaging/dnfpayload.py | 4 +-
> pyanaconda/packaging/yumpayload.py | 87 +++++++++++++++++++++++++++++++++---
> pyanaconda/ui/gui/spokes/network.py | 4 +-
> pyanaconda/ui/gui/spokes/software.py | 2 +-
> pyanaconda/ui/gui/spokes/source.py | 12 ++---
> pyanaconda/ui/gui/spokes/storage.py | 2 +-
> pyanaconda/ui/tui/spokes/software.py | 2 +-
> pyanaconda/ui/tui/spokes/source.py | 8 ++--
> 10 files changed, 116 insertions(+), 27 deletions(-)
>
> diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py
> index 7af5014..115ef28 100644
> --- a/pyanaconda/constants.py
> +++ b/pyanaconda/constants.py
> @@ -108,6 +108,7 @@ THREAD_GEOLOCATION_REFRESH = "AnaGeolocationRefreshThread"
> THREAD_DATE_TIME = "AnaDateTimeThread"
> THREAD_TIME_INIT = "AnaTimeInitThread"
> THREAD_XKL_WRAPPER_INIT = "AnaXklWrapperInitThread"
> +THREAD_REFRESH_BASE_REPO = "AnaBaseRepoRefreshThread"
>
> # Geolocation constants
>
> diff --git a/pyanaconda/packaging/__init__.py b/pyanaconda/packaging/__init__.py
> index 74adf7f..a5bcb01 100644
> --- a/pyanaconda/packaging/__init__.py
> +++ b/pyanaconda/packaging/__init__.py
> @@ -64,6 +64,8 @@ from pyanaconda.product import productName, productVersion
> import urlgrabber
> urlgrabber.grabber.default_grabber.opts.user_agent = "%s (anaconda)/%s" %(productName, productVersion)
>
> +REPO_NOT_SET = False
> +
> ###
> ### ERROR HANDLING
> ###
> @@ -145,9 +147,22 @@ class Payload(object):
> """ A list of addon repo identifiers. """
> return [r.name for r in self.data.repo.dataList()]
>
> - @property
> - def baseRepo(self):
> - """ The identifier of the current base repo. """
> + def getBaseRepo(self, wait=True, callback=None):
> + """
> + Get the identifier of the current base repo.
> +
> + :param wait: whether to block until the identifier is derived from the
> + configuration (may take a long time) or just try to use
> + the cached value and return REPO_NOT_SET if there is none
> + :type wait: bool
> + :param callback: callback that will be called once the indentifier is
> + derived from the configuration (gets the repo ID as
> + the first argument) if it is not returned instantly
> + :type callback: str -> None
> + :returns: id of the current base repo or None if wait=True is used or
> + REPO_NOT_SET if wait=False is used and the value is not cached
> +
> + """
> return None
>
> @property
> diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py
> index d0bda57..3e5e01b 100644
> --- a/pyanaconda/packaging/dnfpayload.py
> +++ b/pyanaconda/packaging/dnfpayload.py
> @@ -217,8 +217,8 @@ class DNFPayload(packaging.PackagePayload):
> # addon repos via kickstart
> return [r.name for r in self.data.repo.dataList()]
>
> - @property
> - def baseRepo(self):
> + def getBaseRepo(self, wait=True, callback=None):
> + # is any locking needed here as in the yumpayload?
> repo_names = [constants.BASE_REPO_NAME] + DEFAULT_REPOS
> for repo in self._base.repos.iter_enabled():
> if repo.id in repo_names:
> diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py
> index 0eb5cf1..4225649 100644
> --- a/pyanaconda/packaging/yumpayload.py
> +++ b/pyanaconda/packaging/yumpayload.py
> @@ -61,7 +61,7 @@ except ImportError:
> log.error("import of yum failed")
> yum = None
>
> -from pyanaconda.constants import BASE_REPO_NAME, DRACUT_ISODIR, INSTALL_TREE, ISO_DIR, MOUNT_DIR, ROOT_PATH
> +from pyanaconda.constants import BASE_REPO_NAME, DRACUT_ISODIR, INSTALL_TREE, ISO_DIR, MOUNT_DIR, ROOT_PATH, THREAD_REFRESH_BASE_REPO
> from pyanaconda.flags import flags
>
> from pyanaconda import iutil
> @@ -77,7 +77,9 @@ from pyanaconda.errors import ERROR_RAISE, errorHandler
> from pyanaconda.packaging import DependencyError, MetadataError, NoNetworkError, NoSuchGroup, \
> NoSuchPackage, PackagePayload, PayloadError, PayloadInstallError, \
> PayloadSetupError
> +from pyanaconda.packaging import REPO_NOT_SET
> from pyanaconda.progress import progressQ
> +from pyanaconda.threads import threadMgr, AnacondaThread
>
> from pyanaconda.localization import langcode_matches_locale
>
> @@ -135,6 +137,13 @@ class YumPayload(PackagePayload):
> self._requiredPackages = []
> self._requiredGroups = []
>
> + # base repo caching
> + self._cached_base_repo = None
> + self._br_cache_valid = False
> + self._br_callbacks = []
> + self._br_cache_valid_lock = threading.Lock()
> + self._br_refresh_lock = threading.Lock()
Two variables assigned the same value, seems like it should just be one.
> +
> self.reset()
>
> def reset(self, root=None):
> @@ -372,18 +381,55 @@ reposdir=%s
>
> return _repos
>
> - @property
> - def baseRepo(self):
> + def getBaseRepo(self, wait=True, callback=None):
> + # prevent potential thread calling callbacks from finishing if we want
> + # to add some more
> + with self._br_refresh_lock:
> + # test and set atomically
> + with self._br_cache_valid_lock:
> + if self._br_cache_valid:
> + return self._cached_base_repo
I'm not sure I understand why you're effectively calling
threading.lock() twice in a row here. It seems like the second call is
unnecessary.
Aside from the above comments, this one looks fine.
Samantha
More information about the anaconda-patches
mailing list