Patchwork [3,of,3] lfs: prefetch lfs blobs during revert

login
register
mail settings
Submitter Matt Harbison
Date Feb. 5, 2018, 2:08 p.m.
Message ID <op.zdyuv8dz9lwrgf@envy>
Download mbox | patch
Permalink /patch/27312/
State New
Headers show

Comments

Matt Harbison - Feb. 5, 2018, 2:08 p.m.
On Mon, 05 Feb 2018 08:51:47 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sun, 04 Feb 2018 01:18:25 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1517722408 18000
>> #      Sun Feb 04 00:33:28 2018 -0500
>> # Node ID 702a3c1d6335cd8f7cf7916029e10e22691e3091
>> # Parent  e779b0c34d6989e45cabbf8da8cdfeb060134dbd
>> lfs: prefetch lfs blobs during revert
>>
>> The revert command oddly prints out what it will do before requesting  
>> the files
>> to be prefetched.  But the 'need to transfer' line indicates the blobs  
>> are being
>> grouped.
>>
>> diff --git a/hgext/lfs/__init__.py b/hgext/lfs/__init__.py
>> --- a/hgext/lfs/__init__.py
>> +++ b/hgext/lfs/__init__.py
>> @@ -333,6 +333,7 @@
>>      wrapfunction(hg, 'postshare', wrapper.hgpostshare)
>>
>>      wrapfunction(merge, 'applyupdates', wrapper.mergemodapplyupdates)
>> +    wrapfunction(cmdutil, '_revertprefetch', wrapper._revertprefetch)
>>
>>      # Make bundle choose changegroup3 instead of changegroup2. This  
>> affects
>>      # "hg bundle" command. Note: it does not cover all bundle formats  
>> like
>> diff --git a/hgext/lfs/wrapper.py b/hgext/lfs/wrapper.py
>> --- a/hgext/lfs/wrapper.py
>> +++ b/hgext/lfs/wrapper.py
>> @@ -277,6 +277,23 @@
>>
>>      return orig(repo, actions, wctx, mctx, overwrite, labels)
>>
>> +def _revertprefetch(orig, repo, ctx, *files):
>> +    """Prefetch the indicated files before the revert operation is  
>> performed."""
>> +    orig(repo, ctx, *files)
>> +
>> +    pointers = []
>> +    localstore = repo.svfs.lfslocalblobstore
>> +
>> +    for group in files:
>> +        for f in group:
>> +            p = pointerfromctx(ctx, f)
>> +            if p and not localstore.has(p.oid()):
>> +                p.filename = f
>> +                pointers.append(p)
>> +
>> +    if pointers:
>> +        repo.svfs.lfsremoteblobstore.readbatch(pointers, localstore)
>
> This looks good to me.
>
> It might be better to provide a single hook point for prefetching,  
> because
> it isn't a trivial task to parse merge actions into a list of files.

Yeah, the next patch does that.  If you have ideas about the deprecation  
issues I mention, I can fold these (or keep them separate if you want) and  
resend tonight.

# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1517771668 18000
#      Sun Feb 04 14:14:28 2018 -0500
# Node ID 397600b45d0386ef1cdc261c0d2c1cf9520e36e9
# Parent  702a3c1d6335cd8f7cf7916029e10e22691e3091
cmdutil: create a generic hook point for prefetching stored files

This will be used by LFS to fetch required files in a group for multiple
commands, prior to being accessed.  That avoids the one-at-a-time fetch  
when the
filelog wrapper goes to access it, and it is missing locally (which costs  
two
round trips to the server.)

There are no core users of the revert prefetch hook, and never have been  
since
it was introduced in 45e02cfad4bd.  I have no idea what the proper way is  
to
deprecate _revertprefetch(), since I think we try to avoid the deprec-warn  
lines
in tests.  Additionally since it's an empty function, I wouldn't be  
surprised if
extensions didn't call this original method at all, and miss the warning
completely.

One last item is that the store lock isn't being held when this is  
called.  I'm
not at all familiar with remotefilelog or its locking requirements, so  
this may
not be a big deal.  Currently, LFS doesn't hold a lock when downloading  
files.
Even though largefiles doesn't either, I'm starting to think it should, and
maybe the .hg/store/lock isn't good enough to cover the globally shared  
cache.

                     tobackup=None):
Yuya Nishihara - Feb. 5, 2018, 2:29 p.m.
On Mon, 05 Feb 2018 09:08:22 -0500, Matt Harbison wrote:
> On Mon, 05 Feb 2018 08:51:47 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> > It might be better to provide a single hook point for prefetching,  
> > because
> > it isn't a trivial task to parse merge actions into a list of files.
> 
> Yeah, the next patch does that.  If you have ideas about the deprecation  
> issues I mention, I can fold these (or keep them separate if you want) and  
> resend tonight.

Maybe we can check if _revertprefetch is replaced?

def _revertprefetchstub():
    pass
_revertprefetch = _revertprefetchstub

...

    if _revertprefetch is not _revertprefetchstub:
        deprecwarn()
   _revertprefetch(...)

Patch

diff --git a/hgext/lfs/__init__.py b/hgext/lfs/__init__.py
--- a/hgext/lfs/__init__.py
+++ b/hgext/lfs/__init__.py
@@ -333,7 +333,7 @@ 
      wrapfunction(hg, 'postshare', wrapper.hgpostshare)

      wrapfunction(merge, 'applyupdates', wrapper.mergemodapplyupdates)
-    wrapfunction(cmdutil, '_revertprefetch', wrapper._revertprefetch)
+    wrapfunction(cmdutil, '_prefetchfiles', wrapper._prefetchfiles)

      # Make bundle choose changegroup3 instead of changegroup2. This  
affects
      # "hg bundle" command. Note: it does not cover all bundle formats like
diff --git a/hgext/lfs/wrapper.py b/hgext/lfs/wrapper.py
--- a/hgext/lfs/wrapper.py
+++ b/hgext/lfs/wrapper.py
@@ -277,19 +277,18 @@ 

      return orig(repo, actions, wctx, mctx, overwrite, labels)

-def _revertprefetch(orig, repo, ctx, *files):
-    """Prefetch the indicated files before the revert operation is  
performed."""
-    orig(repo, ctx, *files)
+def _prefetchfiles(orig, repo, ctx, files):
+    """Prefetch the indicated files before they are accessed by a  
command."""
+    orig(repo, ctx, files)

      pointers = []
      localstore = repo.svfs.lfslocalblobstore

-    for group in files:
-        for f in group:
-            p = pointerfromctx(ctx, f)
-            if p and not localstore.has(p.oid()):
-                p.filename = f
-                pointers.append(p)
+    for f in files:
+        p = pointerfromctx(ctx, f)
+        if p and not localstore.has(p.oid()):
+            p.filename = f
+            pointers.append(p)

      if pointers:
          repo.svfs.lfsremoteblobstore.readbatch(pointers, localstore)
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2877,6 +2877,11 @@ 

  def _revertprefetch(repo, ctx, *files):
      """Let extension changing the storage layer prefetch content"""
+    _prefetchfiles(repo, ctx, [f for sublist in files for f in sublist])
+
+def _prefetchfiles(repo, ctx, files):
+    """Let extensions changing the storage layer prefetch content for any  
non
+    merge based command."""

  def _performrevert(repo, parents, ctx, actions, interactive=False,