| 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
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)
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?
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,
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
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 ;-)
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.
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?
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.
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.
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):