This is an archive of the discontinued Mercurial Phabricator instance.

phabricator: avoid calling differential.getrawdiff
AbandonedPublic

Authored by quark on Jul 11 2017, 11:56 AM.

Download Raw Diff

Details

Reviewers

Summary

Previously, we call differential.getrawdiff per patch despite we have the
diff content already (returned by differential.querydiffs).

This patch implements the serialization logic of differential.getrawdiff
client-side so we no longer need calling differential.getrawdiff. This
removes one API call per patch.

Now fetching a stack of 10 patches is just 2 API calls in the best case.
Worst case batching does not work and it's 11 API calls. The "reading"
verbose message was removed since there is no remote IO needed to read a
patch in that loop now.

Using a test Phabricator instance, this patch reduces phabread reading a
10-patch stack from about 8 seconds to 1.3 seconds. Even with batching
disabled, reading that 10-patch stack could be done in 4 seconds. The
copy-paste constants are a bit unfortunate since "code is documentation" is
true for many Phabricator features. But the perf win could probably justify
it.

Test Plan

Create an interesting (contains multiple copies and renames, mixed mode
change with content changes) patch using the below script:

hg init

# long context could be more interesting
seq 1 200 > long
for i in a b c d e f g; do
  echo $i > $i
done
hg add .
hg commit -m init

sed -i '/10/d;s/22$/22.1\n22.2/g' long

# multiple copies and moves
for i in a b; do
  for j in 1 2 3 4; do
    hg cp $i "${i}${j}"
    [ $((j % 2)) = 0 ] && chmod +x "${i}${j}"
    [ $j -ge 3 ] && echo append >> "${i}${j}"
  done
done

hg rm a
chmod +x c
hg rm d
hg mv e E
chmod +x E
rm f
touch f
hg commit -m change

Send it using phabsend, and retrieve it using phabread. Compare before
and after this patch. The only differences are:

This patch removes a blank line after a patch.
This patch does not omit ,1 in diff hunks. See [1].

Other than that, the patch output is exactly the same. So the new code path
looks reasonably good.

Also try using phabread to read all existing diffs sent to the website and
make sure it works.

[1]: https://secure.phabricator.com/diffusion/ARC/browse/master/src/parser/
ArcanistBundle.php;e00e2939c164df85852ce2d157de4197b399c30b$631

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

quark created this revision.Jul 11 2017, 11:56 AM

Herald added a subscriber: mercurial-devel. · View Herald TranscriptJul 11 2017, 11:56 AM

quark added a child revision: D38: phabricator: allow specifying reviewers on phabsend.Jul 11 2017, 11:56 AM

quark edited the test plan for this revision. (Show Details)Jul 11 2017, 12:22 PM

I'm honestly not comfortable with this, after reflection. Phabricator should document their constants, and honestly it feels really nasty to re-synthesize diffs when phabricator can already hand them to us.

If the time to fetch a stack is not a concern, I can drop this.

In D37#1094, @quark wrote:

If the time to fetch a stack is not a concern, I can drop this.

I'm not worried about it for now. Speed of hg phabread is nowhere near even being a pain point for me (the web UI is a much bigger issue)

quark abandoned this revision.Jul 14 2017, 4:34 PM

Revision Contents
Changeset List

			Path	Packages
M			contrib/phabricator.py (97 lines)

Status	Author	Revision
Closed	quark	D38 phabricator: allow specifying reviewers on phabsend
Abandoned	quark	D37 phabricator: avoid calling differential.getrawdiff
Closed	quark	D36 phabricator: verify local tags before trusting them
Closed	quark	D35 phabricator: sanity check Differential Revision from commit message
Closed	quark	D34 phabricator: finding old nodes in batch
Closed	quark	D33 phabricator: respect metadata sent by arc

Diff 53

contrib/phabricator.py

	r'date': r'%d 0' % commit[r'time'],			r'date': r'%d 0' % commit[r'time'],
	r'node': commit[r'rev'],			r'node': commit[r'rev'],
	r'user': r'%s <%s>' % (commit[r'author'], commit[r'authorEmail']),			r'user': r'%s <%s>' % (commit[r'author'], commit[r'authorEmail']),
	}			}
	if len(commit.get(r'parents', ())) >= 1:			if len(commit.get(r'parents', ())) >= 1:
	meta[r'parent'] = commit[r'parents'][0]			meta[r'parent'] = commit[r'parents'][0]
	return meta			return meta

				# The constants are from https://secure.phabricator.com/diffusion/ARC/
				# browse/master/src/parser/diff/ArcanistDiffChangeType.php
				# They are very stable - haven't been changed since Jan 2011, the first commit
				# of the arcanist project.
				class changetype(object):
				ADD = 1
				CHANGE = 2
				DELETE = 3
				MOVE_AWAY = 4
				COPY_AWAY = 5
				MOVE_HERE = 6
				COPY_HERE = 7
				MULTICOPY = 8

				class filetype(object):
				TEXT = 1

				def getpatchbodyfromdiff(diff):
				"""get patch body (without commit message or metadata) from a diff dict

				This is similar to differential.getrawdiff API. But we reconstruct the diff
				from a diff object fetched earlier via differential.querydiffs API.

				Currently it does not support binary files. However it seems the
				"differential.createrawdiff" API (used by phabsend) couldn't really handle
				base85 binaries so binary files are broken at uploading time, therefore
				this limitation is probably fine.
				"""
				out = util.stringio()
				write = out.write

				for c in diff[r'changes']:
				path = encoding.unitolocal(c[r'currentPath'] or u'')
				oldpath = encoding.unitolocal(c[r'oldPath'] or u'')
				patha = 'a/%s' % (oldpath or path)
				pathb = 'b/%s' % (path or oldpath)

				ftype = int(c[r'fileType'])
				if ftype != filetype.TEXT:
				raise error.Abort(_('unsupported file type %s: %s') % (ftype, path))

				ctype = int(c[r'type'])
				if ctype in [changetype.MULTICOPY, changetype.MOVE_AWAY,
				changetype.COPY_AWAY]:
				# ignore these, COPY_HERE or MOVE_HERE will cover them
				continue

				def getmode(name):
				s = (c[name] or {}).get(r'unix:filemode', r'100644')
				return encoding.unitolocal(s)

				oldmode = getmode(r'oldProperties')
				newmode = getmode(r'newProperties')

				write('diff --git %s %s\n' % (patha, pathb))
				if ctype == changetype.ADD:
				write('new file mode %s\n' % newmode)
				patha = '/dev/null'
				elif ctype == changetype.DELETE:
				write('deleted file mode %s\n' % oldmode)
				pathb = '/dev/null'
				else:
				if oldmode != newmode:
				write('old mode %s\nnew mode %s\n' % (oldmode, newmode))
				if ctype == changetype.CHANGE:
				pass
				elif ctype == changetype.MOVE_HERE:
				write('rename from %s\nrename to %s\n' % (oldpath, path))
				elif ctype == changetype.COPY_HERE:
				write('copy from %s\ncopy to %s\n' % (oldpath, path))
				else:
				raise error.Abort(_('unsupported change type %s: %s')
				% (ctype, path))

				if c[r'hunks']:
				write('--- %s\n+++ %s\n' % (patha, pathb))
				for h in c[r'hunks']:
				oldoff = int(h[r'oldOffset'])
				oldlen = int(h[r'oldLength'])
				newoff = int(h[r'newOffset'])
				newlen = int(h[r'newLength'])
				write('@@ -%d,%d +%d,%d @@\n' % (oldoff, oldlen, newoff, newlen))
				write(encoding.unitolocal(h[r'corpus']))

				# normalize the patch by trimming context to 3 lines
				headers = patch.parsepatch(out.getvalue(), maxcontext=3)
				out = util.stringio()
				for header in headers:
				header.write(out)
				for hunk in header.hunks:
				hunk.write(out)
				return out.getvalue()

	def readpatch(repo, params, write, stack=False):			def readpatch(repo, params, write, stack=False):
	"""generate plain-text patch readable by 'hg import'			"""generate plain-text patch readable by 'hg import'

	write is usually ui.write. params is passed to "differential.query". If			write is usually ui.write. params is passed to "differential.query". If
	stack is True, also write dependent patches.			stack is True, also write dependent patches.
	"""			"""
	# Differential Revisions			# Differential Revisions
	drevs = querydrev(repo, params, stack)			drevs = querydrev(repo, params, stack)

	# Prefetch hg:meta property for all diffs			# Prefetch hg:meta property for all diffs
	diffids = sorted(set(max(int(v) for v in drev[r'diffs']) for drev in drevs))			diffids = sorted(set(max(int(v) for v in drev[r'diffs']) for drev in drevs))
	diffs = callconduit(repo, 'differential.querydiffs', {'ids': diffids})			diffs = callconduit(repo, 'differential.querydiffs', {'ids': diffids})

	# Generate patch for each drev			# Generate patch for each drev
	for drev in drevs:			for drev in drevs:
	repo.ui.note(_('reading D%s\n') % drev[r'id'])

	diffid = max(int(v) for v in drev[r'diffs'])			diffid = max(int(v) for v in drev[r'diffs'])
	body = callconduit(repo, 'differential.getrawdiff', {'diffID': diffid})			body = getpatchbodyfromdiff(diffs[str(diffid)])
	desc = getdescfromdrev(drev)			desc = getdescfromdrev(drev)
	header = '# HG changeset patch\n'			header = '# HG changeset patch\n'

	# Try to preserve metadata from hg:meta property. Write hg patch			# Try to preserve metadata from hg:meta property. Write hg patch
	# headers that can be read by the "import" command. See patchheadermap			# headers that can be read by the "import" command. See patchheadermap
	# and extract in mercurial/patch.py for supported headers.			# and extract in mercurial/patch.py for supported headers.
	meta = getdiffmeta(diffs[str(diffid)])			meta = getdiffmeta(diffs[str(diffid)])
	for k in _metanamemap.keys():			for k in _metanamemap.keys():

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	53			Jul 11 2017, 11:56 AM	★	★