This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1561

basestore: remove empty directories recursively during clean up
ClosedPublic

Authored by singhsrb on Nov 29 2017, 7:11 PM.
Tags
None
Subscribers
quark

Details

Reviewers
quark
Group Reviewers
Restricted Project
Commits
rFBHGX545ee92bfc45: basestore: remove empty directories recursively during clean up
Summary

The cleanup method right now does not clean up all the directories that are
empty. This leads to a situation where we can have a large number of empty
directories for the next repack which is unnecessary. This commit removes all
the empty directories in the cache directory during the clean up.

Test Plan
  • Ran all the tests.
  • Removed about 220k empty directories from the local cache. Running time of hg repack --incremental went from 3-4s every time to around 5s for the first time (because of deleting the empty directories) and ~200 milliseconds every subsequent time. Note that there was nothing to repack during the repack operation.

Diff Detail

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

Event Timeline

singhsrb created this revision.Nov 29 2017, 7:11 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 29 2017, 7:11 PM
singhsrb edited the summary of this revision. (Show Details)Nov 29 2017, 7:15 PM
singhsrb edited the test plan for this revision. (Show Details)Nov 29 2017, 7:25 PM
singhsrb edited the test plan for this revision. (Show Details)Nov 29 2017, 7:28 PM
quark requested changes to this revision.Nov 29 2017, 8:33 PM
quark added a subscriber: quark.
quark added inline comments.
remotefilelog/repack.py
687–694

isdir is a syscall. Instead of testing if root is a directory or not first (which is also subject to file system race condition - someone could replace that directory with a file really quickly), just call rmdir and handle its exception.

This revision now requires changes to proceed.Nov 29 2017, 8:33 PM
singhsrb updated this revision to Diff 3994.Nov 29 2017, 8:54 PM
singhsrb marked an inline comment as done.Nov 29 2017, 8:54 PM
singhsrb added inline comments.
remotefilelog/repack.py
687–694

Nice observation! I have fixed that.

quark added a comment.EditedNov 29 2017, 9:27 PM

It still feels weird that both cleanup and _removeemptydirectories have os.listdir. Could it be recursive? Besides, I think there is no need to test true/false before calling rmdir since filesystem is racy.

osutil = mercurial.policy.importmod('osutil')

def tryrmdir(path):
    try:
        os.rmdir(path)
    except OSError:
        pass

def removeemptydirs(path):
    # osutil.listdir returns stat information which saves some rmdir/listdir syscalls.
    for name, mode in osutil.listdir(path):
        if stat.S_ISDIR(mode):
            removeemptydirs(os.path.join(path, name))
    tryrmdir(path)
singhsrb marked an inline comment as done.Nov 30 2017, 1:57 PM
singhsrb retitled this revision from repack: remove empty directories recursively during clean up to basestore: remove empty directories recursively during clean up.
singhsrb updated this revision to Diff 4014.
singhsrb added a comment.Nov 30 2017, 2:00 PM

@quark I think I have addressed most of your concerns. Also, the earlier code was potentially trying to remove empty directories from the whole cache. So, I have changed it a bit to only care for the repository in consideration.

quark accepted this revision.Nov 30 2017, 6:20 PM
This revision is now accepted and ready to land.Nov 30 2017, 6:20 PM
Closed by commit rFBHGX545ee92bfc45: basestore: remove empty directories recursively during clean up (authored by singhsrb). · Explain WhyNov 30 2017, 8:22 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mremotefilelog/basestore.py (21 lines)
Mremotefilelog/repack.py (24 lines)

Diff 3986

remotefilelog/basestore.py

from __future__ import absolute_import from __future__ import absolute_import
import errno, hashlib, heapq, os, shutil, time import errno, hashlib, os, shutil, time
from . import ( from . import (
constants, constants,
shallowutil, shallowutil,
) )
from mercurial import error from mercurial import error
from mercurial.i18n import _ from mercurial.i18n import _
for node in nodes: for node in nodes:
ledger.markdataentry(self, filename, node) ledger.markdataentry(self, filename, node)
ledger.markhistoryentry(self, filename, node) ledger.markhistoryentry(self, filename, node)
def cleanup(self, ledger): def cleanup(self, ledger):
ui = self.ui ui = self.ui
entries = ledger.sources.get(self, []) entries = ledger.sources.get(self, [])
count = 0 count = 0
directories = set()
for entry in entries: for entry in entries:
if entry.gced or (entry.datarepacked and entry.historyrepacked): if entry.gced or (entry.datarepacked and entry.historyrepacked):
ui.progress(_("cleaning up"), count, unit="files", ui.progress(_("cleaning up"), count, unit="files",
total=len(entries)) total=len(entries))
path = self._getfilepath(entry.filename, entry.node) path = self._getfilepath(entry.filename, entry.node)
dirpath = os.path.dirname(path)
directories.add((-len(dirpath), dirpath))
try: try:
os.remove(path) os.remove(path)
except OSError as ex: except OSError as ex:
# If the file is already gone, no big deal # If the file is already gone, no big deal
if ex.errno != errno.ENOENT: if ex.errno != errno.ENOENT:
raise raise
count += 1 count += 1
ui.progress(_("cleaning up"), None) ui.progress(_("cleaning up"), None)
# Clean up directories
cachepath = shallowutil.getcachepath(ui)
dirheap = list(directories)
heapq.heapify(dirheap)
seen = set([cachepath])
while dirheap:
length, dirpath = heapq.heappop(dirheap)
try:
os.rmdir(dirpath)
parent = os.path.dirname(dirpath)
if parent not in seen:
seen.add(parent)
heapq.heappush(dirheap, (-len(parent), parent))
except OSError:
pass
# BELOW THIS ARE NON-STANDARD APIS # BELOW THIS ARE NON-STANDARD APIS
def _getfiles(self): def _getfiles(self):
"""Return a list of (filename, [node,...]) for all the revisions that """Return a list of (filename, [node,...]) for all the revisions that
exist in the store. exist in the store.
This is useful for obtaining a list of all the contents of the store This is useful for obtaining a list of all the contents of the store
when performing a repack to another store, since the store API requires when performing a repack to another store, since the store API requires

remotefilelog/repack.py

# Populate ledger from source # Populate ledger from source
self.data.markledger(ledger, options=self.options) self.data.markledger(ledger, options=self.options)
self.history.markledger(ledger, options=self.options) self.history.markledger(ledger, options=self.options)
# Run repack # Run repack
self.repackdata(ledger, targetdata) self.repackdata(ledger, targetdata)
self.repackhistory(ledger, targethistory) self.repackhistory(ledger, targethistory)
# Call cleanup on each source # Clean up
for source in ledger.sources: self.cleanup(ledger)
source.cleanup(ledger)
def _chainorphans(self, ui, filename, nodes, orphans, deltabases): def _chainorphans(self, ui, filename, nodes, orphans, deltabases):
"""Reorderes ``orphans`` into a single chain inside ``nodes`` and """Reorderes ``orphans`` into a single chain inside ``nodes`` and
``deltabases``. ``deltabases``.
We often have orphan entries (nodes without a base that aren't We often have orphan entries (nodes without a base that aren't
referenced by other nodes -- i.e., part of a chain) due to gaps in referenced by other nodes -- i.e., part of a chain) due to gaps in
history. Rather than store them as individual fulltexts, we prefer to history. Rather than store them as individual fulltexts, we prefer to
count += 1 count += 1
ui.progress(_("repacking history"), count, unit=self.unit, ui.progress(_("repacking history"), count, unit=self.unit,
total=len(byfile)) total=len(byfile))
ui.progress(_("repacking history"), None) ui.progress(_("repacking history"), None)
target.close(ledger=ledger) target.close(ledger=ledger)
def cleanup(self, ledger):
# Call cleanup on each source
for source in ledger.sources:
source.cleanup(ledger)
# Clean up leftover empty directories
cachepath = shallowutil.getcachepath(self.repo.ui)
for name in os.listdir(cachepath):
self._removeemptydirectories(os.path.join(cachepath, name))
def _removeemptydirectories(self, root):
if os.path.isdir(root):
paths = [os.path.join(root, name) for name in os.listdir(root)]
if all([self._removeemptydirectories(path) for path in paths]):
os.rmdir(root)
return True
quarkUnsubmitted
Done

isdir is a syscall. Instead of testing if root is a directory or not first (which is also subject to file system race condition - someone could replace that directory with a file really quickly), just call rmdir and handle its exception.

quark: `isdir` is a syscall. Instead of testing if root is a directory or not first (which is also…
singhsrbAuthorUnsubmitted
Not Done

Nice observation! I have fixed that.

singhsrb: Nice observation! I have fixed that.
return False
def _toposort(self, ancestors): def _toposort(self, ancestors):
def parentfunc(node): def parentfunc(node):
p1, p2, linknode, copyfrom = ancestors[node] p1, p2, linknode, copyfrom = ancestors[node]
parents = [] parents = []
if p1 != nullid: if p1 != nullid:
parents.append(p1) parents.append(p1)
if p2 != nullid: if p2 != nullid:
parents.append(p2) parents.append(p2)