This is an archive of the discontinued Mercurial Phabricator instance.

Differential D894

fsmonitor: warn when fsmonitor could be used
ClosedPublic

Authored by indygreg on Oct 1 2017, 6:49 PM.

Details

Reviewers
mbthomas
krbullock
durin42
Group Reviewers
hg-reviewers
Commits
rHGdf2ff314e36f: fsmonitor: warn when fsmonitor could be used
rHGcb0a1764050c: fsmonitor: warn when fsmonitor could be used
Summary

fsmonitor can significantly speed up operations on large working
directories. But fsmonitor isn't enabled by default, so naive users
may not realize there is a potential to make Mercurial faster.

This commit introduces a warning to working directory updates when
fsmonitor could be used.

The following conditions must be met:

  • Working directory is previously empty
  • New working directory adds >= N files (currently 50,000)
  • Running on Linux or MacOS
  • fsmonitor not enabled
  • Warning not disabled via config override

Because of the empty working directory restriction, most users will
only see this warning during hg clone (assuming very few users
actually do an hg up null).

The addition of a warning may be considered a BC change. However, clone
has printed warnings before. Until recently, Mercurial printed a warning
with the server's certificate fingerprint when it wasn't explicitly
trusted for example. The warning goes to stderr. So it shouldn't
interfere with scripts parsing meaningful output.

The OS restriction was on the advice of Facebook engineers, who only
feel confident with watchman's stability on the supported platforms.

.. feature::

Print warning when fsmonitor isn't being used on a large repository

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

indygreg created this revision.Oct 1 2017, 6:49 PM
mbthomas accepted this revision.Oct 2 2017, 4:32 PM
mbthomas added a subscriber: mbthomas.

LGTM

mercurial/merge.py
1969–1973

I'm not sure if we're planning to work towards PEP8, but it (and I) prefer using parentheses to break long if conditionals, rather than backslashes.

krbullock requested changes to this revision.Oct 4 2017, 10:52 AM
krbullock added a subscriber: krbullock.
krbullock added inline comments.
mercurial/merge.py
1969–1973

It's been our style basically forever to use parens instead of backslashes. I'm honestly shocked we don't have a check-code rule for this.

tests/test-fsmonitor-warning.t
9 ↗(On Diff #2315)

We can't squeeze these into a test that's already set up? Brand-new test cases are discouraged, since so much time in our test suite is already burned on initializing repos.

This revision now requires changes to proceed.Oct 4 2017, 10:52 AM
quark added a subscriber: quark.Oct 17 2017, 7:40 PM
quark added inline comments.
tests/test-fsmonitor-warning.t
9 ↗(On Diff #2315)

There was no fsmonitor related tests. So I think this is fine.

durin42 requested changes to this revision.Oct 18 2017, 4:11 PM
durin42 added a subscriber: durin42.
durin42 added inline comments.
mercurial/merge.py
1969–1973

Yes, definitely use parens and not \.

tests/test-fsmonitor-warning.t
9 ↗(On Diff #2315)

You could easily fold this into any number of existing tests - really all you need is a repo with N files, and the ability to clone twice in a #if block.

indygreg updated this revision to Diff 2998.Oct 18 2017, 4:58 PM
durin42 accepted this revision.Oct 18 2017, 5:01 PM
Closed by commit rHGcb0a1764050c: fsmonitor: warn when fsmonitor could be used (authored by indygreg). · Explain WhyOct 18 2017, 5:05 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/fsmonitor/__init__.py (20 lines)
Mmercurial/configitems.py (6 lines)
Mmercurial/merge.py (33 lines)
Mtests/hghave.py (5 lines)
Mtests/test-clone.t (77 lines)

Diff 2999

hgext/fsmonitor/__init__.py

On a particular Linux system, for a real-world repository with over 400,000 On a particular Linux system, for a real-world repository with over 400,000
files hosted on ext4, vanilla `hg status` takes 1.3 seconds. On the same files hosted on ext4, vanilla `hg status` takes 1.3 seconds. On the same
system, with fsmonitor it takes about 0.3 seconds. system, with fsmonitor it takes about 0.3 seconds.
fsmonitor requires no configuration -- it will tell Watchman about your fsmonitor requires no configuration -- it will tell Watchman about your
repository as necessary. You'll need to install Watchman from repository as necessary. You'll need to install Watchman from
https://facebook.github.io/watchman/ and make sure it is in your PATH. https://facebook.github.io/watchman/ and make sure it is in your PATH.
fsmonitor is incompatible with the largefiles and eol extensions, and
will disable itself if any of those are active.
The following configuration options exist: The following configuration options exist:
:: ::
[fsmonitor] [fsmonitor]
mode = {off, on, paranoid} mode = {off, on, paranoid}
When `mode = off`, fsmonitor will disable itself (similar to not loading the When `mode = off`, fsmonitor will disable itself (similar to not loading the
Whether or not to walk the whole repo ourselves when our cached state has been Whether or not to walk the whole repo ourselves when our cached state has been
invalidated, for example when Watchman has been restarted or .hgignore rules invalidated, for example when Watchman has been restarted or .hgignore rules
have been changed. Walking the repo in that case can result in competing for have been changed. Walking the repo in that case can result in competing for
I/O with Watchman. For large repos it is recommended to set this value to I/O with Watchman. For large repos it is recommended to set this value to
false. You may wish to set this to true if you have a very fast filesystem false. You may wish to set this to true if you have a very fast filesystem
that can outpace the IPC overhead of getting the result data for the full repo that can outpace the IPC overhead of getting the result data for the full repo
from Watchman. Defaults to false. from Watchman. Defaults to false.
fsmonitor is incompatible with the largefiles and eol extensions, and ::
will disable itself if any of those are active.
[fsmonitor]
warn_when_unused = (boolean)
Whether to print a warning during certain operations when fsmonitor would be
beneficial to performance but isn't enabled.
::
[fsmonitor]
warn_update_file_count = (integer)
If ``warn_when_unused`` is set and fsmonitor isn't enabled, a warning will
be printed during working directory updates if this many files will be
created.
''' '''
# Platforms Supported # Platforms Supported
# =================== # ===================
# #
# **Linux:** *Stable*. Watchman and fsmonitor are both known to work reliably, # **Linux:** *Stable*. Watchman and fsmonitor are both known to work reliably,
# even under severe loads. # even under severe loads.
# #

mercurial/configitems.py

default=True, default=True,
) )
coreconfigitem('format', 'usegeneraldelta', coreconfigitem('format', 'usegeneraldelta',
default=True, default=True,
) )
coreconfigitem('format', 'usestore', coreconfigitem('format', 'usestore',
default=True, default=True,
) )
coreconfigitem('fsmonitor', 'warn_when_unused',
default=True,
)
coreconfigitem('fsmonitor', 'warn_update_file_count',
default=50000,
)
coreconfigitem('hooks', '.*', coreconfigitem('hooks', '.*',
default=dynamicdefault, default=dynamicdefault,
generic=True, generic=True,
) )
coreconfigitem('hgweb-paths', '.*', coreconfigitem('hgweb-paths', '.*',
default=list, default=list,
generic=True, generic=True,
) )

mercurial/merge.py

modifiednodeid, modifiednodeid,
nullhex, nullhex,
nullid, nullid,
nullrev, nullrev,
) )
from . import ( from . import (
copies, copies,
error, error,
extensions,
filemerge, filemerge,
match as matchmod, match as matchmod,
obsutil, obsutil,
pycompat, pycompat,
scmutil, scmutil,
subrepo, subrepo,
util, util,
worker, worker,
### apply phase ### apply phase
if not branchmerge: # just jump to the new rev if not branchmerge: # just jump to the new rev
fp1, fp2, xp1, xp2 = fp2, nullid, xp2, '' fp1, fp2, xp1, xp2 = fp2, nullid, xp2, ''
if not partial: if not partial:
repo.hook('preupdate', throw=True, parent1=xp1, parent2=xp2) repo.hook('preupdate', throw=True, parent1=xp1, parent2=xp2)
# note that we're in the middle of an update # note that we're in the middle of an update
repo.vfs.write('updatestate', p2.hex()) repo.vfs.write('updatestate', p2.hex())
# Advertise fsmonitor when its presence could be useful.
#
# We only advertise when performing an update from an empty working
# directory. This typically only occurs during initial clone.
#
# We give users a mechanism to disable the warning in case it is
# annoying.
#
# We only allow on Linux and MacOS because that's where fsmonitor is
# considered stable.
fsmonitorwarning = repo.ui.configbool('fsmonitor', 'warn_when_unused')
fsmonitorthreshold = repo.ui.configint('fsmonitor',
'warn_update_file_count')
try:
extensions.find('fsmonitor')
fsmonitorenabled = repo.ui.config('fsmonitor', 'mode') != 'off'
# We intentionally don't look at whether fsmonitor has disabled
# itself because a) fsmonitor may have already printed a warning
# b) we only care about the config state here.
except KeyError:
fsmonitorenabled = False
if (fsmonitorwarning
and not fsmonitorenabled
and p1.node() == nullid
and len(actions['g']) >= fsmonitorthreshold
and pycompat.sysplatform.startswith(('linux', 'darwin'))):
mbthomasUnsubmitted
Not Done

I'm not sure if we're planning to work towards PEP8, but it (and I) prefer using parentheses to break long if conditionals, rather than backslashes.

mbthomas: I'm not sure if we're planning to work towards PEP8, but it (and I) prefer using parentheses to…
krbullockUnsubmitted
Not Done

It's been our style basically forever to use parens instead of backslashes. I'm honestly shocked we don't have a check-code rule for this.

krbullock: It's been our style basically forever to use parens instead of backslashes. I'm honestly…
durin42Unsubmitted
Not Done

Yes, definitely use parens and not \.

durin42: Yes, definitely use parens and not \.
repo.ui.warn(
_('(warning: large working directory being used without '
'fsmonitor enabled; enable fsmonitor to improve performance; '
'see "hg help -e fsmonitor")\n'))
stats = applyupdates(repo, actions, wc, p2, overwrite, labels=labels) stats = applyupdates(repo, actions, wc, p2, overwrite, labels=labels)
wc.flushall() wc.flushall()
if not partial: if not partial:
with repo.dirstate.parentchange(): with repo.dirstate.parentchange():
repo.setparents(fp1, fp2) repo.setparents(fp1, fp2)
recordupdates(repo, actions, branchmerge) recordupdates(repo, actions, branchmerge)
# update completed, clear state # update completed, clear state

tests/hghave.py

'productbuild', br'Usage: productbuild ', 'productbuild', br'Usage: productbuild ',
ignorestatus=1) ignorestatus=1)
and matchoutput('lsbom', br'Usage: lsbom', ignorestatus=1) and matchoutput('lsbom', br'Usage: lsbom', ignorestatus=1)
and matchoutput( and matchoutput(
'xar --help', br'Usage: xar', ignorestatus=1)) 'xar --help', br'Usage: xar', ignorestatus=1))
except ImportError: except ImportError:
return False return False
@check('linuxormacos', 'Linux or MacOS')
def has_linuxormacos():
# This isn't a perfect test for MacOS. But it is sufficient for our needs.
return sys.platform.startswith(('linux', 'darwin'))
@check("docker", "docker support") @check("docker", "docker support")
def has_docker(): def has_docker():
pat = br'A self-sufficient runtime for' pat = br'A self-sufficient runtime for'
if matchoutput('docker --help', pat): if matchoutput('docker --help', pat):
if 'linux' not in sys.platform: if 'linux' not in sys.platform:
# TODO: in theory we should be able to test docker-based # TODO: in theory we should be able to test docker-based
# package creation on non-linux using boot2docker, but in # package creation on non-linux using boot2docker, but in
# practice that requires extra coordination to make sure # practice that requires extra coordination to make sure

tests/test-clone.t

sending hello command sending hello command
sending between command sending between command
abort: no suitable response from remote hg! abort: no suitable response from remote hg!
[255] [255]
We should not have created a file named owned - if it exists, the We should not have created a file named owned - if it exists, the
attack succeeded. attack succeeded.
$ if test -f owned; then echo 'you got owned'; fi $ if test -f owned; then echo 'you got owned'; fi
Cloning without fsmonitor enabled does not print a warning for small repos
$ hg clone a fsmonitor-default
updating to bookmark @ on branch stable
3 files updated, 0 files merged, 0 files removed, 0 files unresolved
Lower the warning threshold to simulate a large repo
$ cat >> $HGRCPATH << EOF
> [fsmonitor]
> warn_update_file_count = 2
> EOF
We should see a warning about no fsmonitor on supported platforms
#if linuxormacos no-fsmonitor
$ hg clone a nofsmonitor
updating to bookmark @ on branch stable
(warning: large working directory being used without fsmonitor enabled; enable fsmonitor to improve performance; see "hg help -e fsmonitor")
3 files updated, 0 files merged, 0 files removed, 0 files unresolved
#else
$ hg clone a nofsmonitor
updating to bookmark @ on branch stable
3 files updated, 0 files merged, 0 files removed, 0 files unresolved
#endif
We should not see warning about fsmonitor when it is enabled
#if fsmonitor
$ hg clone a fsmonitor-enabled
updating to bookmark @ on branch stable
3 files updated, 0 files merged, 0 files removed, 0 files unresolved
#endif
We can disable the fsmonitor warning
$ hg --config fsmonitor.warn_when_unused=false clone a fsmonitor-disable-warning
updating to bookmark @ on branch stable
3 files updated, 0 files merged, 0 files removed, 0 files unresolved
Loaded fsmonitor but disabled in config should still print warning
#if linuxormacos fsmonitor
$ hg --config fsmonitor.mode=off clone a fsmonitor-mode-off
updating to bookmark @ on branch stable
(warning: large working directory being used without fsmonitor enabled; enable fsmonitor to improve performance; see "hg help -e fsmonitor") (fsmonitor !)
3 files updated, 0 files merged, 0 files removed, 0 files unresolved
#endif
Warning not printed if working directory isn't empty
$ hg -q clone a fsmonitor-update
(warning: large working directory being used without fsmonitor enabled; enable fsmonitor to improve performance; see "hg help -e fsmonitor") (?)
$ cd fsmonitor-update
$ hg up acb14030fe0a
1 files updated, 0 files merged, 2 files removed, 0 files unresolved
(leaving bookmark @)
$ hg up cf0fe1914066
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
`hg update` from null revision also prints
$ hg up null
0 files updated, 0 files merged, 2 files removed, 0 files unresolved
#if linuxormacos no-fsmonitor
$ hg up cf0fe1914066
(warning: large working directory being used without fsmonitor enabled; enable fsmonitor to improve performance; see "hg help -e fsmonitor")
2 files updated, 0 files merged, 0 files removed, 0 files unresolved
#else
$ hg up cf0fe1914066
2 files updated, 0 files merged, 0 files removed, 0 files unresolved
#endif
$ cd ..