[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