This is an archive of the discontinued Mercurial Phabricator instance.

Differential D80

phabstatus: query conduit more efficiently for "hg log"
ClosedPublic

Authored by simpkins on Jul 13 2017, 10:32 PM.
Tags
None
Subscribers
ryanmce

Details

Reviewers
ryanmce
Group Reviewers
Restricted Project
Commits
rFBHGX9c846cbfe7d5: phabstatus: query conduit more efficiently for "hg log"
Summary

Update the phabstatus code to peek ahead at revisions to be logged by
hg log and hg log --graph and query phabricator for multiple revisions at
once, rather than having to query them one at a time.

Previously it had special integration with the smartlog where it could do this
for smartlog, but not for the normal "hg log" command.

Wrapping the revset iterator this way feels somewhat hacky, but this seems like
the best way to achieve the desired results without upstream changes. (In an
ideal world we would perhaps update the log code to process things in an
asynchronous fashion, to better support templates that may take a non-trivial
amount time to render. Ideally the revset query would be processed in parallel
with template rendering.)

Test Plan

Confirmed existing tests still pass, and manually tested various combinations
of hg log, hg log --graph and hg smartlog

Diff Detail

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

Event Timeline

simpkins created this revision.Jul 13 2017, 10:32 PM
ryanmce accepted this revision.Jul 19 2017, 9:56 AM
ryanmce added a subscriber: ryanmce.

Nice addition

This revision is now accepted and ready to land.Jul 19 2017, 9:56 AM
Closed by commit rFBHGX9c846cbfe7d5: phabstatus: query conduit more efficiently for "hg log" (authored by simpkins). · Explain WhyJul 19 2017, 10:22 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/phabstatus.py (165 lines)
Mtests/test-check-config-hg.t (1 line)

Diff 329

hgext3rd/phabstatus.py

# phabstatus.py # phabstatus.py
# #
# Copyright 2015 Facebook, Inc. # Copyright 2015 Facebook, Inc.
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from mercurial import extensions, registrar from mercurial import cmdutil, extensions, registrar
from mercurial import util as hgutil from mercurial import util as hgutil
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial import obsutil from mercurial import obsutil
from phabricator import ( from phabricator import (
arcconfig, arcconfig,
conduit, conduit,
diffprops, diffprops,
for diff in diffid: for diff in diffid:
matchingresponse = resp.get(diff) matchingresponse = resp.get(diff)
if not matchingresponse: if not matchingresponse:
result.append("Error") result.append("Error")
else: else:
result.append(matchingresponse) result.append(matchingresponse)
return result return result
def populateresponseforphab(repo, ctx): def populateresponseforphab(repo, diffnum):
""":populateresponse: Runs the memoization function """:populateresponse: Runs the memoization function
for use of phabstatus and sync status for use of phabstatus and sync status
""" """
if hgutil.safehasattr(repo, '_smartlogrevs'): if not hgutil.safehasattr(repo, '_phabstatusrevs'):
return
if (hgutil.safehasattr(repo, '_phabstatuscache') and
(repo, diffnum) in repo._phabstatuscache):
# We already have cached data for this diff
return
next_revs = repo._phabstatusrevs.peekahead()
if repo._phabstatusrevs.done:
# repo._phabstatusrevs doesn't have anything else to process.
# Remove it so we will bail out earlier next time.
del repo._phabstatusrevs
alldiffnumbers = [getdiffnum(repo, repo[rev]) alldiffnumbers = [getdiffnum(repo, repo[rev])
for rev in repo._smartlogrevs] for rev in next_revs]
okdiffnumbers = [d for d in alldiffnumbers if d is not None] okdiffnumbers = set(d for d in alldiffnumbers if d is not None)
# Make sure we always include the requested diff number
okdiffnumbers.add(diffnum)
# To populate the cache, the result will be used by the templater # To populate the cache, the result will be used by the templater
getdiffstatus(repo, *okdiffnumbers) getdiffstatus(repo, *okdiffnumbers)
# Do this once per smartlog call, not for every revs to be displayed
del repo._smartlogrevs
templatekeyword = registrar.templatekeyword() templatekeyword = registrar.templatekeyword()
@templatekeyword('phabstatus') @templatekeyword('phabstatus')
def showphabstatus(repo, ctx, templ, **args): def showphabstatus(repo, ctx, templ, **args):
"""String. Return the diff approval status for a given hg rev """String. Return the diff approval status for a given hg rev
""" """
populateresponseforphab(repo, ctx)
diffnum = getdiffnum(repo, ctx) diffnum = getdiffnum(repo, ctx)
if diffnum is not None: if diffnum is None:
return None
populateresponseforphab(repo, diffnum)
result = getdiffstatus(repo, diffnum)[0] result = getdiffstatus(repo, diffnum)[0]
if isinstance(result, dict) and "status" in result: if isinstance(result, dict) and "status" in result:
return result.get("status") return result.get("status")
else: else:
return "Error" return "Error"
""" """
in order to determine whether the local changeset is in sync with the in order to determine whether the local changeset is in sync with the
remote one we compare the hash of the current changeset with the one we remote one we compare the hash of the current changeset with the one we
get from the remote (phabricator) repo. There are three different cases get from the remote (phabricator) repo. There are three different cases
and we deal with them seperately. and we deal with them seperately.
1) If this is the first revision in a diff: We look at the count field and 1) If this is the first revision in a diff: We look at the count field and
understand that this is the first changeset, so we compare the hash we get understand that this is the first changeset, so we compare the hash we get
from remote repo with the predessesor's hash from the local changeset. The from remote repo with the predessesor's hash from the local changeset. The
reason for that is the D number is ammended on the changeset after it is reason for that is the D number is ammended on the changeset after it is
sent to phabricator. sent to phabricator.
2) If this is the last revision, i.e. it is alread committed: Then we 2) If this is the last revision, i.e. it is alread committed: Then we
don't say anything. All good. don't say anything. All good.
3) If this is a middle revision: Then we compare the hashes as regular. 3) If this is a middle revision: Then we compare the hashes as regular.
""" """
@templatekeyword('syncstatus') @templatekeyword('syncstatus')
def showsyncstatus(repo, ctx, templ, **args): def showsyncstatus(repo, ctx, templ, **args):
"""String. Return whether the local revision is in sync """String. Return whether the local revision is in sync
with the remote (phabricator) revision with the remote (phabricator) revision
""" """
diffnum = getdiffnum(repo, ctx) diffnum = getdiffnum(repo, ctx)
if diffnum is None: if diffnum is None:
return None return None
populateresponseforphab(repo, ctx) populateresponseforphab(repo, diffnum)
results = getdiffstatus(repo, diffnum) results = getdiffstatus(repo, diffnum)
try: try:
result = results[0] result = results[0]
remote = result["hash"] remote = result["hash"]
status = result["status"] status = result["status"]
count = int(result["count"]) count = int(result["count"])
except (IndexError, KeyError, ValueError, TypeError): except (IndexError, KeyError, ValueError, TypeError):
# We got no result back, or it did not contain all required fields # We got no result back, or it did not contain all required fields
elif status == "Committed": elif status == "Committed":
return "committed" return "committed"
else: else:
return "unsync" return "unsync"
def getdiffnum(repo, ctx): def getdiffnum(repo, ctx):
return diffprops.parserevfromcommitmsg(ctx.description()) return diffprops.parserevfromcommitmsg(ctx.description())
def _getdag(orig, *args): class PeekaheadRevsetIter(object):
repo = args[1] """
# We retain the smartlogrevision, this way showphabstatus knows that there PeekaheadRevsetIter is a helper class that wraps a revision set iterator,
# are multiple revisions to resolve and allows the phabstatus code to peek ahead in the list as the logging
repo._smartlogrevs = args[2] code is iterating through it.
return orig(*args)
The main logging code uses the normal iterator interface (next()) to
iterate through this revision set.
The phabstatus code will call peekahead() to peek ahead in the list, so it
can query information for multiple revisions at once, rather than only
processing them one at a time as the logging code requests them.
"""
def __init__(self, revs, chunksize=30):
self.mainiter = iter(revs)
# done is set to true once mainiter has thrown StopIteration
self.done = False
# chunk is the peekahead chunk we have returned from peekahead().
self.chunk = list()
# chunk_idx represents how far into self.chunk() the main iteration
# code has seen via the next() API.
self.chunk_idx = 0
self.chunksize = chunksize
def next(self):
if self.chunk_idx < len(self.chunk):
# We still have data remaining in the peekahead chunk to return
result = self.chunk[self.chunk_idx]
self.chunk_idx += 1
if self.chunk_idx >= len(self.chunk):
self.chunk = list()
self.chunk_idx = 0
return result
if self.done:
raise StopIteration()
try:
return next(self.mainiter)
except StopIteration:
self.done = True
raise
def peekahead(self, chunksize=None):
chunksize = chunksize or self.chunksize
while len(self.chunk) < chunksize and not self.done:
try:
self.chunk.append(next(self.mainiter))
except StopIteration:
self.done = True
return self.chunk
def _getlogrevs(orig, repo, pats, opts):
# Call the original function
revs, expr, filematcher = orig(repo, pats, opts)
# Wrap the revs result so that iter(revs) returns a PeekaheadRevsetIter()
# the first time it is invoked, and sets repo._phabstatusrevs so that the
# phabstatus code will be able to peek ahead at the revs to be logged.
orig_type = revs.__class__
class wrapped_class(type(revs)):
def __iter__(self):
# The first time __iter__() is called, return a
# PeekaheadRevsetIter(), and assign it to repo._phabstatusrevs
revs.__class__ = orig_type
# By default, peek ahead 30 revisions at a time
peekahead = repo.ui.configint('phabstatus', 'logpeekahead', 30)
repo._phabstatusrevs = PeekaheadRevsetIter(revs, peekahead)
return repo._phabstatusrevs
_is_phabstatus_wrapped = True
if not hgutil.safehasattr(revs, '_is_phabstatus_wrapped'):
revs.__class__ = wrapped_class
return revs, expr, filematcher
class PeekaheadList(object):
"""
PeekaheadList exposes peekahead() and done just like PeekaheadRevsetIter,
but wraps a simple list instead of a revset generator. peekahead() returns
the full list.
"""
def __init__(self, revs):
self.revs = revs
self.done = False
def peekahead(self):
self.done = True
return self.revs
def _getsmartlogdag(orig, ui, repo, revs, *args):
# smartlog just uses a plain list for its revisions, and not an
# abstractsmartset type. We just save a copy of it.
repo._phabstatusrevs = PeekaheadList(revs)
return orig(ui, repo, revs, *args)
def extsetup(ui): def extsetup(ui):
# Wrap the APIs used to get the revisions for "hg log" so we
# can peekahead into the rev list and query phabricator for multiple diffs
# at once.
extensions.wrapfunction(cmdutil, 'getlogrevs', _getlogrevs)
extensions.wrapfunction(cmdutil, 'getgraphlogrevs', _getlogrevs)
# Also wrap the APIs used by smartlog
def _smartlogloaded(loaded): def _smartlogloaded(loaded):
smartlog = None smartlog = None
try: try:
smartlog = extensions.find('smartlog') smartlog = extensions.find('smartlog')
except KeyError: except KeyError:
pass pass
if smartlog: if smartlog:
extensions.wrapfunction(smartlog, 'getdag', _getdag) extensions.wrapfunction(smartlog, 'getdag', _getsmartlogdag)
extensions.afterloaded('smartlog', _smartlogloaded) extensions.afterloaded('smartlog', _smartlogloaded)

tests/test-check-config-hg.t

undocumented: grep.command (str) ['grep'] undocumented: grep.command (str) ['grep']
undocumented: morestatus.show (bool) undocumented: morestatus.show (bool)
undocumented: nointerrupt.interactiveonly (bool) [True] undocumented: nointerrupt.interactiveonly (bool) [True]
undocumented: perftweaks.cachenoderevs (bool) [True] undocumented: perftweaks.cachenoderevs (bool) [True]
undocumented: perftweaks.disablebranchcache (bool) undocumented: perftweaks.disablebranchcache (bool)
undocumented: perftweaks.disablecasecheck (bool) undocumented: perftweaks.disablecasecheck (bool)
undocumented: perftweaks.disabletags (bool) undocumented: perftweaks.disabletags (bool)
undocumented: perftweaks.preferdeltas (bool) undocumented: perftweaks.preferdeltas (bool)
undocumented: phabstatus.logpeekahead (int) [30]
undocumented: phrevset.callsign (str) undocumented: phrevset.callsign (str)
undocumented: pushrebase.blocknonpushrebase (bool) undocumented: pushrebase.blocknonpushrebase (bool)
undocumented: pushrebase.rewritedates (bool) undocumented: pushrebase.rewritedates (bool)
undocumented: rage.fastmanifestcached (bool) undocumented: rage.fastmanifestcached (bool)
undocumented: remotefilelog.backgroundrepack (bool) undocumented: remotefilelog.backgroundrepack (bool)
undocumented: remotefilelog.cachegroup (str) undocumented: remotefilelog.cachegroup (str)
undocumented: remotefilelog.data.gencountlimit (int) [2] undocumented: remotefilelog.data.gencountlimit (int) [2]
undocumented: remotefilelog.debug (bool) undocumented: remotefilelog.debug (bool)