[yocto] [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)

[Paul Eggleton]

Hi Robert

I like the performance improvements, thanks. A few comments below though.

On Wednesday, 18 April 2018 11:04:33 PM NZST Robert Yang wrote:
> The utils.setup_django() costs a lot of time, but both update.py and
> update_layer.py calls it, so move layer validation from update_layer.py to
> update.py to avoid calling update_layer.py when possible can save a lot of
> time.
> 
> Now we don't have to call update_layer.py in the following cases:
> * The branch doesn't exist
> * The layer is already update to date on specified branch (when no
>   reload)
> * The layer dir or conf/layer.layer doesn't exist
> 
> We can save up to 98% time in my testing:
> 
> $ update.py -b master --nofetch [--fullreload]
> 
>                    Before    Now       Reduced
> No update:         276s      3.6s      98%
> Partial update:    312s      87s       72%
> Full repload:      1016s     980s      3%
> 
> Note:
> * All of the testing are based on --nofetch
> 
> * "No update" means all layers on the branch is up-to-date, for
>   example, when we run it twice, there is no update in the second run, so we
>   only need about 3s now, which is the most common case when we use cron to run
>   it per half an hour.
> 
> * "Partly update" means part of the layers have been updated.
> 
> * "Fullreload" means all of the layers have been updated.
> 
> Signed-off-by: Robert Yang <liezhi.yang at windriver.com>
> ---
>  layerindex/update.py       | 79 +++++++++++++++++++++++++++++++++++++++++++++-
>  layerindex/update_layer.py | 39 +++--------------------
>  2 files changed, 82 insertions(+), 36 deletions(-)
> 
> diff --git a/layerindex/update.py b/layerindex/update.py
> index 44a7b90..2bb2df7 100755
> --- a/layerindex/update.py
> +++ b/layerindex/update.py
> @@ -140,6 +140,15 @@ def fetch_repo(vcs_url, repodir, urldir, fetchdir, layer_name):
>          logger.error("Fetch of layer %s failed: %s" % (layer_name, e.output))
>          return (vcs_url, e.output)
>  
> +def print_subdir_error(newbranch, layername, vcs_subdir, branchdesc):
> +    # This will error out if the directory is completely invalid or had never existed at this point
> +    # If it previously existed but has since been deleted, you will get the revision where it was
> +    # deleted - so we need to handle that case separately later
> +    if newbranch:
> +        logger.info("Skipping update of layer %s for branch %s - subdirectory %s does not exist on this branch" % (layername, branchdesc, vcs_subdir))
> +    elif vcs_subdir:
> +        logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layername, branchdesc))
> +
>  def main():
>      if LooseVersion(git.__version__) < '0.3.1':
>          logger.error("Version of GitPython is too old, please install GitPython (python-git) 0.3.1 or later in order to use this script")
> @@ -195,7 +204,7 @@ def main():
>  
>      utils.setup_django()
>      import settings
> -    from layerindex.models import Branch, LayerItem, Update, LayerUpdate
> +    from layerindex.models import Branch, LayerItem, Update, LayerUpdate, LayerBranch
>  
>      logger.setLevel(options.loglevel)
>  
> @@ -337,6 +346,74 @@ def main():
>                          collections.add((layerbranch.collection, layerbranch.version))
>  
>                  for layer in layerquery:
> +                    layerbranch = layer.get_layerbranch(branch)
> +                    branchname = branch
> +                    branchdesc = branch
> +                    newbranch = False
> +                    branchobj = utils.get_branch(branch)
> +                    if layerbranch:
> +                        if layerbranch.actual_branch:
> +                            branchname = layerbranch.actual_branch
> +                            branchdesc = "%s (%s)" % (branch, branchname)
> +                    else:
> +                        # LayerBranch doesn't exist for this branch, create it
> +                        newbranch = True
> +                        layerbranch = LayerBranch()
> +                        layerbranch.layer = layer
> +                        layerbranch.branch = branchobj
> +                        layerbranch_source = layer.get_layerbranch(branchobj)
> +                        if not layerbranch_source:
> +                            layerbranch_source = layer.get_layerbranch(None)
> +                        if layerbranch_source:
> +                            layerbranch.vcs_subdir = layerbranch_source.vcs_subdir
> +

It seems that you are missing a call somewhere to layerbranch.save() later
on, guarded with "if newbranch:". This was easy to miss though when copying
over from update_layer.py seeing as we were relying upon saving it later on
there (and that is still needed).

> +                    if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload:
> +                        logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc))
> +                        collections.add((layerbranch.collection, layerbranch.version))
> +                        continue
> +
> +                    if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:
> +                        # Check out appropriate branch
> +                        if not options.nocheckout:
> +                            utils.checkout_layer_branch(layerbranch, repodir, logger=logger)

Isn't "if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:"
superfluous here? We're skipping to the next layer if that wasn't true via
"continue" above, so it should always evaluate to true.

Additionally this results in the checkout being done twice - once here and
a second time in the update_layer.py script. I guess it's not taking much time
the second time though so we could probably ignore it.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre