( )⚙ D1227 repack: always use all history packs for ancestry data

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1227

repack: always use all history packs for ancestry data
ClosedPublic

Authored by phillco on Oct 25 2017, 4:49 PM.
Tags
None
Subscribers
None

Details

Reviewers
durham
Group Reviewers
Restricted Project
Commits
rFBHGX57e3e76b273a: repack: always use all history packs for ancestry data
Summary

When repacking data, we sort data nodes topologically by ancestry in order to
ensure the best (smallest) delta chain. Unfortunately the history we use to do
this will be whatever history packs the samre repack job chose for its history
repacking portion, which might be comically small and/or irrelevant.

To fix this, select all history packfiles, and pass them to the data packer as
fullhistory. Print a debug warning whenever any nodes are missing ancestry.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

phillco created this revision.Oct 25 2017, 4:49 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 25 2017, 4:49 PM
phillco planned changes to this revision.Oct 25 2017, 4:54 PM

Forgot to patch the full-repack entry point. Everything else can be reviewed, though.

phillco requested review of this revision.Oct 25 2017, 5:00 PM

Oh wait, I forgot we only need to do this in the case of an incremental repack.

phillco edited the summary of this revision. (Show Details)EditedOct 25 2017, 5:01 PM
This comment has been deleted.
tests/test-treemanifest-repack.t
209–222

Sigh, these break check-code. Any suggestions?

phillco edited the summary of this revision. (Show Details)Oct 25 2017, 5:15 PM
phillco updated this revision to Diff 3086.
phillco updated this revision to Diff 3087.Oct 25 2017, 5:19 PM
phillco updated this revision to Diff 3088.
phillco added a reviewer: durham.Oct 25 2017, 10:13 PM
phillco added inline comments.
tests/test-treemanifest-repack.t
195–208

Sigh, these break check-code. Any suggestions?

durham edited the summary of this revision. (Show Details)Oct 26 2017, 3:54 PM
durham accepted this revision.Oct 26 2017, 4:04 PM

Some comments might warrant a change, but looks good enough to accept.

remotefilelog/repack.py
148

Might be worthy of just being a top level function.

155

Maybe add a comment explaining the difference between historypacks and allhistorypacks, just for easy scanning of what this code does.

230

The first parameter of the tuple could be just prefix right?

tests/test-treemanifest-repack.t
173

s/except/expect/

175

Does this xargs work on both osx and linux? I'm always wary of bashisms in the tests

195–208

Why is there so much white space in the output?

This revision is now accepted and ready to land.Oct 26 2017, 4:04 PM
phillco added inline comments.Oct 26 2017, 4:29 PM
tests/test-treemanifest-repack.t
195–208

I think debugdatapack prints extra lines in some cases.

durham added inline comments.Nov 6 2017, 2:21 PM
tests/test-treemanifest-repack.t
195–208

Really? I don't think check code would complain if these were part of test output. If they were test output I'd expect them to have 2 spaces at the front, which they don't appear to (at least from what highlighting with my mouse says).

phillco added inline comments.Nov 6 2017, 9:35 PM
tests/test-treemanifest-repack.t
195–208

ah, turns out my editor was also stripping them. In any case I made hgdebugdatapack take multiple files, which I'll send as a followup.

phillco edited the summary of this revision. (Show Details)Nov 6 2017, 9:44 PM
phillco updated this revision to Diff 3303.
phillco updated this revision to Diff 3305.Nov 6 2017, 9:47 PM
phillco marked 3 inline comments as done.Nov 6 2017, 10:16 PM
phillco updated this revision to Diff 3306.
phillco closed this revision.Nov 6 2017, 10:18 PM

Landed

Revision Contents
Changeset List

Diff 3088

remotefilelog/repack.py

(sharedpackpath, shareddatastores, sharedhistorystores)) (sharedpackpath, shareddatastores, sharedhistorystores))
def _incrementalrepack(repo, datastore, historystore, packpath, category, def _incrementalrepack(repo, datastore, historystore, packpath, category,
allowincompletedata=False): allowincompletedata=False):
shallowutil.mkstickygroupdir(repo.ui, packpath) shallowutil.mkstickygroupdir(repo.ui, packpath)
files = osutil.listdir(packpath, stat=True) files = osutil.listdir(packpath, stat=True)
datapacks = _computeincrementaldatapack(repo.ui, files) def topacks(files, constructor):
fullpaths = list(os.path.join(packpath, p) for p in datapacks) paths = list(os.path.join(packpath, p) for p in files)
datapacks = list(datapack.datapack(p) for p in fullpaths) packs = list(constructor(p) for p in paths)
return packs
durhamUnsubmitted
Done

Might be worthy of just being a top level function.

durham: Might be worthy of just being a top level function.
datapacks = topacks(_computeincrementaldatapack(repo.ui, files),
datapack.datapack)
datapacks.extend(s for s in datastore datapacks.extend(s for s in datastore
if not isinstance(s, datapack.datapackstore)) if not isinstance(s, datapack.datapackstore))
historypacks = _computeincrementalhistorypack(repo.ui, files) historypacks = topacks(_computeincrementalhistorypack(repo.ui, files),
durhamUnsubmitted
Done

Maybe add a comment explaining the difference between historypacks and allhistorypacks, just for easy scanning of what this code does.

durham: Maybe add a comment explaining the difference between historypacks and allhistorypacks, just…
fullpaths = list(os.path.join(packpath, p) for p in historypacks) historypack.historypack)
historypacks = list(historypack.historypack(p) for p in fullpaths)
historypacks.extend(s for s in historystore historypacks.extend(s for s in historystore
if not isinstance(s, historypack.historypackstore)) if not isinstance(s, historypack.historypackstore))
datasource = contentstore.unioncontentstore( allhistoryfiles = _allpackfileswithsuffix(files, historypack.PACKSUFFIX,
historypack.INDEXSUFFIX)
allhistorypacks = topacks((f for f, mode, stat in allhistoryfiles),
historypack.historypack)
allhistorypacks.extend(s for s in historystore
if not isinstance(s, historypack.historypackstore))
_runrepack(repo,
contentstore.unioncontentstore(
*datapacks, *datapacks,
allowincomplete=allowincompletedata) allowincomplete=allowincompletedata),
historysource = metadatastore.unionmetadatastore(*historypacks, metadatastore.unionmetadatastore(
allowincomplete=True) *historypacks,
allowincomplete=True),
_runrepack(repo, datasource, historysource, packpath, category) packpath, category,
fullhistory=metadatastore.unionmetadatastore(
*allhistorypacks,
allowincomplete=True))
def _computeincrementaldatapack(ui, files): def _computeincrementaldatapack(ui, files):
"""Given a set of pack files and a set of generation size limits, this """Given a set of pack files and a set of generation size limits, this
function computes the list of files that should be packed as part of an function computes the list of files that should be packed as part of an
incremental repack. incremental repack.
It tries to strike a balance between keeping incremental repacks cheap (i.e. It tries to strike a balance between keeping incremental repacks cheap (i.e.
packing small things when possible, and rolling the packs up to the big ones packing small things when possible, and rolling the packs up to the big ones
repacksizelimit = ui.configbytes('remotefilelog', 'history.repacksizelimit', repacksizelimit = ui.configbytes('remotefilelog', 'history.repacksizelimit',
'100MB') '100MB')
maxrepackpacks = ui.configint('remotefilelog', 'history.maxrepackpacks', 50) maxrepackpacks = ui.configint('remotefilelog', 'history.maxrepackpacks', 50)
return _computeincrementalpack(ui, files, generations, return _computeincrementalpack(ui, files, generations,
historypack.PACKSUFFIX, historypack.INDEXSUFFIX, gencountlimit, historypack.PACKSUFFIX, historypack.INDEXSUFFIX, gencountlimit,
repacksizelimit, maxrepackpacks) repacksizelimit, maxrepackpacks)
def _computeincrementalpack(ui, files, limits, packsuffix, indexsuffix, def _allpackfileswithsuffix(files, packsuffix, indexsuffix):
gencountlimit, repacksizelimit, maxrepackpacks): result = []
# Group the packs by generation (i.e. by size)
generations = []
for i in xrange(len(limits)):
generations.append([])
sizes = {}
fileset = set(fn for fn, mode, stat in files) fileset = set(fn for fn, mode, stat in files)
for filename, mode, stat in files: for filename, mode, stat in files:
if not filename.endswith(packsuffix): if not filename.endswith(packsuffix):
continue continue
prefix = filename[:-len(packsuffix)] prefix = filename[:-len(packsuffix)]
# Don't process a pack if it doesn't have an index. # Don't process a pack if it doesn't have an index.
if (prefix + indexsuffix) not in fileset: if (prefix + indexsuffix) not in fileset:
continue continue
result.append((filename[:-len(packsuffix)], mode, stat))
durhamUnsubmitted
Done

The first parameter of the tuple could be just prefix right?

durham: The first parameter of the tuple could be just `prefix` right?
return result
def _computeincrementalpack(ui, files, limits, packsuffix, indexsuffix,
gencountlimit, repacksizelimit, maxrepackpacks):
# Group the packs by generation (i.e. by size)
generations = []
for i in xrange(len(limits)):
generations.append([])
sizes = {}
for prefix, mode, stat in _allpackfileswithsuffix(files, packsuffix,
indexsuffix):
size = stat.st_size size = stat.st_size
sizes[prefix] = size sizes[prefix] = size
for i, limit in enumerate(limits): for i, limit in enumerate(limits):
if size > limit: if size > limit:
generations[i].append(prefix) generations[i].append(prefix)
break break
# Steps for picking what packs to repack: # Steps for picking what packs to repack:
# If there aren't any good candidates for a repack, # If there aren't any good candidates for a repack,
# repack the two largest ones. # repack the two largest ones.
if not chosenpacks and len(generations[0]) > 1: if not chosenpacks and len(generations[0]) > 1:
chosenpacks = generations[0] chosenpacks = generations[0]
return chosenpacks return chosenpacks
def _runrepack(repo, data, history, packpath, category): def _runrepack(repo, data, history, packpath, category, fullhistory=None):
shallowutil.mkstickygroupdir(repo.ui, packpath) shallowutil.mkstickygroupdir(repo.ui, packpath)
def isold(repo, filename, node): def isold(repo, filename, node):
"""Check if the file node is older than a limit. """Check if the file node is older than a limit.
Unless a limit is specified in the config the default limit is taken. Unless a limit is specified in the config the default limit is taken.
""" """
filectx = repo.filectx(filename, fileid=node) filectx = repo.filectx(filename, fileid=node)
filetime = repo[filectx.linkrev()].date() filetime = repo[filectx.linkrev()].date()
# Currently default TTL limit is 30 days # Currently default TTL limit is 30 days
defaultlimit = 60 * 60 * 24 * 30 defaultlimit = 60 * 60 * 24 * 30
ttl = repo.ui.configint('remotefilelog', 'nodettl', defaultlimit) ttl = repo.ui.configint('remotefilelog', 'nodettl', defaultlimit)
limit = time.time() - ttl limit = time.time() - ttl
return filetime[0] < limit return filetime[0] < limit
garbagecollect = repo.ui.configbool('remotefilelog', 'gcrepack') garbagecollect = repo.ui.configbool('remotefilelog', 'gcrepack')
packer = repacker(repo, data, history, category, gc=garbagecollect, if not fullhistory:
fullhistory = history
packer = repacker(repo, data, history, fullhistory, category,
gc=garbagecollect,
isold=isold) isold=isold)
# internal config: remotefilelog.datapackversion # internal config: remotefilelog.datapackversion
dv = repo.ui.configint('remotefilelog', 'datapackversion', 0) dv = repo.ui.configint('remotefilelog', 'datapackversion', 0)
with datapack.mutabledatapack(repo.ui, packpath, version=dv) as dpack: with datapack.mutabledatapack(repo.ui, packpath, version=dv) as dpack:
with historypack.mutablehistorypack(repo.ui, packpath) as hpack: with historypack.mutablehistorypack(repo.ui, packpath) as hpack:
try: try:
keepkeys.add(keyfn(filename, filenode)) keepkeys.add(keyfn(filename, filenode))
return keepkeys return keepkeys
class repacker(object): class repacker(object):
"""Class for orchestrating the repack of data and history information into a """Class for orchestrating the repack of data and history information into a
new format. new format.
""" """
def __init__(self, repo, data, history, category, gc=False, isold=None): def __init__(self, repo, data, history, fullhistory, category, gc=False,
isold=None):
self.repo = repo self.repo = repo
self.data = data self.data = data
self.history = history self.history = history
self.fullhistory = fullhistory
self.unit = constants.getunits(category) self.unit = constants.getunits(category)
self.garbagecollect = gc self.garbagecollect = gc
if self.garbagecollect: if self.garbagecollect:
if not isold: if not isold:
raise ValueError("Function 'isold' is not properly specified") raise ValueError("Function 'isold' is not properly specified")
# use (filename, node) tuple as a keepset key # use (filename, node) tuple as a keepset key
self.keepkeys = keepset(repo, lambda f, n : (f, n)) self.keepkeys = keepset(repo, lambda f, n : (f, n))
self.isold = isold self.isold = isold
nodes = list(node for node in entries.iterkeys()) nodes = list(node for node in entries.iterkeys())
nohistory = [] nohistory = []
for i, node in enumerate(nodes): for i, node in enumerate(nodes):
if node in ancestors: if node in ancestors:
continue continue
ui.progress(_("building history"), i, unit='nodes', ui.progress(_("building history"), i, unit='nodes',
total=len(nodes)) total=len(nodes))
try: try:
ancestors.update(self.history.getancestors(filename, node, ancestors.update(self.fullhistory.getancestors(filename,
known=ancestors)) node, known=ancestors))
except KeyError: except KeyError:
# Since we're packing data entries, we may not have the # Since we're packing data entries, we may not have the
# corresponding history entries for them. It's not a big # corresponding history entries for them. It's not a big
# deal, but the entries won't be delta'd perfectly. # deal, but the entries won't be delta'd perfectly.
nohistory.append(node) nohistory.append(node)
ui.progress(_("building history"), None) ui.progress(_("building history"), None)
# Order the nodes children first, so we can produce reverse deltas # Order the nodes children first, so we can produce reverse deltas
orderednodes = list(reversed(self._toposort(ancestors))) orderednodes = list(reversed(self._toposort(ancestors)))
if len(nohistory) > 0:
ui.debug('repackdata: %d nodes without history\n' %
len(nohistory))
orderednodes.extend(sorted(nohistory)) orderednodes.extend(sorted(nohistory))
# Compute deltas and write to the pack # Compute deltas and write to the pack
deltabases = defaultdict(lambda: (nullid, 0)) deltabases = defaultdict(lambda: (nullid, 0))
nodes = set(nodes) nodes = set(nodes)
for i, node in enumerate(orderednodes): for i, node in enumerate(orderednodes):
# orderednodes is all ancestors, but we only want to serialize # orderednodes is all ancestors, but we only want to serialize
# the files we have. # the files we have.

tests/test-treemanifest-repack.t

$ cp 21501384df03b8489b366c5218be639fa08830e4.dataidx x1501384df03b8489b366c5218be639fa08830e4.dataidx $ cp 21501384df03b8489b366c5218be639fa08830e4.dataidx x1501384df03b8489b366c5218be639fa08830e4.dataidx
$ cp 347263bf1efbdb5bf7e1d1565b6b504073fb9093.datapack x47263bf1efbdb5bf7e1d1565b6b504073fb9093.datapack $ cp 347263bf1efbdb5bf7e1d1565b6b504073fb9093.datapack x47263bf1efbdb5bf7e1d1565b6b504073fb9093.datapack
$ cp 347263bf1efbdb5bf7e1d1565b6b504073fb9093.dataidx x47263bf1efbdb5bf7e1d1565b6b504073fb9093.dataidx $ cp 347263bf1efbdb5bf7e1d1565b6b504073fb9093.dataidx x47263bf1efbdb5bf7e1d1565b6b504073fb9093.dataidx
$ cd ../../../../ $ cd ../../../../
- repack incremental kicks in once there are a number of packs - repack incremental kicks in once there are a number of packs
- (set the repacksizelimit so that we test that we only repack up to 1500 bytes, - (set the repacksizelimit so that we test that we only repack up to 1500 bytes,
- and it leaves one datapack behind) - and it leaves one datapack behind)
$ cp -R .hg/store/packs .hg/store/packs.bak
$ hg repack --incremental --config remotefilelog.data.generations=300,200 --config remotefilelog.data.repacksizelimit=1500B $ hg repack --incremental --config remotefilelog.data.generations=300,200 --config remotefilelog.data.repacksizelimit=1500B
$ ls_l .hg/store/packs/manifests | grep datapack | wc -l $ ls_l .hg/store/packs/manifests | grep datapack | wc -l
.*3 (re) .*3 (re)
$ ls_l .hg/store/packs/manifests | grep datapack | grep 248 $ ls_l .hg/store/packs/manifests | grep datapack | grep 248
-r--r--r-- 248 *.datapack (glob) -r--r--r-- 248 *.datapack (glob)
- Verify that full history is used to choose node order for data repack, even if
- we actually repack fewer history packs.
$ rm -r .hg/store/packs
$ mv .hg/store/packs.bak .hg/store/packs
- history.maxrepackpacks=0 is passed to ensure no history packs are repacked. If
- any entries have missing ancestry, a debug warning is printed.
$ hg repack --config remotefilelog.data.generations=300,200 --config remotefilelog.data.repacksizelimit=1500B --config remotefilelog.history.maxrepackpacks=0 --debug
- Inspect all data packs. We except to see none with > 1 delta bases of 000000000000
durhamUnsubmitted
Not Done

s/except/expect/

durham: s/except/expect/
- (which would indicate we made duplicative chains.)
$ ls .hg/store/packs/manifests/*.datapack | xargs -L1 hg debugdatapack
durhamUnsubmitted
Not Done

Does this xargs work on both osx and linux? I'm always wary of bashisms in the tests

durham: Does this xargs work on both osx and linux? I'm always wary of bashisms in the tests
Node Delta Base Delta Length
edbe899c31e7 000000000000 89
23d6a1af5d5b edbe899c31e7 58
53dd6ae0d984 23d6a1af5d5b 58
10d05e165f71 53dd6ae0d984 58
fba208634b61 10d05e165f71 58
e24ac4639900 fba208634b61 58
1e27a5956e61 e24ac4639900 58
dir
Node Delta Base Delta Length
31a760f1fb09 000000000000 43
d62223eee804 31a760f1fb09 55
6bfa2a87412d d62223eee804 55
984db90696cd 6bfa2a87412d 55
84977318a34f 984db90696cd 55
f175c774e18b 84977318a34f 55
phillcoAuthorUnsubmitted
Not Done

Sigh, these break check-code. Any suggestions?

phillco: Sigh, these break check-code. Any suggestions?
durhamUnsubmitted
Not Done

Why is there so much white space in the output?

durham: Why is there so much white space in the output?
phillcoAuthorUnsubmitted
Not Done

I think debugdatapack prints extra lines in some cases.

phillco: I think debugdatapack prints extra lines in some cases.
durhamUnsubmitted
Not Done

Really? I don't think check code would complain if these were part of test output. If they were test output I'd expect them to have 2 spaces at the front, which they don't appear to (at least from what highlighting with my mouse says).

durham: Really? I don't think check code would complain if these were part of test output. If they…
phillcoAuthorUnsubmitted
Not Done

ah, turns out my editor was also stripping them. In any case I made hgdebugdatapack take multiple files, which I'll send as a followup.

phillco: ah, turns out my editor was also stripping them. In any case I made hgdebugdatapack take…
- Clean up the pile of packs we made - Clean up the pile of packs we made
$ hg repack $ hg repack
Test repacking from revlogs to pack files on the server Test repacking from revlogs to pack files on the server
$ cd ../master $ cd ../master
$ cat >> .hg/hgrc <<EOF $ cat >> .hg/hgrc <<EOF
> [extensions] > [extensions]
> treemanifest=$TESTDIR/../treemanifest > treemanifest=$TESTDIR/../treemanifest
> remotefilelog=$TESTDIR/../remotefilelog > remotefilelog=$TESTDIR/../remotefilelog
> [remotefilelog] > [remotefilelog]
> server=True > server=True
> [treemanifest] > [treemanifest]
> server=True > server=True
phillcoAuthorUnsubmitted
Not Done

Sigh, these break check-code. Any suggestions?

phillco: Sigh, these break check-code. Any suggestions?
> EOF > EOF
$ hg backfilltree $ hg backfilltree
$ cat .hg/store/fncache | sort $ cat .hg/store/fncache | sort
data/a.i data/a.i
data/dir/b.i data/dir/b.i
meta/dir/00manifest.i meta/dir/00manifest.i
$ hg repack $ hg repack