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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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–379

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–379

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 24788

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))
) )
def _filterstorefile(srcrepo, dstrepo, requirements, path, mode, st): def _files_to_copy_post_revlog_clone(srcrepo):
mharbison72Unsubmitted
Not Done

Accidental change?

mharbison72: Accidental change?
pulkitAuthorUnsubmitted
Done

Ha yep!

pulkit: Ha yep!
"""Determine whether to copy a store file during upgrade. """yields files which should be copied to destination after revlogs
are cloned"""
This function is called when migrating store files from ``srcrepo`` to for path, kind, st in sorted(srcrepo.store.vfs.readdir(b'', stat=True)):
``dstrepo`` as part of upgrading a repository. # don't copy revlogs as they are already cloned
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.
"""
# Skip revlogs.
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 _replacestores(currentrepo, upgradedrepo, backupvfs, upgrade_op): def _replacestores(currentrepo, upgradedrepo, backupvfs, upgrade_op):
"""Replace the stores after current repository is upgraded """Replace the stores after current repository is upgraded
Creates a backup of current repository store at backup path Creates a backup of current repository store at backup path
Replaces upgraded store files in current repo from upgraded one Replaces upgraded store files in current repo from upgraded one
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 upgraded in a temporary repository\n')) ui.status(_(b'data fully upgraded in a temporary repository\n'))