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
Lint Skipped
Unit
Unit Tests Skipped

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
Mmercurial/dispatch.py (3 lines)

Diff 1011

mercurial/dispatch.py

if util.safehasattr(req.ui, 'ferr'): if util.safehasattr(req.ui, 'ferr'):
if err is not None and err.errno != errno.EPIPE: if err is not None and err.errno != errno.EPIPE:
req.ui.ferr.write('abort: %s\n' % err.strerror) req.ui.ferr.write('abort: %s\n' % err.strerror)
req.ui.ferr.flush() req.ui.ferr.flush()
sys.exit(status & 255) sys.exit(status & 255)
def _enabledemandimport(): def _enabledemandimport():
"""enable importing on demand to reduce startup time""" """enable importing on demand to reduce startup time"""
if 'CHGINTERNALMARK' in encoding.environ:
# do not enable demandimport for chgserver
return
if sys.version_info[0] < 3 or sys.version_info >= (3, 6): if sys.version_info[0] < 3 or sys.version_info >= (3, 6):
demandimport.enable() demandimport.enable()
def _setbinary(): def _setbinary():
"""use binary mode for standard streams""" """use binary mode for standard streams"""
for fp in (sys.stdin, sys.stdout, sys.stderr): for fp in (sys.stdin, sys.stdout, sys.stderr):
util.setbinary(fp) util.setbinary(fp)