This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4666

fastannotate: process files as they arrive
ClosedPublic

Authored by martinvonz on Sep 20 2018, 12:32 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHGd8a7690ccc74: fastannotate: process files as they arrive
Summary

peer.commandexecutor()'s context manager waits for all responses to
arrive in its exit() method. We want to process the results as
they arrive, so we should do that inside the context manager
scope. Note that the futures' result() methods have been replaced to
make sure that the command executor's sendcommands() method is called
when the first future's result is requested, so we don't need to do
that.

A minor side-effect is that we can no longer easily tell when the
server has started sending us responses, so that long statement was
lost.

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

martinvonz created this revision.Sep 20 2018, 12:32 AM
yuja added a subscriber: yuja.Sep 20 2018, 7:58 AM

Queued, thanks.

+ for result in results:
+ r = result.result()

Should we use a future-aware iterator to pick any finished future without
waiting for the first unfinished item?

Closed by commit rHGd8a7690ccc74: fastannotate: process files as they arrive (authored by martinvonz). · Explain WhySep 20 2018, 8:26 AM
This revision was automatically updated to reflect the committed changes.
indygreg added a comment.Sep 20 2018, 11:28 AM

A futures-aware iterator is logically the correct thing to do. I just can't recall if that works with commandexecutor to the buffering nature of outgoing requests. It may require an explicit e.close() or e.sendcommands() to trigger sending the commands over the wire, or else things may deadlock (since I don't think a futures-aware iterator actually calls result(), which would trigger sending buffered commands).

Not that it matters: we don't yet support out-of-order responses. We never will in wire protocol version 1. Version 2 should support it someday. At that time, we can audit wire protocol usage and switch to a futures-aware iterator, as appropriate.

Revision Contents
Changeset List

Diff 11204

hgext/fastannotate/protocol.py

results = [] results = []
with peer.commandexecutor() as batcher: with peer.commandexecutor() as batcher:
ui.debug('fastannotate: requesting %d files\n' % len(paths)) ui.debug('fastannotate: requesting %d files\n' % len(paths))
for p in paths: for p in paths:
results.append(batcher.callcommand( results.append(batcher.callcommand(
'getannotate', 'getannotate',
{'path': p, 'lastnode':lastnodemap.get(p)})) {'path': p, 'lastnode':lastnodemap.get(p)}))
ui.debug('fastannotate: server returned\n')
for result in results: for result in results:
r = result.result() r = result.result()
# TODO: pconvert these paths on the server? # TODO: pconvert these paths on the server?
r = {util.pconvert(p): v for p, v in r.iteritems()} r = {util.pconvert(p): v for p, v in r.iteritems()}
for path in sorted(r): for path in sorted(r):
# ignore malicious paths # ignore malicious paths
if not path.startswith('fastannotate/') or '/../' in (path + '/'): if (not path.startswith('fastannotate/')
or '/../' in (path + '/')):
ui.debug('fastannotate: ignored malicious path %s\n' % path) ui.debug('fastannotate: ignored malicious path %s\n' % path)
continue continue
content = r[path] content = r[path]
if ui.debugflag: if ui.debugflag:
ui.debug('fastannotate: writing %d bytes to %s\n' ui.debug('fastannotate: writing %d bytes to %s\n'
% (len(content), path)) % (len(content), path))
repo.vfs.makedirs(os.path.dirname(path)) repo.vfs.makedirs(os.path.dirname(path))
with repo.vfs(path, 'wb') as f: with repo.vfs(path, 'wb') as f:
f.write(content) f.write(content)
def _filterfetchpaths(repo, paths): def _filterfetchpaths(repo, paths):
"""return a subset of paths whose history is long and need to fetch linelog """return a subset of paths whose history is long and need to fetch linelog
from the server. works with remotefilelog and non-remotefilelog repos. from the server. works with remotefilelog and non-remotefilelog repos.
""" """
threshold = repo.ui.configint('fastannotate', 'clientfetchthreshold', 10) threshold = repo.ui.configint('fastannotate', 'clientfetchthreshold', 10)
if threshold <= 0: if threshold <= 0:
return paths return paths

tests/test-fastannotate-protocol.t

sending hello command sending hello command
sending between command sending between command
remote: * (glob) (?) remote: * (glob) (?)
remote: capabilities: * (glob) remote: capabilities: * (glob)
remote: * (glob) (?) remote: * (glob) (?)
sending protocaps command sending protocaps command
fastannotate: requesting 1 files fastannotate: requesting 1 files
sending getannotate command sending getannotate command
fastannotate: server returned
fastannotate: writing 112 bytes to fastannotate/default/a.l fastannotate: writing 112 bytes to fastannotate/default/a.l
fastannotate: writing 94 bytes to fastannotate/default/a.m fastannotate: writing 94 bytes to fastannotate/default/a.m
fastannotate: a: using fast path (resolved fctx: True) fastannotate: a: using fast path (resolved fctx: True)
0: 1 0: 1
1: 2 1: 2
2: 3 2: 3
3: 4 3: 4
sending hello command sending hello command
sending between command sending between command
remote: * (glob) (?) remote: * (glob) (?)
remote: capabilities: * (glob) remote: capabilities: * (glob)
remote: * (glob) (?) remote: * (glob) (?)
sending protocaps command sending protocaps command
fastannotate: requesting 1 files fastannotate: requesting 1 files
sending getannotate command sending getannotate command
fastannotate: server returned
fastannotate: a: 1 new changesets in the main branch fastannotate: a: 1 new changesets in the main branch
0: 1 0: 1
1: 2 1: 2
2: 3 2: 3
3: 4 3: 4
4: 5 4: 5
if the client has a different "@" which is behind the server. no download is if the client has a different "@" which is behind the server. no download is
$ diff $p1/a.l $p2/a.l $ diff $p1/a.l $p2/a.l
$ diff $p1/a.m $p2/a.m $ diff $p1/a.m $p2/a.m
in the "fctx" mode, the client could also build the cache locally in the "fctx" mode, the client could also build the cache locally
$ hg annotate a --config fastannotate.modes=fctx --debug --config fastannotate.mainbranch=4 | grep fastannotate $ hg annotate a --config fastannotate.modes=fctx --debug --config fastannotate.mainbranch=4 | grep fastannotate
fastannotate: requesting 1 files fastannotate: requesting 1 files
fastannotate: server returned
fastannotate: a: 1 new changesets in the main branch fastannotate: a: 1 new changesets in the main branch
the server would rebuild broken cache automatically the server would rebuild broken cache automatically
$ cp $p2/a.m $p2/a.m.bak $ cp $p2/a.m $p2/a.m.bak
$ echo BROKEN1 > $p1/a.m $ echo BROKEN1 > $p1/a.m
$ echo BROKEN2 > $p2/a.m $ echo BROKEN2 > $p2/a.m
$ hg fastannotate a --debug | grep 'fastannotate: writing' | sort $ hg fastannotate a --debug | grep 'fastannotate: writing' | sort
sending hello command sending hello command
sending between command sending between command
remote: * (glob) (?) remote: * (glob) (?)
remote: capabilities: * (glob) remote: capabilities: * (glob)
remote: * (glob) (?) remote: * (glob) (?)
sending protocaps command sending protocaps command
fastannotate: requesting 1 files fastannotate: requesting 1 files
sending getannotate command sending getannotate command
fastannotate: server returned
fastannotate: writing * (glob) fastannotate: writing * (glob)
fastannotate: writing * (glob) fastannotate: writing * (glob)
$ diff $p1/a.l $p2/a.l $ diff $p1/a.l $p2/a.l
$ diff $p1/a.m $p2/a.m $ diff $p1/a.m $p2/a.m
with the clientfetchthreshold config option, the client can build up the cache with the clientfetchthreshold config option, the client can build up the cache
without downloading from the server without downloading from the server