This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9673

engine: prevent a function call for each store file
ClosedPublic

Authored by pulkit on Dec 31 2020, 11:42 AM.

Details

Reviewers
marmoute
mharbison72
Group Reviewers
hg-reviewers
Commits
rHG52abb1af2995: engine: prevent a function call for each store file
Summary

Python function calls are not cheap. Instead of calling the function once for
each file in store, we use a dedicated function which filters and yields the
required files.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

pulkit created this revision.Dec 31 2020, 11:42 AM
marmoute accepted this revision.Jan 7 2021, 9:39 PM

on second though, I am not sure what's the motivation is here. How long does filtering all entry actually takes ?

pulkit added a comment.Jan 8 2021, 11:30 AM
In D9673#146237, @marmoute wrote:

on second though, I am not sure what's the motivation is here. How long does filtering all entry actually takes ?

I didn't perf-measure it. Earlier it used to do a function call for each file inside .hg/store and now it's just one function call.

marmoute added a comment.Jan 8 2021, 2:36 PM

yeah, but that function itself is doing 1 call per file, so I doubt we will be a significant performance win.

mharbison72 requested changes to this revision.Jan 12 2021, 6:39 PM
mharbison72 added a subscriber: mharbison72.
mharbison72 added inline comments.
mercurial/upgrade_utils/engine.py
378

Accidental change?

This revision now requires changes to proceed.Jan 12 2021, 6:39 PM
mharbison72 accepted this revision.Jan 12 2021, 6:45 PM
This revision is now accepted and ready to land.Jan 12 2021, 6:45 PM
mharbison72 requested changes to this revision.Jan 12 2021, 6:47 PM

Oops, typo on my end with phabupdate. I still wanted to flag the new return statement.

This revision now requires changes to proceed.Jan 12 2021, 6:47 PM
pulkit added a comment.Jan 13 2021, 4:32 AM
In D9673#146406, @marmoute wrote:

yeah, but that function itself is doing 1 call per file, so I doubt we will be a significant performance win.

Hm, no? Now we have one function which yields results. Does yielding call function multiple times?

mercurial/upgrade_utils/engine.py
378

Ha yep!

mharbison72 accepted this revision.Jan 13 2021, 12:11 PM
This revision is now accepted and ready to land.Jan 13 2021, 12:11 PM
pulkit added a commit: rHG52abb1af2995: engine: prevent a function call for each store file.Jan 13 2021, 12:14 PM
Closed by commit rHG52abb1af2995: engine: prevent a function call for each store file (authored by pulkit). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/upgrade_utils/engine.py (55 lines)

Diff 24560

mercurial/upgrade_utils/engine.py

ui.status( ui.status(
_( _(
b'finished migrating %d total revisions; total change in store ' b'finished migrating %d total revisions; total change in store '
b'size: %s\n' b'size: %s\n'
) )
% (revcount, util.bytecount(dstsize - srcsize)) % (revcount, util.bytecount(dstsize - srcsize))
) )
return True
mharbison72Unsubmitted
Not Done

Accidental change?

mharbison72: Accidental change?
pulkitAuthorUnsubmitted
Done

Ha yep!

pulkit: Ha yep!
def _filterstorefile(srcrepo, dstrepo, requirements, path, mode, st):
"""Determine whether to copy a store file during upgrade.
This function is called when migrating store files from ``srcrepo`` to
``dstrepo`` as part of upgrading a repository.
Args:
srcrepo: repo we are copying from
dstrepo: repo we are copying to
requirements: set of requirements for ``dstrepo``
path: store file being examined
mode: the ``ST_MODE`` file type of ``path``
st: ``stat`` data structure for ``path``
Function should return ``True`` if the file is to be copied. def _files_to_copy_post_revlog_clone(srcrepo):
""" """yields files which should be copied to destination after revlogs
# Skip revlogs. are cloned"""
for path, kind, st in sorted(srcrepo.store.vfs.readdir(b'', stat=True)):
# don't copy revlogs as they are already cloned
if path.endswith((b'.i', b'.d', b'.n', b'.nd')): if path.endswith((b'.i', b'.d', b'.n', b'.nd')):
return False continue
# Skip transaction related files. # Skip transaction related files.
if path.startswith(b'undo'): if path.startswith(b'undo'):
return False continue
# Only copy regular files. # Only copy regular files.
if mode != stat.S_IFREG: if kind != stat.S_IFREG:
return False continue
# Skip other skipped files. # Skip other skipped files.
if path in (b'lock', b'fncache'): if path in (b'lock', b'fncache'):
return False continue
# TODO: should we skip cache too?
return True yield path
def finishdatamigration(ui, srcrepo, dstrepo, requirements): def finishdatamigration(ui, srcrepo, dstrepo, requirements):
"""Hook point for extensions to perform additional actions during upgrade. """Hook point for extensions to perform additional actions during upgrade.
This function is called after revlogs and store files have been copied but This function is called after revlogs and store files have been copied but
before the new store is swapped into the original location. before the new store is swapped into the original location.
""" """
ui, ui,
srcrepo, srcrepo,
dstrepo, dstrepo,
tr, tr,
upgrade_op, upgrade_op,
) )
# Now copy other files in the store directory. # Now copy other files in the store directory.
# The sorted() makes execution deterministic. for p in _files_to_copy_post_revlog_clone(srcrepo):
for p, kind, st in sorted(srcrepo.store.vfs.readdir(b'', stat=True)):
if not _filterstorefile(
srcrepo, dstrepo, upgrade_op.new_requirements, p, kind, st
):
continue
srcrepo.ui.status(_(b'copying %s\n') % p) srcrepo.ui.status(_(b'copying %s\n') % p)
src = srcrepo.store.rawvfs.join(p) src = srcrepo.store.rawvfs.join(p)
dst = dstrepo.store.rawvfs.join(p) dst = dstrepo.store.rawvfs.join(p)
util.copyfile(src, dst, copystat=True) util.copyfile(src, dst, copystat=True)
finishdatamigration(ui, srcrepo, dstrepo, requirements) finishdatamigration(ui, srcrepo, dstrepo, requirements)
ui.status(_(b'data fully migrated to temporary repository\n')) ui.status(_(b'data fully migrated to temporary repository\n'))