This is an archive of the discontinued Mercurial Phabricator instance.

Differential D351

demandimport: disable if chg is being used
ClosedPublic

Authored by quark on Aug 11 2017, 2:26 PM.

Details

Reviewers
phillco
yuja
Group Reviewers
hg-reviewers
Commits
rHG3cfc9070245f: demandimport: disable if chg is being used
Summary

In chg's case, making modules lazily loaded could actually slow down things
since chg pre-imports them. Therefore disable demandimport if chg is being
used.

This is not done by setting HGDEMANDIMPORT chg client-side because that
has side-effects on child processes (hooks, etc).

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Aug 11 2017, 2:26 PM
yuja added a subscriber: yuja.Aug 15 2017, 3:02 AM

Perhaps the chg executable can simply set HGDEMANDIMPORT=disable
if it isn't specified by user.

quark added a comment.EditedAug 15 2017, 11:15 AM

I tried that approach. If we only set HGDEMANDIMPORT for the forked process to execute hg server, it will be an infinite loop - environment config hash will mismatch. Setting HGDEMANDIMPORT for both the forked and non-forked chg client processes seems less cleaner.

sid0 added a subscriber: sid0.Aug 15 2017, 2:05 PM

I tried that approach. If we only set HGDEMANDIMPORT for the forked process to execute hg server, it will be an infinite loop - environment config hash will mismatch. Setting HGDEMANDIMPORT for both the forked and non-forked chg client processes seems less cleaner.

This should be a comment in this patch :)

phillco accepted this revision.Aug 15 2017, 2:39 PM
phillco added a subscriber: phillco.

lgtm, pending sid's comment.

yuja added a comment.Aug 15 2017, 10:52 PM

Setting HGDEMANDIMPORT for both the forked and non-forked chg client processes seems less cleaner.

That seems less bad since the hg script doesn't have to know the chg stuff.

quark added a comment.Aug 15 2017, 11:40 PM

That seems less bad since the hg script doesn't have to know the chg stuff.

Having HGDEMANDIMPORT set may affect hg ran by child process (like hooks).

How about moving the demandimport stuff to dispatch.py? I think eventually dispatch.py will have some special handling about chg.

quark edited the summary of this revision. (Show Details)Aug 16 2017, 1:45 PM
quark retitled this revision from demandimport: disable by default if chg is being used to demandimport: disable if chg is being used.
quark updated this revision to Diff 1003.
quark planned changes to this revision.Aug 16 2017, 2:55 PM
quark updated this revision to Diff 1011.Aug 16 2017, 3:13 PM
yuja added a comment.Aug 17 2017, 9:23 AM

Can you update the comment in chgserver.py in new series? It says
"CHGINTERNALMARK is temporarily set by chg client to detect if chg will start another chg."

quark added a comment.Aug 17 2017, 11:09 AM

How about using sys.argv[0] to test if chg is being used?

Instead of running hg serve --cmdserver chgunix ..., chg client executes chgserve .... And the hg script could notice that by checking sys.argv[0] and does some special handling (expand it to hg serve --cmdserver chgunix for now, but we might want to use a different entry point in the future).

yuja added a comment.Aug 18 2017, 9:11 AM

Instead of running hg serve --cmdserver chgunix ..., chg client executes chgserve .... And the hg script could notice that by checking sys.argv[0] and does some special handling

At this point, I don't think it's simpler than using CHGINTERNALMARK or HGDEMANDIMPORT.

We might need another entry point for chg in future for some reason, but that is the last option IMHO.

quark updated this revision to Diff 1076.Aug 18 2017, 1:58 PM
quark updated this revision to Diff 1078.Aug 18 2017, 2:12 PM
yuja accepted this revision.Aug 19 2017, 2:46 AM

Queued this, thanks.

This revision is now accepted and ready to land.Aug 19 2017, 2:46 AM
Closed by commit rHG3cfc9070245f: demandimport: disable if chg is being used (authored by quark). · Explain WhyAug 19 2017, 2:57 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgdemandimport/__init__.py (4 lines)
Mmercurial/chgserver.py (7 lines)

Diff 1087

hgdemandimport/__init__.py

demandimport.init(ignore) demandimport.init(ignore)
# Re-export. # Re-export.
isenabled = demandimport.isenabled isenabled = demandimport.isenabled
disable = demandimport.disable disable = demandimport.disable
deactivated = demandimport.deactivated deactivated = demandimport.deactivated
def enable(): def enable():
if os.environ.get('HGDEMANDIMPORT') != 'disable': # chg pre-imports modules so do not enable demandimport for it
if ('CHGINTERNALMARK' not in os.environ
and os.environ.get('HGDEMANDIMPORT') != 'disable'):
demandimport.enable() demandimport.enable()

mercurial/chgserver.py

def newconnection(self): def newconnection(self):
self._lastactive = time.time() self._lastactive = time.time()
def createcmdserver(self, repo, conn, fin, fout): def createcmdserver(self, repo, conn, fin, fout):
return chgcmdserver(self.ui, repo, fin, fout, conn, return chgcmdserver(self.ui, repo, fin, fout, conn,
self._hashstate, self._baseaddress) self._hashstate, self._baseaddress)
def chgunixservice(ui, repo, opts): def chgunixservice(ui, repo, opts):
# CHGINTERNALMARK is temporarily set by chg client to detect if chg will # CHGINTERNALMARK is set by chg client. It is an indication of things are
# start another chg. drop it to avoid possible side effects. # started by chg so other code can do things accordingly, like disabling
# demandimport or detecting chg client started by chg client. When executed
# here, CHGINTERNALMARK is no longer useful and hence dropped to make
# environ cleaner.
if 'CHGINTERNALMARK' in encoding.environ: if 'CHGINTERNALMARK' in encoding.environ:
del encoding.environ['CHGINTERNALMARK'] del encoding.environ['CHGINTERNALMARK']
if repo: if repo:
# one chgserver can serve multiple repos. drop repo information # one chgserver can serve multiple repos. drop repo information
ui.setconfig('bundle', 'mainreporoot', '', 'repo') ui.setconfig('bundle', 'mainreporoot', '', 'repo')
h = chgunixservicehandler(ui) h = chgunixservicehandler(ui)
return commandserver.unixforkingservice(ui, repo=None, opts=opts, handler=h) return commandserver.unixforkingservice(ui, repo=None, opts=opts, handler=h)