Patchwork [3,of,3,V2] chgserver: add a config option to preload repo

login
register
mail settings
Submitter Jun Wu
Date March 16, 2016, 12:08 p.m.
Message ID <6e00407bda3045d0abe1.1458130091@x1c>
Download mbox | patch
Permalink /patch/13913/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - March 16, 2016, 12:08 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1457986958 0
#      Mon Mar 14 20:22:38 2016 +0000
# Node ID 6e00407bda3045d0abe19c0c2c5eba8fd296d41e
# Parent  f09ac10a2f99ebc65143aff19fa73d1ad3024da0
chgserver: add a config option to preload repo

chgserver was designed to load repo per request to minimize the number of
server processes and to be more confident. But it may also work with repo
preloaded as well, which should be good for performance but is more risky
in current situation due to repo.invalidateall and ui/config handling is
not perfect yet.

This patch adds an experimental config option "chgserver.preloadrepo" and
changes confighash and mtimehash to optionally include the repo directory.
It will also change the default behavior to drop the repo object, which
will solve the big issue that chg uses a wrong repo for users.
Pierre-Yves David - March 16, 2016, 8:12 p.m.
On 03/16/2016 05:08 AM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1457986958 0
> #      Mon Mar 14 20:22:38 2016 +0000
> # Node ID 6e00407bda3045d0abe19c0c2c5eba8fd296d41e
> # Parent  f09ac10a2f99ebc65143aff19fa73d1ad3024da0
> chgserver: add a config option to preload repo
>
> chgserver was designed to load repo per request to minimize the number of
> server processes and to be more confident. But it may also work with repo
> preloaded as well, which should be good for performance but is more risky
> in current situation due to repo.invalidateall and ui/config handling is
> not perfect yet.
>
> This patch adds an experimental config option "chgserver.preloadrepo" and
> changes confighash and mtimehash to optionally include the repo directory.
> It will also change the default behavior to drop the repo object, which
> will solve the big issue that chg uses a wrong repo for users.
>
> diff --git a/hgext/chgserver.py b/hgext/chgserver.py
> --- a/hgext/chgserver.py
> +++ b/hgext/chgserver.py
> @@ -34,8 +34,11 @@
>   ::
>
>     [chgserver]
> -  idletimeout = 3600 # seconds, after which an idle server will exit
> -  skiphash = False   # whether to skip config or env change checks
> +  idletimeout = 3600  # seconds, after which an idle server will exit
> +  preloadrepo = False # whether to preload repo object (experimental)
> +                      # set it to true may increase the number of server
> +                      # processes but will make things a bit faster
> +  skiphash = False    # whether to skip config or env change checks

Isn't it a bit early to play with that ? I would advocate that we get 
something running (pass all tests, including 3rd party of interrest to 
you) then introduce these option with some timing data.

Because right now, we suspect this would help but we seems to be lacking 
performance number to back there usefulness.

(also, please make a patch for each option)
Jun Wu - March 16, 2016, 10:59 p.m.
On 03/16/2016 08:12 PM, Pierre-Yves David wrote:
> Isn't it a bit early to play with that ? I would advocate that we get
> something running (pass all tests, including 3rd party of interrest to you)
> then introduce these option with some timing data.

The patch is about a new direction. I do want feedback on this topic early.
It won't change the current chg behavior by default.

> Because right now, we suspect this would help but we seems to be lacking
> performance number to back there usefulness.

I have the number for a large fb repo. "preloadrepo = True" saves 40ms.

> (also, please make a patch for each option)

Do you mean I need 2 extra patches to do the space change?
Pierre-Yves David - March 16, 2016, 11:13 p.m.
On 03/16/2016 03:59 PM, Jun Wu wrote:
> On 03/16/2016 08:12 PM, Pierre-Yves David wrote:
>> Isn't it a bit early to play with that ? I would advocate that we get
>> something running (pass all tests, including 3rd party of interrest to
>> you)
>> then introduce these option with some timing data.
>
> The patch is about a new direction. I do want feedback on this topic early.
> It won't change the current chg behavior by default.

Then it should live the 'experimental' section until we get a better 
idea of if this is useful or not.

>> Because right now, we suspect this would help but we seems to be lacking
>> performance number to back there usefulness.
>
> I have the number for a large fb repo. "preloadrepo = True" saves 40ms.

You should include them in the commit description then.

>> (also, please make a patch for each option)
>
> Do you mean I need 2 extra patches to do the space change?

Nevermind I got confused ny the skiphash movement and though you were 
adding it. Sorry about that

Cheers,
Yuya Nishihara - March 17, 2016, 1:13 a.m.
On Wed, 16 Mar 2016 16:13:01 -0700, Pierre-Yves David wrote:
> On 03/16/2016 03:59 PM, Jun Wu wrote:
> > On 03/16/2016 08:12 PM, Pierre-Yves David wrote:
> >> Isn't it a bit early to play with that ? I would advocate that we get
> >> something running (pass all tests, including 3rd party of interrest to
> >> you)
> >> then introduce these option with some timing data.
> >
> > The patch is about a new direction. I do want feedback on this topic early.
> > It won't change the current chg behavior by default.
> 
> Then it should live the 'experimental' section until we get a better
> idea of if this is useful or not.

Oops, I said "chgserver" section would be okay as the extension is experimental.

> >> Because right now, we suspect this would help but we seems to be lacking
> >> performance number to back there usefulness.
> >
> > I have the number for a large fb repo. "preloadrepo = True" saves 40ms.

But the cached repo would be useless once the repository is modified, because
the server forks per connection, right?

I don't think this caching strategy would work well.


> +def _reporoot(repo):
> +    if not repo:
> +        return None
> +    return os.path.realpath(getattr(repo, 'root', None))

Nit: os.path.realpath(None) raises exception
Pierre-Yves David - March 17, 2016, 1:17 a.m.
On 03/16/2016 06:13 PM, Yuya Nishihara wrote:
> On Wed, 16 Mar 2016 16:13:01 -0700, Pierre-Yves David wrote:
>> On 03/16/2016 03:59 PM, Jun Wu wrote:
>>> On 03/16/2016 08:12 PM, Pierre-Yves David wrote:
>>>> Isn't it a bit early to play with that ? I would advocate that we get
>>>> something running (pass all tests, including 3rd party of interrest to
>>>> you)
>>>> then introduce these option with some timing data.
>>>
>>> The patch is about a new direction. I do want feedback on this topic early.
>>> It won't change the current chg behavior by default.
>>
>> Then it should live the 'experimental' section until we get a better
>> idea of if this is useful or not.
>
> Oops, I said "chgserver" section would be okay as the extension is experimental.

ha, interresting point. You are probably right here. But the option 
still have a flavor of "some random flag that won't survive once we know 
what is the right way." so it made me thing of experimental.

How much of chgserver were already existing in its own repository. 
Because in that case, this is "experimental" but not so much as we 
probably don't want to break older user for nothing?

(on "where" this option live, I'll let yuya decide)

>>>> Because right now, we suspect this would help but we seems to be lacking
>>>> performance number to back there usefulness.
>>>
>>> I have the number for a large fb repo. "preloadrepo = True" saves 40ms.
>
> But the cached repo would be useless once the repository is modified, because
> the server forks per connection, right?
>
> I don't think this caching strategy would work well.
>
>
>> +def _reporoot(repo):
>> +    if not repo:
>> +        return None
>> +    return os.path.realpath(getattr(repo, 'root', None))
>
> Nit: os.path.realpath(None) raises exception

If this raises exception, I would raise your concern a bit above "nit" 
then ;-)
Jun Wu - March 17, 2016, 7:59 a.m.
On 03/17/2016 01:13 AM, Yuya Nishihara wrote:
> But the cached repo would be useless once the repository is modified,
> because the server forks per connection, right?
>
> I don't think this caching strategy would work well.

There is repo.invalidateall() in commandserver.py. So I guess it is designed to
work, although it needs more attention. From my profiling data. Skipping
reposetup()s seems to be the only way if we want trivial commands like
"hg bookmark" faster.

>> +def _reporoot(repo): +    if not repo: +        return None +    return
>> os.path.realpath(getattr(repo, 'root', None))
>
> Nit: os.path.realpath(None) raises exception

Nice catch.
Yuya Nishihara - March 18, 2016, 4:47 a.m.
On Thu, 17 Mar 2016 07:59:27 +0000, Jun Wu wrote:
> On 03/17/2016 01:13 AM, Yuya Nishihara wrote:
> > But the cached repo would be useless once the repository is modified,
> > because the server forks per connection, right?
> >
> > I don't think this caching strategy would work well.
> 
> There is repo.invalidateall() in commandserver.py. So I guess it is designed to
> work, although it needs more attention.

I mean filecache never be updated (i.e. have no/stale cache) because repository
operation is done after fork().

> From my profiling data. Skipping
> reposetup()s seems to be the only way if we want trivial commands like
> "hg bookmark" faster.

Uh, is reposteup() such slow?
Jun Wu - March 18, 2016, 1:28 p.m.
On 03/17/2016 09:47 PM, Yuya Nishihara wrote:
> I mean filecache never be updated (i.e. have no/stale cache) because repository
> operation is done after fork().

Can they be invalidated in repo.invalidall ?

>>  From my profiling data. Skipping
>> reposetup()s seems to be the only way if we want trivial commands like
>> "hg bookmark" faster.
>
> Uh, is reposteup() such slow?

40ms in our case. I wrote a lightweight C tracer and it seems a lot of small 
stuffs added up to this value.
Yuya Nishihara - March 18, 2016, 2:55 p.m.
On Fri, 18 Mar 2016 06:28:43 -0700, Jun Wu wrote:
> On 03/17/2016 09:47 PM, Yuya Nishihara wrote:
> > I mean filecache never be updated (i.e. have no/stale cache) because repository
> > operation is done after fork().
> 
> Can they be invalidated in repo.invalidall ?

If mtime differs, filecache should be discarded by invalidateall().
Since the repo object in the main process never be updated, mtime would always
differ once you modified the repository.

> >>  From my profiling data. Skipping
> >> reposetup()s seems to be the only way if we want trivial commands like
> >> "hg bookmark" faster.
> >
> > Uh, is reposteup() such slow?
> 
> 40ms in our case. I wrote a lightweight C tracer and it seems a lot of small 
> stuffs added up to this value.

Hmm, it seems cold cache is still warmer in your case.
Jun Wu - March 18, 2016, 6:06 p.m.
On 03/18/2016 07:55 AM, Yuya Nishihara wrote:
> If mtime differs, filecache should be discarded by invalidateall(). Since
> the repo object in the main process never be updated, mtime would always
> differ once you modified the repository.

Thanks for the explanation. I think there are ways to periodically make the
cache warmer. But that sounds like another series.

> Hmm, it seems cold cache is still warmer in your case.

I was testing "hg bookmark", it seems not related to filecache? If filecache is
the issue here, we probably can do further optimizations later.

Patch

diff --git a/hgext/chgserver.py b/hgext/chgserver.py
--- a/hgext/chgserver.py
+++ b/hgext/chgserver.py
@@ -34,8 +34,11 @@ 
 ::
 
   [chgserver]
-  idletimeout = 3600 # seconds, after which an idle server will exit
-  skiphash = False   # whether to skip config or env change checks
+  idletimeout = 3600  # seconds, after which an idle server will exit
+  preloadrepo = False # whether to preload repo object (experimental)
+                      # set it to true may increase the number of server
+                      # processes but will make things a bit faster
+  skiphash = False    # whether to skip config or env change checks
 """
 
 from __future__ import absolute_import
@@ -45,6 +48,7 @@ 
 import inspect
 import os
 import re
+import stat
 import struct
 import sys
 import threading
@@ -95,10 +99,12 @@ 
                     |TZ
                     )\Z''', re.X)
 
-def _confighash(ui):
+def _confighash(ui, reporoot=None):
     """return a quick hash for detecting config/env changes
 
     confighash is the hash of sensitive config items and environment variables.
+    optionally it can include repo root path, which is useful if repo needs to
+    be preloaded as well.
 
     for chgserver, it is designed that once confighash changes, the server is
     not qualified to serve its client and should redirect the client to a new
@@ -112,15 +118,19 @@ 
     sectionhash = _hashlist(sectionitems)
     envitems = [(k, v) for k, v in os.environ.iteritems() if _envre.match(k)]
     envhash = _hashlist(sorted(envitems))
-    return sectionhash[:6] + envhash[:6]
+    result = sectionhash[:6] + envhash[:6]
+    if reporoot:
+        result += _hashlist([os.path.abspath(reporoot)])[:6]
+    return result
 
-def _getmtimepaths(ui):
+def _getmtimepaths(ui, reporoot=None):
     """get a list of paths that should be checked to detect change
 
     The list will include:
     - extensions (will not cover all files for complex extensions)
     - mercurial/__version__.py
     - python binary
+    - reporoot, reporoot/.hg (if reporoot is present)
     """
     modules = [m for n, m in extensions.extensions(ui)]
     try:
@@ -134,6 +144,8 @@ 
             files.append(inspect.getabsfile(m))
         except TypeError:
             pass
+    if reporoot:
+        files += [reporoot, os.path.join(reporoot, '.hg')]
     return sorted(set(files))
 
 def _mtimehash(paths):
@@ -145,6 +157,7 @@ 
     it's possible to return different hashes for same file contents.
     it's also possible to return a same hash for different file contents for
     some carefully crafted situation.
+    for directories, inode number is used instead of size and mtime.
 
     for chgserver, it is designed that once mtimehash changes, the server is
     considered outdated immediately and should no longer provide service.
@@ -152,7 +165,10 @@ 
     def trystat(path):
         try:
             st = os.stat(path)
-            return (st.st_mtime, st.st_size)
+            if stat.S_ISDIR(st.st_mode):
+                return (st.st_ino,)
+            else:
+                return (st.st_mtime, st.st_size)
         except OSError:
             # could be ENOENT, EPERM etc. not fatal in any case
             pass
@@ -166,10 +182,10 @@ 
         self.mtimepaths = mtimepaths
 
     @staticmethod
-    def fromui(ui, mtimepaths=None):
+    def fromui(ui, reporoot=None, mtimepaths=None):
         if mtimepaths is None:
-            mtimepaths = _getmtimepaths(ui)
-        confighash = _confighash(ui)
+            mtimepaths = _getmtimepaths(ui, reporoot)
+        confighash = _confighash(ui, reporoot)
         mtimehash = _mtimehash(mtimepaths)
         _log('confighash = %s mtimehash = %s\n' % (confighash, mtimehash))
         return hashstate(confighash, mtimehash, mtimepaths)
@@ -258,6 +274,7 @@ 
     return chgui(srcui)
 
 def _renewui(srcui, args=None):
+    """Return (newui, reporoot)"""
     if not args:
         args = []
 
@@ -291,7 +308,7 @@ 
             # ui.configsource returns 'none' by default
             source = ''
         newui.setconfig(section, name, value, source)
-    return newui
+    return (newui, path)
 
 class channeledsystem(object):
     """Propagate ui.system() request in the following format:
@@ -433,14 +450,17 @@ 
               This may happen if we cannot parse the config.
         """
         args = self._readlist()
+        reporoot = None
         try:
-            self.ui = _renewui(self.ui, args)
+            self.ui, reporoot = _renewui(self.ui, args)
+            reporoot = os.path.realpath(reporoot)
         except error.ParseError as inst:
             dispatch._formatparse(self.ui.warn, inst)
             self.ui.flush()
             self.cresult.write('exit 255')
             return
-        newhash = hashstate.fromui(self.ui, self.hashstate.mtimepaths)
+        newhash = hashstate.fromui(self.ui, self.repo and reporoot,
+                                   self.hashstate.mtimepaths)
         insts = []
         if newhash.mtimehash != self.hashstate.mtimehash:
             addr = _hashaddress(self.baseaddress, self.hashstate.confighash)
@@ -513,7 +533,7 @@ 
 
         if set(['HGPLAIN', 'HGPLAINEXCEPT']) & diffkeys:
             # reload config so that ui.plain() takes effect
-            self.ui = _renewui(self.ui)
+            self.ui = _renewui(self.ui)[0]
 
         _clearenvaliases(commands.table)
 
@@ -560,6 +580,11 @@ 
             traceback.print_exc(file=cerr)
             raise
 
+def _reporoot(repo):
+    if not repo:
+        return None
+    return os.path.realpath(getattr(repo, 'root', None))
+
 def _tempaddress(address):
     return '%s.%d.tmp' % (address, os.getpid())
 
@@ -607,9 +632,9 @@ 
 
     def issocketowner(self):
         try:
-            stat = os.stat(self.server_address)
-            return (stat.st_ino == self._socketstat.st_ino and
-                    stat.st_mtime == self._socketstat.st_mtime)
+            st = os.stat(self.server_address)
+            return (st.st_ino == self._socketstat.st_ino and
+                    st.st_mtime == self._socketstat.st_mtime)
         except OSError:
             return False
 
@@ -628,7 +653,11 @@ 
 
 class chgunixservice(commandserver.unixservice):
     def init(self):
-        self.repo = None
+        if not self.ui.configbool('chgserver', 'preloadrepo', False) \
+           or not _reporoot(self.repo):
+            # do not preload repo if we cannot get repo root (therefore cannot
+            # hash it), or the user choose not to
+            self.repo = None
         self._inithashstate()
         class cls(AutoExitMixIn, SocketServer.ForkingMixIn,
                   SocketServer.UnixStreamServer):
@@ -647,7 +676,7 @@ 
         if self.ui.configbool('chgserver', 'skiphash', False):
             self.hashstate = None
             return
-        self.hashstate = hashstate.fromui(self.ui)
+        self.hashstate = hashstate.fromui(self.ui, _reporoot(self.repo))
         self.address = _hashaddress(self.address, self.hashstate.confighash)
 
     def _createsymlink(self):