Patchwork [remotefilelog-ext] Reuse ssh connection across miss fetches

login
register
mail settings
Submitter Durham Goode
Date Dec. 10, 2015, 12:32 a.m.
Message ID <7fde3699448cc3b3b6f3.1449707570@dev8486.prn1.facebook.com>
Download mbox | patch
Permalink /patch/11956/
State Changes Requested
Delegated to: Augie Fackler
Headers show

Comments

Durham Goode - Dec. 10, 2015, 12:32 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1449707498 28800
#      Wed Dec 09 16:31:38 2015 -0800
# Node ID 7fde3699448cc3b3b6f3b1b5fe64b856d974fcc3
# Parent  ddb335eb76d96758d3171ada6564d46390166f3c
Reuse ssh connection across miss fetches

Previously we recreated the ssh connection for each prefetch. In the case where
we were fetching files one by one (like when we forgot to batch request files),
it results in a 1+ second overhead for each fetch.

This changes makes us hold onto the ssh connection and simply issue new requests
along the same connection.
Durham Goode - Dec. 11, 2015, 12:50 a.m.
On 12/9/15 4:32 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1449707498 28800
> #      Wed Dec 09 16:31:38 2015 -0800
> # Node ID 7fde3699448cc3b3b6f3b1b5fe64b856d974fcc3
> # Parent  ddb335eb76d96758d3171ada6564d46390166f3c
> Reuse ssh connection across miss fetches
>
> Previously we recreated the ssh connection for each prefetch. In the case where
> we were fetching files one by one (like when we forgot to batch request files),
> it results in a 1+ second overhead for each fetch.
>
> This changes makes us hold onto the ssh connection and simply issue new requests
> along the same connection.
>
cc Augie since he has worked in this area recently
Augie Fackler - Dec. 11, 2015, 3:21 p.m.
On Wed, Dec 09, 2015 at 04:32:50PM -0800, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1449707498 28800
> #      Wed Dec 09 16:31:38 2015 -0800
> # Node ID 7fde3699448cc3b3b6f3b1b5fe64b856d974fcc3
> # Parent  ddb335eb76d96758d3171ada6564d46390166f3c
> Reuse ssh connection across miss fetches

Overall looks fine, but I do have a comment below about code
structuring and some isinstance checks look fishy to me...

>
> Previously we recreated the ssh connection for each prefetch. In the case where
> we were fetching files one by one (like when we forgot to batch request files),
> it results in a 1+ second overhead for each fetch.
>
> This changes makes us hold onto the ssh connection and simply issue new requests
> along the same connection.
>
> diff --git a/remotefilelog/fileserverclient.py b/remotefilelog/fileserverclient.py
> --- a/remotefilelog/fileserverclient.py
> +++ b/remotefilelog/fileserverclient.py
> @@ -133,7 +133,6 @@ def _getfilesbatch(
>
>  def _getfiles(
>      remote, receivemissing, fallbackpath, progresstick, missed, idmap):
> -    remote._callstream("getfiles")
>      i = 0
>      while i < len(missed):
>          # issue a batch of requests
> @@ -152,8 +151,6 @@ def _getfiles(
>          for j in range(start, end):
>              receivemissing(remote.pipei, missed[j])
>              progresstick()
> -    remote.cleanup()
> -    remote = None
>
>  class fileserverclient(object):
>      """A client for requesting files from the remote file server.
> @@ -169,6 +166,7 @@ class fileserverclient(object):
>
>          self.localcache = localcache(repo)
>          self.remotecache = cacheconnection()
> +        self.remoteserver = None
>
>      def request(self, fileids):
>          """Takes a list of filename/node pairs and fetches them from the
> @@ -241,14 +239,26 @@ class fileserverclient(object):
>                  verbose = self.ui.verbose
>                  self.ui.verbose = False
>                  try:
> -                    if not fallbackpath:
> -                        raise util.Abort("no remotefilelog server configured - "
> -                            "is your .hg/hgrc trusted?")
> -                    remote = hg.peer(self.ui, {}, fallbackpath)
> +                    newconnection = False
> +                    if not self.remoteserver:
> +                        if not fallbackpath:
> +                            raise util.Abort("no remotefilelog server "
> +                                "configured - is your .hg/hgrc trusted?")
> +                        self.remoteserver = hg.peer(self.ui, {}, fallbackpath)
> +                        newconnection = True
> +                    elif (isinstance(self.remoteserver, sshpeer.sshpeer) and
> +                             self.remoteserver.subprocess.poll() != None):
> +                        # The ssh connection died, so recreate it.
> +                        self.remoteserver = hg.peer(self.ui, {}, fallbackpath)
> +                        newconnection = True

I would be inclined to tease this out into some sort of
self._connect() method or something just to try and keep things clear.

What's up with the sshpeer.sshpeer isinstance checks? Nothing else is
assigning to this attribute right now, so it shouldn't matter...

> +
> +                    remote = self.remoteserver
>                      # TODO: deduplicate this with the constant in shallowrepo
>                      if remote.capable("remotefilelog"):
>                          if not isinstance(remote, sshpeer.sshpeer):
>                              raise util.Abort('remotefilelog requires ssh servers')
> +                        if newconnection:
> +                            remote._callstream("getfiles")
>                          _getfiles(remote, self.receivemissing, fallbackpath,
>                                    progresstick, missed, idmap)
>                      elif remote.capable("getfile"):
> @@ -331,6 +341,10 @@ class fileserverclient(object):
>          if self.remotecache.connected:
>              self.remotecache.close()
>
> +        if self.remoteserver and isinstance(self.remoteserver, sshpeer.sshpeer):
> +            self.remoteserver.cleanup()
> +            self.remoteserver = None
> +
>      def prefetch(self, fileids, force=False):
>          """downloads the given file versions to the cache
>          """
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Durham Goode - Dec. 11, 2015, 5:22 p.m.
On 12/11/15 7:21 AM, Augie Fackler wrote:
> On Wed, Dec 09, 2015 at 04:32:50PM -0800, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1449707498 28800
>> #      Wed Dec 09 16:31:38 2015 -0800
>> # Node ID 7fde3699448cc3b3b6f3b1b5fe64b856d974fcc3
>> # Parent  ddb335eb76d96758d3171ada6564d46390166f3c
>> Reuse ssh connection across miss fetches
> Overall looks fine, but I do have a comment below about code
> structuring and some isinstance checks look fishy to me...
>
>> Previously we recreated the ssh connection for each prefetch. In the case where
>> we were fetching files one by one (like when we forgot to batch request files),
>> it results in a 1+ second overhead for each fetch.
>>
>> This changes makes us hold onto the ssh connection and simply issue new requests
>> along the same connection.
>>
>> diff --git a/remotefilelog/fileserverclient.py b/remotefilelog/fileserverclient.py
>> --- a/remotefilelog/fileserverclient.py
>> +++ b/remotefilelog/fileserverclient.py
>> @@ -133,7 +133,6 @@ def _getfilesbatch(
>>
>>   def _getfiles(
>>       remote, receivemissing, fallbackpath, progresstick, missed, idmap):
>> -    remote._callstream("getfiles")
>>       i = 0
>>       while i < len(missed):
>>           # issue a batch of requests
>> @@ -152,8 +151,6 @@ def _getfiles(
>>           for j in range(start, end):
>>               receivemissing(remote.pipei, missed[j])
>>               progresstick()
>> -    remote.cleanup()
>> -    remote = None
>>
>>   class fileserverclient(object):
>>       """A client for requesting files from the remote file server.
>> @@ -169,6 +166,7 @@ class fileserverclient(object):
>>
>>           self.localcache = localcache(repo)
>>           self.remotecache = cacheconnection()
>> +        self.remoteserver = None
>>
>>       def request(self, fileids):
>>           """Takes a list of filename/node pairs and fetches them from the
>> @@ -241,14 +239,26 @@ class fileserverclient(object):
>>                   verbose = self.ui.verbose
>>                   self.ui.verbose = False
>>                   try:
>> -                    if not fallbackpath:
>> -                        raise util.Abort("no remotefilelog server configured - "
>> -                            "is your .hg/hgrc trusted?")
>> -                    remote = hg.peer(self.ui, {}, fallbackpath)
>> +                    newconnection = False
>> +                    if not self.remoteserver:
>> +                        if not fallbackpath:
>> +                            raise util.Abort("no remotefilelog server "
>> +                                "configured - is your .hg/hgrc trusted?")
>> +                        self.remoteserver = hg.peer(self.ui, {}, fallbackpath)
>> +                        newconnection = True
>> +                    elif (isinstance(self.remoteserver, sshpeer.sshpeer) and
>> +                             self.remoteserver.subprocess.poll() != None):
>> +                        # The ssh connection died, so recreate it.
>> +                        self.remoteserver = hg.peer(self.ui, {}, fallbackpath)
>> +                        newconnection = True
> I would be inclined to tease this out into some sort of
> self._connect() method or something just to try and keep things clear.
I'll give that a try.
>
> What's up with the sshpeer.sshpeer isinstance checks? Nothing else is
> assigning to this attribute right now, so it shouldn't matter...
The http peer is also stored in self.remoteserver, so the http tests 
fails without that check.
>
>> +
>> +                    remote = self.remoteserver
>>                       # TODO: deduplicate this with the constant in shallowrepo
>>                       if remote.capable("remotefilelog"):
>>                           if not isinstance(remote, sshpeer.sshpeer):
>>                               raise util.Abort('remotefilelog requires ssh servers')
>> +                        if newconnection:
>> +                            remote._callstream("getfiles")
>>                           _getfiles(remote, self.receivemissing, fallbackpath,
>>                                     progresstick, missed, idmap)
>>                       elif remote.capable("getfile"):
>> @@ -331,6 +341,10 @@ class fileserverclient(object):
>>           if self.remotecache.connected:
>>               self.remotecache.close()
>>
>> +        if self.remoteserver and isinstance(self.remoteserver, sshpeer.sshpeer):
>> +            self.remoteserver.cleanup()
>> +            self.remoteserver = None
>> +
>>       def prefetch(self, fileids, force=False):
>>           """downloads the given file versions to the cache
>>           """
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> https://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Dec. 11, 2015, 5:46 p.m.
On Fri, Dec 11, 2015 at 12:22 PM, Durham Goode <durham@fb.com> wrote:
>> What's up with the sshpeer.sshpeer isinstance checks? Nothing else is
>> assigning to this attribute right now, so it shouldn't matter...
>
> The http peer is also stored in self.remoteserver, so the http tests fails
> without that check.


Ah. That's a little groty. I think where I'd go with this is look for
self.remoteserver.cleanup() and call that if it exists in the
teardown.

I'm less sure what to do about the bit where we ensure the connection
is alive - perhaps we should expand the peer API to include something
useful on that front instead of just having random isinstance checks
sprinkled in various bits of code. That's more of an idea for later, I
guess, but it seems like something we should clean up before too long.
Durham Goode - Dec. 11, 2015, 7:23 p.m.
On 12/11/15 9:46 AM, Augie Fackler wrote:
> On Fri, Dec 11, 2015 at 12:22 PM, Durham Goode <durham@fb.com> wrote:
>>> What's up with the sshpeer.sshpeer isinstance checks? Nothing else is
>>> assigning to this attribute right now, so it shouldn't matter...
>> The http peer is also stored in self.remoteserver, so the http tests fails
>> without that check.
>
> Ah. That's a little groty. I think where I'd go with this is look for
> self.remoteserver.cleanup() and call that if it exists in the
> teardown.
>
> I'm less sure what to do about the bit where we ensure the connection
> is alive - perhaps we should expand the peer API to include something
> useful on that front instead of just having random isinstance checks
> sprinkled in various bits of code. That's more of an idea for later, I
> guess, but it seems like something we should clean up before too long.
K, I made the requested changes (moved to _connect() function, keyed 
cleanup off of 'cleanup' attribute), and pushed it.

I'm going to be doing a bit of work in remotefilelog in the near future, 
so I'll look into refactoring some of this stuff (like adding a 
isalive() on ssh/httppeer's)
Augie Fackler - Dec. 11, 2015, 7:29 p.m.
> On Dec 11, 2015, at 14:23, Durham Goode <durham@fb.com> wrote:
> 
> 
> 
> On 12/11/15 9:46 AM, Augie Fackler wrote:
>> On Fri, Dec 11, 2015 at 12:22 PM, Durham Goode <durham@fb.com> wrote:
>>>> What's up with the sshpeer.sshpeer isinstance checks? Nothing else is
>>>> assigning to this attribute right now, so it shouldn't matter...
>>> The http peer is also stored in self.remoteserver, so the http tests fails
>>> without that check.
>> 
>> Ah. That's a little groty. I think where I'd go with this is look for
>> self.remoteserver.cleanup() and call that if it exists in the
>> teardown.
>> 
>> I'm less sure what to do about the bit where we ensure the connection
>> is alive - perhaps we should expand the peer API to include something
>> useful on that front instead of just having random isinstance checks
>> sprinkled in various bits of code. That's more of an idea for later, I
>> guess, but it seems like something we should clean up before too long.
> K, I made the requested changes (moved to _connect() function, keyed cleanup off of 'cleanup' attribute), and pushed it.
> 
> I'm going to be doing a bit of work in remotefilelog in the near future, so I'll look into refactoring some of this stuff (like adding a isalive() on ssh/httppeer's)

Awesome. I'm happy to chat about things and/or do reviews if it's helpful.

AF

Patch

diff --git a/remotefilelog/fileserverclient.py b/remotefilelog/fileserverclient.py
--- a/remotefilelog/fileserverclient.py
+++ b/remotefilelog/fileserverclient.py
@@ -133,7 +133,6 @@  def _getfilesbatch(
 
 def _getfiles(
     remote, receivemissing, fallbackpath, progresstick, missed, idmap):
-    remote._callstream("getfiles")
     i = 0
     while i < len(missed):
         # issue a batch of requests
@@ -152,8 +151,6 @@  def _getfiles(
         for j in range(start, end):
             receivemissing(remote.pipei, missed[j])
             progresstick()
-    remote.cleanup()
-    remote = None
 
 class fileserverclient(object):
     """A client for requesting files from the remote file server.
@@ -169,6 +166,7 @@  class fileserverclient(object):
 
         self.localcache = localcache(repo)
         self.remotecache = cacheconnection()
+        self.remoteserver = None
 
     def request(self, fileids):
         """Takes a list of filename/node pairs and fetches them from the
@@ -241,14 +239,26 @@  class fileserverclient(object):
                 verbose = self.ui.verbose
                 self.ui.verbose = False
                 try:
-                    if not fallbackpath:
-                        raise util.Abort("no remotefilelog server configured - "
-                            "is your .hg/hgrc trusted?")
-                    remote = hg.peer(self.ui, {}, fallbackpath)
+                    newconnection = False
+                    if not self.remoteserver:
+                        if not fallbackpath:
+                            raise util.Abort("no remotefilelog server "
+                                "configured - is your .hg/hgrc trusted?")
+                        self.remoteserver = hg.peer(self.ui, {}, fallbackpath)
+                        newconnection = True
+                    elif (isinstance(self.remoteserver, sshpeer.sshpeer) and
+                             self.remoteserver.subprocess.poll() != None):
+                        # The ssh connection died, so recreate it.
+                        self.remoteserver = hg.peer(self.ui, {}, fallbackpath)
+                        newconnection = True
+
+                    remote = self.remoteserver
                     # TODO: deduplicate this with the constant in shallowrepo
                     if remote.capable("remotefilelog"):
                         if not isinstance(remote, sshpeer.sshpeer):
                             raise util.Abort('remotefilelog requires ssh servers')
+                        if newconnection:
+                            remote._callstream("getfiles")
                         _getfiles(remote, self.receivemissing, fallbackpath,
                                   progresstick, missed, idmap)
                     elif remote.capable("getfile"):
@@ -331,6 +341,10 @@  class fileserverclient(object):
         if self.remotecache.connected:
             self.remotecache.close()
 
+        if self.remoteserver and isinstance(self.remoteserver, sshpeer.sshpeer):
+            self.remoteserver.cleanup()
+            self.remoteserver = None
+
     def prefetch(self, fileids, force=False):
         """downloads the given file versions to the cache
         """