This is an archive of the discontinued Mercurial Phabricator instance.

Differential D169

perftweaks: add logging for rebase size
ClosedPublic

Authored by durham on Jul 21 2017, 9:00 AM.

Download Raw Diff

Details

Reviewers

quark

Group Reviewers

Restricted Project

Commits

rFBHGX66513c593eb6: perftweaks: add logging for rebase size

Summary

Let's add logging for the size of rebases (the number of commits being rebased
and the distance over which they are rebased).

Test Plan

Ran a rebase and saw the data show up in ptail

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

durham created this revision.Jul 21 2017, 9:00 AM

I think it would also be interesting to log if we had any conflicts. Of course not in this diff.

hgext3rd/perftweaks.py
28–30	It could be recharge-related brain-rot, but I vaguely remember something like this not working. Don't you have to wrap the actually-included rebase via extensions.find rather than importing it? Or am I remembering some old or non-existent failure case?
45	This will only log when the rebase is started, right? Do we also want to include this info in a rebase --continue, for example?

durham added inline comments.Jul 21 2017, 11:54 AM

hgext3rd/perftweaks.py
45	I only wanted the initial rebase for now, and we can filter out failures by looking error codes. That will let us get fair comparisons of rebases (by excluding conflicts and continues and failures).

Use extensions.find

This will have issue with upcoming multi-dest changes. Do we really care about the precise distance? If not, I think we can just subtract rev numbers, like
abs(destrev - min(rebaseset))

If we can get the precise number, we might as well use it. What about the multi-dest change makes calculating the distance between the destination and the source difficult?

Consider rebase A to B, B to C, rebase A to C, B to C. They should have similar (+/-1) distance. How would you define distance for it?

I think what we want is the sum of update distance, like when merge.udpate gets executed, how far is it away from the current wd parent. That seems to be a general purpose thing that applies to every command.

Rejecting since I'd love us to go the more general-purposed approach.

This revision now requires changes to proceed.Jul 21 2017, 4:58 PM

Ah, I just noticed D166 does that. Is the rebase change still necessary?

We also want to track how many commits are being rebased, which D166 doesn't cover. Also, with inmemory merge, the update distance won't necessarily reflect the rebase distance. Is the multi-dest stuff landed already? If so, what are the input revsets (like here we have dest and rebaseset), so I can construct an appropriate one for multi-target rebases. If that stuff is not landed yet, I'd like to go ahead and land this.

perftweaks.py is no longer just a perftweaks. I think it makes sense to split this extension in the future

In D169#3132, @durham wrote:

We also want to track how many commits are being rebased, which D166 doesn't cover.

I see. commitcount should work with multi-dest too.

Also, with inmemory merge, the update distance won't necessarily reflect the rebase distance.

My main concern is the rebase distance could not be calculated accurately in an easy way. Even without multi-dest, the current rebase allows -r arbitrary_revset. Something like -r draft() -d master with multiple feature branches could give controversial results. Merge is also a problem. I saw there is a comment saying it could be inaccurate. I'll accept the diff.

Is the multi-dest stuff landed already?

No. Blocked by D21.

stash added inline comments.Jul 31 2017, 1:20 PM

hgext3rd/perftweaks.py
296–300	Just curious: is there a reason you are using two `repo.ui.log(...)` instead of 1?

quark accepted this revision.Jul 31 2017, 1:21 PM

This revision is now accepted and ready to land.Jul 31 2017, 1:21 PM

quark added inline comments.Jul 31 2017, 1:23 PM

hgext3rd/perftweaks.py
297–299	Maybe make it more clear about order cases: # The code assumes rebase source is a linear stack within a single feature branch, # and there is only one destination. If that is not the case, the distance might # be not accurate.

durham added inline comments.Jul 31 2017, 1:31 PM

hgext3rd/perftweaks.py
296–300	No reason. Just an artifact of how it was developed. I'll combine them.

Closed by commit rFBHGX66513c593eb6: perftweaks: add logging for rebase size (authored by durham). · Explain WhyJul 31 2017, 1:58 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext3rd/perftweaks.py (26 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	362		Jul 21 2017, 9:00 AM	★	★
Diff 2	376	Use extensions.find	Jul 21 2017, 12:02 PM	★	★
Diff 3	458	rFBHGX66513c593eb69af03b586f8709bd3f02d7f7d675	Jul 31 2017, 1:57 PM	★	★

Diff 458

hgext3rd/perftweaks.py

	extensions,			extensions,
	merge,			merge,
	phases,			phases,
	revlog,			revlog,
	scmutil,			scmutil,
	tags,			tags,
	util,			util,
	)			)
	from mercurial.extensions import wrapfunction			from mercurial.extensions import wrapfunction
	from mercurial.node import bin, nullid, nullrev			from mercurial.node import bin, nullid, nullrev
	import errno			import errno
				ryanmceUnsubmitted Not Done It could be recharge-related brain-rot, but I vaguely remember something like this not working. Don't you have to wrap the actually-included rebase via extensions.find rather than importing it? Or am I remembering some old or non-existent failure case? ryanmce: It could be recharge-related brain-rot, but I vaguely remember something like this not working.
	import os			import os

	testedwith = 'ships-with-fb-hgext'			testedwith = 'ships-with-fb-hgext'

	def extsetup(ui):			def extsetup(ui):
	wrapfunction(tags, '_readtagcache', _readtagcache)			wrapfunction(tags, '_readtagcache', _readtagcache)
	wrapfunction(merge, '_checkcollision', _checkcollision)			wrapfunction(merge, '_checkcollision', _checkcollision)
	wrapfunction(branchmap.branchcache, 'update', _branchmapupdate)			wrapfunction(branchmap.branchcache, 'update', _branchmapupdate)
	if ui.configbool('perftweaks', 'preferdeltas'):			if ui.configbool('perftweaks', 'preferdeltas'):
	wrapfunction(revlog.revlog, '_isgooddelta', _isgooddelta)			wrapfunction(revlog.revlog, '_isgooddelta', _isgooddelta)

	wrapfunction(dispatch, 'runcommand', _trackdirstatesizes)			wrapfunction(dispatch, 'runcommand', _trackdirstatesizes)
	wrapfunction(dispatch, 'runcommand', _tracksparseprofiles)			wrapfunction(dispatch, 'runcommand', _tracksparseprofiles)
	wrapfunction(merge, 'update', _trackupdatesize)			wrapfunction(merge, 'update', _trackupdatesize)

				ryanmceUnsubmitted Not Done This will only log when the rebase is started, right? Do we also want to include this info in a rebase --continue, for example? ryanmce: This will only log when the rebase is started, right? Do we also want to include this info in a…
				durhamAuthorUnsubmitted Not Done I only wanted the initial rebase for now, and we can filter out failures by looking error codes. That will let us get fair comparisons of rebases (by excluding conflicts and continues and failures). durham: I only wanted the initial rebase for now, and we can filter out failures by looking error codes.
				try:
				rebase = extensions.find('rebase')
				wrapfunction(rebase.rebaseruntime, '_preparenewrebase',
				_trackrebasesize)
				except KeyError:
				pass

	# noderev cache creation			# noderev cache creation
	# The node rev cache is a cache of rev numbers that we are likely to do a			# The node rev cache is a cache of rev numbers that we are likely to do a
	# node->rev lookup for. Since looking up rev->node is cheaper than			# node->rev lookup for. Since looking up rev->node is cheaper than
	# node->rev, we use this cache to prefill the changelog radix tree with			# node->rev, we use this cache to prefill the changelog radix tree with
	# mappings.			# mappings.
	wrapfunction(branchmap.branchcache, 'write', _branchmapwrite)			wrapfunction(branchmap.branchcache, 'write', _branchmapwrite)
	wrapfunction(phases.phasecache, 'advanceboundary', _editphases)			wrapfunction(phases.phasecache, 'advanceboundary', _editphases)
	wrapfunction(phases.phasecache, 'retractboundary', _editphases)			wrapfunction(phases.phasecache, 'retractboundary', _editphases)
	def _trackupdatesize(orig, repo, node, branchmerge, args, *kwargs):			def _trackupdatesize(orig, repo, node, branchmerge, args, *kwargs):
	if not branchmerge:			if not branchmerge:
	distance = len(repo.revs('(%s %% .) + (. %% %s)', node, node))			distance = len(repo.revs('(%s %% .) + (. %% %s)', node, node))
	repo.ui.log('update_size', '', update_distance=distance)			repo.ui.log('update_size', '', update_distance=distance)

	stats = orig(repo, node, branchmerge, args, *kwargs)			stats = orig(repo, node, branchmerge, args, *kwargs)
	repo.ui.log('update_size', '', update_filecount=sum(stats))			repo.ui.log('update_size', '', update_filecount=sum(stats))
	return stats			return stats

				def _trackrebasesize(orig, self, dest, rebaseset):
				result = orig(self, dest, rebaseset)

				# The code assumes the rebase source is roughly a linear stack within a
				# single feature branch, and there is only one destination. If that is not
				# the case, the distance might be not accurate.
				repo = self.repo
				destrev = dest.rev()
				commitcount = len(rebaseset)
				distance = len(repo.revs('(%ld %% %d) + (%d %% %ld)',
				rebaseset, destrev, destrev, rebaseset))
				# 'distance' includes the commits being rebased, so subtract them to get the
				quarkUnsubmitted Not Done Maybe make it more clear about order cases: # The code assumes rebase source is a linear stack within a single feature branch, # and there is only one destination. If that is not the case, the distance might # be not accurate. quark: Maybe make it more clear about order cases: ``` # The code assumes rebase source is a linear…
				# actual distance being traveled. Even though we log update_distance above,
				stashUnsubmitted Not Done Just curious: is there a reason you are using two `repo.ui.log(...)` instead of 1? stash: Just curious: is there a reason you are using two `repo.ui.log(...)` instead of 1?
				durhamAuthorUnsubmitted Not Done No reason. Just an artifact of how it was developed. I'll combine them. durham: No reason. Just an artifact of how it was developed. I'll combine them.
				# a rebase may run multiple updates, so that value might be not be accurate.
				repo.ui.log('rebase_size', '', rebase_commitcount=commitcount,
				rebase_distance=distance - commitcount)

				return result