Gregory Szorc <gregory.szorc@gmail.com> [Wed, 29 Aug 2018 16:43:17 -0700] rev 39560
wireprotoframing: buffer emitted data to reduce frame count
An upcoming commit introduces a wire protocol command that can emit
hundreds of thousands of small objects. Without a buffering layer,
we would emit a single, small frame for every object. Performance
profiling revealed this to be a source of significant overhead for
both client and server.
This commit introduces a very crude buffering layer so that we emit
fewer, bigger frames in such a scenario. This code will likely get
rewritten in the future to be part of the streams API, as we'll
need a similar strategy for compressing data. I don't want to think
about it too much at the moment though.
server
before: user 32.500+0.000 sys 1.160+0.000
after: user 20.230+0.010 sys 0.180+0.000
client
before: user 133.400+0.000 sys 93.120+0.000
after: user 68.370+0.000 sys 32.950+0.000
This appears to indicate we have significant overhead in the frame
processing code on both client and server. It might be worth profiling
that at some point...
Differential Revision: https://phab.mercurial-scm.org/D4473
Gregory Szorc <gregory.szorc@gmail.com> [Wed, 05 Sep 2018 09:06:40 -0700] rev 39559
wireprotov2: implement commands as a generator of objects
Previously, wire protocol version 2 inherited version 1's model of
having separate types to represent the results of different wire
protocol commands.
As I implemented more powerful commands in future commits, I found
I was using a common pattern of returning a special type to hold a
generator. This meant the command function required a closure to
do most of the work. That made logic flow more difficult to follow.
I also noticed that many commands were effectively a sequence of
objects to be CBOR encoded.
I think it makes sense to define version 2 commands as generators.
This way, commands can simply emit the data structures they wish to
send to the client. This eliminates the need for a closure in
command functions and removes encoding from the bodies of commands.
As part of this commit, the handling of response objects has been
moved into the serverreactor class. This puts the reactor in the
driver's seat with regards to CBOR encoding and error handling.
Having error handling in the function that emits frames is
particularly important because exceptions in that function can lead
to things getting in a bad state: I'm fairly certain that uncaught
exceptions in the frame generator were causing deadlocks.
I also introduced a dedicated error type for explicit error reporting
in command handlers. This will be used in subsequent commits.
There's still a bit of work to be done here, especially around
formalizing the error handling "protocol." I've added yet another
TODO to track this so we don't forget.
Test output changed because we're using generators and no longer know
we are at the end of the data until we hit the end of the generator.
This means we can't emit the end-of-stream flag until we've exhausted
the generator. Hence the introduction of 0-sized end-of-stream frames.
Differential Revision: https://phab.mercurial-scm.org/D4472
Gregory Szorc <gregory.szorc@gmail.com> [Mon, 27 Aug 2018 13:30:44 -0700] rev 39558
internals: extract frame-based protocol docs to own document
wireprotocol.txt is quite long and difficult to digest. The
frame-based protocol is effectively a standalone concept (and could
even be used outside of Mercurial). So this commit extracts its
docs to a standalone file.
The first few paragraphs were rewritten as part of the extraction.
Sections headers were adjusted accordingly.
Existing referalls in wireprotocol.txt were updated to refer to the
new doc / concept, which I've started referring to as `hgrpc`.
I'm on the fence as to whether to move the HTTP and SSH transport
details to the new doc as well. For now, I'm leaving them in
wireprotocol.txt.
Differential Revision: https://phab.mercurial-scm.org/D4443
Yuya Nishihara <yuya@tcha.org> [Wed, 12 Sep 2018 22:19:29 +0900] rev 39557
narrow: remove hack to write narrowspec to shared .hg directory
AFAIK, we no longer need it since the narrowspec file was move to the
store directory in
576eef1ab43d, "narrow: move .hg/narrowspec to
.hg/store/narrowspec."
Yuya Nishihara <yuya@tcha.org> [Wed, 12 Sep 2018 22:15:43 +0900] rev 39556
narrowspec: remove parseserverpatterns() which isn't used anymore
Follows up
10a8472f6662, "narrow: drop support for remote expansion."
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 17:22:15 -0700] rev 39555
hg: write narrow patterns after repo creation
Now that hg.clone() knows when a narrow clone is requested, it
makes sense to have it update the narrow patterns for the repo
soon after the repo is created, before any exchange occurs.
Previously, the narrow extension was monkeypatching an exchange
function to do this. The old code is redundant and has been
removed.
Differential Revision: https://phab.mercurial-scm.org/D4541
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 16:59:17 -0700] rev 39554
narrow: don't wrap exchange.pull() during clone
The wrapped version was setting up the narrow repo requirement when
a narrow clone was requested.
Previous commits taught hg.clone() and repo creation to add the narrow
requirement when a narrow clone was requested. So this requirement
should already be set up for us and this code is no longer necessary.
Differential Revision: https://phab.mercurial-scm.org/D4540
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 17:21:18 -0700] rev 39553
exchange: support defining narrow file patterns for pull
This commit teaches exchange.pull() about the desire to perform a
narrow file pull. We simply pass include and exclude patterns to
the function. The values are validated and stored on the pulloperation
instance.
hg.clone() has been taught to pass these arguments to exchange.pull().
If the arguments are not passed to exchange.pull(), the active narrow
patterns from the repository will automatically be used. We /could/
always use the narrow patterns from the repo. However, allowing
explicit values to be passed in allows us to perform data fetching
that doesn't necessarily align with the repo configuration. This
provides more flexibility.
Differential Revision: https://phab.mercurial-scm.org/D4539
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 17:20:14 -0700] rev 39552
commands: pass include and exclude options to hg.clone()
These arguments are defined by the narrow extension. Let's teach
core to recognize them so we can delete some code from the narrow
extension and start to exercise the in-core code for performing a
narrow clone.
We have no way of easily testing it, but this change should result in
.hg/requires having the narrow requirement from the time the file
is written rather than added as part of pull. We'll confirm this when
we delete some monkeypatched functions from the narrow extension in
later commits.
Test output changed because hg.clone() is now receiving patterns
and validation of those values is occurring sooner, before the exchange
code runs and prints the message that was deleted.
Differential Revision: https://phab.mercurial-scm.org/D4538
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 14:16:05 -0700] rev 39551
localrepo: add requirement when narrow files creation option present
The previous commit taught hg.clone() to define a creation option
when file include or exclude patterns are passed.
This commit teaches the new repo creation code to convert that creation
option into a repository requirement.
While not yet used by the narrow extension, the eventual side-effect
of this change is that newly-created repositories will have the narrow
requirement from their creation onset. Currently, the requirement is
added to the repo at exchange.pull() time via a wrapped function in
the narrow extension.
Differential Revision: https://phab.mercurial-scm.org/D4537
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 17:15:35 -0700] rev 39550
hg: recognize include and exclude patterns when cloning
This commit teaches clone() to accept arguments defining file
patterns to clone. This is the first step in teaching core code
about the existence of a narrow clone.
Right now, we only perform validation of the arguments and pass
additional options into createopts to influence repository
creation. Nothing of consequence happens with that creation option
yet, however.
For now, arbitrary restrictions exist, such as not allowing patterns
for shared repos and disabling local copies when patterns are
defined. We can potentially lift these restrictions in the future
once partial clone/storage support is more flushed out. I figure
it is best to reduce the surface area for bugs for the time being.
It may seem weird to prefix these arguments with "store." However,
clone is effectively pull + update and file patterns could apply to
both the store and the working directory. The prefix is there to
disambiguate in the future when this function may want to use
different sets of patterns for the store and working directory.
Differential Revision: https://phab.mercurial-scm.org/D4536
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 17:11:32 -0700] rev 39549
hg: allow extra arguments to be passed to repo creation (API)
Currently, repository creation is influenced by consulting the
ui instance and turning config options into requirements. This
means that in order to influence repository creation, you need
to define and set a config option and that the option must translate
to a requirement stored in the .hg/requires file.
This commit introduces a new mechanism to influence repository
creation. hg.repository() and hg.peer() have been taught to
receive a new optional argument defining extra options to apply
to repository creation. This value is passed along to the various
instance() functions and can be used to influence repository
creation. This will allow us to pass rich data directly to repository
creation without having to go through the config layer. It also allows
us to be more explicit about the features requested during repository
creation and provides a natural point to detect unhandled options
influencing repository creation. The new code detects when unknown
creation options are present and aborts in that case.
.. api:: options can now be passed to influence repository creation
The various instance() functions to spawn new peers or repository
instances now receive a ``createopts`` argument that can be a
dict defining additional options to influence repository creation.
localrepo.newreporequirements() also receives this argument.
Differential Revision: https://phab.mercurial-scm.org/D4535
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 13:46:59 -0700] rev 39548
localrepo: move repo creation logic out of localrepository.__init__ (API)
It has long bothered me that local repository creation is handled as
part of localrepository.__init__. Upcoming changes I want to make
around how repositories are initialized and instantiated will make
the continued existence of repository creation code in
localrepository.__init__ even more awkward.
localrepository instances are almost never constructed directly:
instead, callers are supposed to go through hg.repository() to obtain
a handle on a repository. And hg.repository() calls
localrepo.instance() to return a new repo instance.
This commit teaches localrepo.instance() to handle the create=True
logic. Most of the code for repo construction has been moved to a
standalone function. This allows extensions to monkeypatch the function
to further customize freshly-created repositories.
A few calls to localrepo.localrepository.__init__ that were passing
create=True were converted to call localrepo.instance().
.. api:: local repo creation moved out of constructor
``localrepo.localrepository.__init__`` no longer accepts a
``create`` argument to create a new repository. New repository
creation is now performed as part of ``localrepo.instance()``
and the bulk of the work is performed by
``localrepo.createrepository()``.
Differential Revision: https://phab.mercurial-scm.org/D4534
Matt Harbison <matt_harbison@yahoo.com> [Tue, 11 Sep 2018 13:52:17 -0400] rev 39547
subrepo: mask out passwords embedded in the messages displaying a URL
I noticed the password in maintenance logs for the "no changes since last push"
and "pushing to" messages when pushing with an explicit path. But the test case
here with :pushurl was also affected. I didn't see that cloning or pulling
subrepos on demand had this problem, but it seems safer to just mask that too.
There's a bit of a disconnect here, because it looks like clone is slicing off
the password (makes sense not to store it in the hgrc in cleartext). But not
shearing it off of an explicit path causes the subrepo not to realize that it
already pushed the latest stuff. This is the easiest fix, however.
Gregory Szorc <gregory.szorc@gmail.com> [Fri, 07 Sep 2018 15:57:55 -0700] rev 39546
localrepo: pass ui to newreporequirements() (API)
newreporequirements() is called as part of creating a new repository.
It doesn't make much sense for it to receive a repo instance as part
of determining what requirements for new repos should be.
.. api::
localrepo.newreporequirements() receives a ui instead of a repo
Differential Revision: https://phab.mercurial-scm.org/D4533
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 15:40:33 -0700] rev 39545
narrow: set opts['narrow'] instead of local variable
This will allow the command function in core to infer the presence
of the option without duplicating logic.
Differential Revision: https://phab.mercurial-scm.org/D4532
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 15:53:12 -0700] rev 39544
narrow: drop support for remote expansion (BC)
Previous patches to validate narrow patterns accidentically dropped
support for the include: syntax that allows patterns to be expanded
from a remote.
This feature was never implemented in core and is only implemented on
Google's custom server. Per @martinvonz's review comment in D4522, it
is OK to drop this feature since it isn't used.
The concept of this feature does seem useful. I anticipate it making
a comeback some day in some shape or form. But for now, let's jettison
the dead code.
Differential Revision: https://phab.mercurial-scm.org/D4530
Gregory Szorc <gregory.szorc@gmail.com> [Fri, 07 Sep 2018 18:35:54 -0700] rev 39543
fastannotate: use repo.local()
This is the proper way to check whether we're dealing with a local
repository, since extensions should be coding to an interface and
not testing for exact types.
Differential Revision: https://phab.mercurial-scm.org/D4542
Martin von Zweigbergk <martinvonz@google.com> [Tue, 11 Sep 2018 16:04:55 -0700] rev 39542
tests: drop extra "file:" prefix from paths in narrow test
It looks like these were added by mistake in
f4d4bd8c8911 (narrow: add
a --narrowspec flag to clone command, 2018-08-08).
Differential Revision: https://phab.mercurial-scm.org/D4531
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 11:47:10 -0700] rev 39541
narrow: validate spec files are well-formed during clone (BC)
Previously, specfiles would get read then normalized. We want
specfiles to be normalized on read so there is no confusion about
what the format of specfiles should be.
This commit validates the parsed result of --specfile. If entries
aren't prefixed, an error is raised.
Previously, validation would occur at exchange time, hence why we
dropped a line of test output related to server iteraction.
Differential Revision: https://phab.mercurial-scm.org/D4526
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 10:59:21 -0700] rev 39540
narrow: validate patterns on incoming bundle2 part
The remote data is untrusted and needs to be validated for
pattern conformance.
Differential Revision: https://phab.mercurial-scm.org/D4525
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 15:28:41 -0700] rev 39539
narrowspec: validate patterns when loading and saving spec file
Patterns should be normalized and validated before being passed into
narrowspec.save(). Let's assert that by checking immediately before
writing the narrow spec file. And let's assert that patterns loaded
from the spec file also conform.
Differential Revision: https://phab.mercurial-scm.org/D4524
Yuya Nishihara <yuya@tcha.org> [Mon, 10 Sep 2018 22:34:19 +0900] rev 39538
ancestor: use heapreplace() in place of heappop/heappush()
This should be slightly faster.
Overall perfancestors result::
cpython nginx mercurial
------------- ---------------- ---------------- ----------------
b6db2e80a9ce^ 0.103461 0.006303 0.035716
8eb2145ff0fb 0.192307 (x1.86) 0.012115 (x1.92) 0.052135 (x1.46)
this patch 0.139986 (x1.35) 0.006389 (x1.01) 0.037176 (x1.04)
Yuya Nishihara <yuya@tcha.org> [Tue, 11 Sep 2018 22:36:51 +0900] rev 39537
ancestor: rename local aliases of heapq functions in _lazyancestorsiter()
The original names no longer look pretty. Just call them as heap*() instead.
Yuya Nishihara <yuya@tcha.org> [Mon, 10 Sep 2018 21:58:59 +0900] rev 39536
ancestor: optimize _lazyancestorsiter() for contiguous chains
If there's no revision between p1 and current, p1 must be the next revision
to visit. In this case, we can get around the overhead of heappop/push
operations. Note that this is faster than using heapreplace().
'current - p1 == 1' could be generalized as 'all(r not in seen for r in
xrange(p1, current)', but Python is too slow to do such thing.
Yuya Nishihara <yuya@tcha.org> [Mon, 10 Sep 2018 21:54:40 +0900] rev 39535
ancestor: unroll loop of parents in _lazyancestorsiter()
This change itself isn't major performance win, but it helps optimizing
the visit loop for contiguous chains. See the next patch.
Yuya Nishihara <yuya@tcha.org> [Mon, 10 Sep 2018 21:46:19 +0900] rev 39534
ancestor: return early from _lazyancestorsiter() when reached to stoprev
There's no need to empty the heap.
Yuya Nishihara <yuya@tcha.org> [Tue, 11 Sep 2018 22:38:32 +0900] rev 39533
ancestor: remove alias of initrevs from _lazyancestorsiter()
It's just redundant and less comprehensible.
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 10:36:07 -0700] rev 39532
narrow: validate patterns returned by expandnarrow
Remotes could supply malicious or invalid patterns. We should
validate them as soon as possible.
Differential Revision: https://phab.mercurial-scm.org/D4523
Gregory Szorc <gregory.szorc@gmail.com> [Tue, 11 Sep 2018 15:25:35 -0700] rev 39531
narrowspec: limit patterns to path: and rootfilesin: (BC)
Some matcher patterns are computationally expensive and may even
have security issues (e.g. evaluating some file sets). For these
reasons, we want to limit the types of matcher patterns that can
be used in narrow specs and by command line arguments used for
defining narrow specs.
This commit teaches ``narrowspec.parsepatterns()`` to validate the
pattern types against "safe" patterns.
Surprisingly, no existing tests broke. So tests for the feature
have been added.
We also added a function to validate a patterns data structure.
This will be used in future commits.
Differential Revision: https://phab.mercurial-scm.org/D4522