( )⚙ D11262 repair: improve performance of detection of revisions affected by issue6528

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11262

repair: improve performance of detection of revisions affected by issue6528
ClosedPublic

Authored by Alphare on Aug 6 2021, 6:12 AM.

Details

Reviewers
marmoute
pulkit
Group Reviewers
hg-reviewers
Commits
rHG32e21ac3adb1: repair: improve performance of detection of revisions affected by issue6528
Summary

Explanations inside the patch. I've tested this on Mozilla-Central and it's
5 times faster than the naive approach on my laptop.

Diff Detail

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

Event Timeline

Alphare created this revision.Aug 6 2021, 6:12 AM
marmoute requested changes to this revision.Aug 6 2021, 6:57 AM
marmoute added a subscriber: marmoute.

The overall logic seems fine, but the affected_chain cache and the expected_metadata_len seems wrong,

mercurial/revlogutils/rewrite.py
591–592

Should we check if they are affected ? or if they have metadata ? because if we have no diff compared to a revision that has metadata, we do have metadat and might have our parent swapped, unlike the delta base.

621–623

I am confused about this check, why are we using filename here ?

This revision now requires changes to proceed.Aug 6 2021, 6:57 AM
Alphare added inline comments.Aug 6 2021, 8:37 AM
mercurial/revlogutils/rewrite.py
591–592

I'm not sure I understand what you're asking

621–623

Right, we should simply use a single placeholder byte (for the shortest possible copy source), and not the filename, I forgot about that.

Alphare updated this revision to Diff 29836.Aug 6 2021, 1:50 PM
marmoute added a comment.Aug 6 2021, 5:35 PM

Looks overall good, but for the comment I made were we seems to do extra, unnecessary, revision restoration. I'll have a look tomorrow to check if we can safely remove this (and if not, why).

mercurial/revlogutils/rewrite.py
604

This seems to trigger the slow path for a non linear revision and might slow things down. Why do we needs it ?

marmoute added inline comments.Aug 7 2021, 6:12 AM
mercurial/revlogutils/rewrite.py
604

Dropping this line does not affect the correctness of the result and results in a significant speed up : 2m53 → 1m43

I am sending an updating this line dropped.

marmoute updated this revision to Diff 29841.Aug 7 2021, 6:13 AM
marmoute accepted this revision.Aug 7 2021, 6:27 AM
baymax updated this revision to Diff 29844.Aug 7 2021, 9:47 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

baymax updated this revision to Diff 29856.Aug 7 2021, 6:09 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

pulkit accepted this revision.Aug 10 2021, 8:16 AM
This revision is now accepted and ready to land.Aug 10 2021, 8:16 AM

Revision Contents
Changeset List

PathPackages
Mmercurial/revlogutils/rewrite.py (63 lines)

Diff 29844

mercurial/revlogutils/rewrite.py

# censor code related to censoring revision # censor code related to censoring revision
# coding: utf8 # coding: utf8
# #
# Copyright 2021 Pierre-Yves David <pierre-yves.david@octobus.net> # Copyright 2021 Pierre-Yves David <pierre-yves.david@octobus.net>
# Copyright 2015 Google, Inc <martinvonz@google.com> # Copyright 2015 Google, Inc <martinvonz@google.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
import binascii import binascii
import contextlib import contextlib
import os import os
import struct
from ..node import ( from ..node import (
nullrev, nullrev,
) )
from .constants import ( from .constants import (
COMP_MODE_PLAIN, COMP_MODE_PLAIN,
ENTRY_DATA_COMPRESSED_LENGTH, ENTRY_DATA_COMPRESSED_LENGTH,
ENTRY_DATA_COMPRESSION_MODE, ENTRY_DATA_COMPRESSION_MODE,
rl.opener.rename(new_file_path, index_file) rl.opener.rename(new_file_path, index_file)
rl.clearcaches() rl.clearcaches()
rl._loadindex() rl._loadindex()
finally: finally:
util.tryunlink(new_file_path) util.tryunlink(new_file_path)
def _is_revision_affected(ui, fl, filerev, path): def _is_revision_affected(fl, filerev, metadata_cache=None):
"""Mercurial currently (5.9rc0) uses `p1 == nullrev and p2 != nullrev` as a """Mercurial currently (5.9rc0) uses `p1 == nullrev and p2 != nullrev` as a
special meaning compared to the reverse in the context of filelog-based special meaning compared to the reverse in the context of filelog-based
copytracing. issue6528 exists because new code assumed that parent ordering copytracing. issue6528 exists because new code assumed that parent ordering
didn't matter, so this detects if the revision contains metadata (since didn't matter, so this detects if the revision contains metadata (since
it's only used for filelog-based copytracing) and its parents are in the it's only used for filelog-based copytracing) and its parents are in the
"wrong" order.""" "wrong" order."""
try: try:
raw_text = fl.rawdata(filerev) raw_text = fl.rawdata(filerev)
except error.CensoredNodeError: except error.CensoredNodeError:
# We don't care about censored nodes as they never carry metadata # We don't care about censored nodes as they never carry metadata
return False return False
has_meta = raw_text.startswith(b'\x01\n') has_meta = raw_text.startswith(b'\x01\n')
if metadata_cache is not None:
metadata_cache[filerev] = has_meta
if has_meta: if has_meta:
(p1, p2) = fl.parentrevs(filerev) (p1, p2) = fl.parentrevs(filerev)
if p1 != nullrev and p2 == nullrev: if p1 != nullrev and p2 == nullrev:
return True return True
return False return False
def _is_revision_affected_fast(repo, fl, filerev, metadata_cache):
"""Optimization fast-path for `_is_revision_affected`.
`metadata_cache` is a dict of `{rev: has_metadata}` which allows any
revision to check if its base has metadata, saving computation of the full
text, instead looking at the current delta.
marmouteUnsubmitted
Not Done

Should we check if they are affected ? or if they have metadata ? because if we have no diff compared to a revision that has metadata, we do have metadat and might have our parent swapped, unlike the delta base.

marmoute: Should we check if they are affected ? or if they have metadata ? because if we have no diff…
AlphareAuthorUnsubmitted
Done

I'm not sure I understand what you're asking

Alphare: I'm not sure I understand what you're asking
This optimization only works if the revisions are looked at in order."""
rl = fl._revlog
if rl.iscensored(filerev):
# Censored revisions don't contain metadata, so they cannot be affected
metadata_cache[filerev] = False
return False
p1, p2 = rl.parentrevs(filerev)
if p1 == nullrev or p2 != nullrev:
return False
marmouteUnsubmitted
Not Done

This seems to trigger the slow path for a non linear revision and might slow things down. Why do we needs it ?

marmoute: This seems to trigger the slow path for a non linear revision and might slow things down. Why…
marmouteUnsubmitted
Not Done

Dropping this line does not affect the correctness of the result and results in a significant speed up : 2m53 → 1m43

I am sending an updating this line dropped.

marmoute: Dropping this line does not affect the correctness of the result and results in a significant…
delta_parent = rl.deltaparent(filerev)
parent_has_metadata = metadata_cache.get(delta_parent)
if parent_has_metadata is None:
is_affected = _is_revision_affected(fl, filerev, metadata_cache)
return is_affected
chunk = rl._chunk(filerev)
if not len(chunk):
# No diff for this revision
return parent_has_metadata
header_length = 12
if len(chunk) < header_length:
raise error.Abort(_(b"patch cannot be decoded"))
start, _end, _length = struct.unpack(b">lll", chunk[:header_length])
if start < 2: # len(b'\x01\n') == 2
marmouteUnsubmitted
Not Done

I am confused about this check, why are we using filename here ?

marmoute: I am confused about this check, why are we using `filename` here ?
AlphareAuthorUnsubmitted
Done

Right, we should simply use a single placeholder byte (for the shortest possible copy source), and not the filename, I forgot about that.

Alphare: Right, we should simply use a single placeholder byte (for the shortest possible copy source)…
# This delta does *something* to the metadata marker (if any).
# Check it the slow way
is_affected = _is_revision_affected(fl, filerev, metadata_cache)
return is_affected
# The diff did not remove or add the metadata header, it's then in the same
# situation as its parent
metadata_cache[filerev] = parent_has_metadata
return parent_has_metadata
def _from_report(ui, repo, context, from_report, dry_run): def _from_report(ui, repo, context, from_report, dry_run):
""" """
Fix the revisions given in the `from_report` file, but still checks if the Fix the revisions given in the `from_report` file, but still checks if the
revisions are indeed affected to prevent an unfortunate cyclic situation revisions are indeed affected to prevent an unfortunate cyclic situation
where we'd swap well-ordered parents again. where we'd swap well-ordered parents again.
See the doc for `debug_fix_issue6528` for the format documentation. See the doc for `debug_fix_issue6528` for the format documentation.
""" """
ui.write(_(b"loading report file '%s'\n") % from_report) ui.write(_(b"loading report file '%s'\n") % from_report)
with context(), open(from_report, mode='rb') as f: with context(), open(from_report, mode='rb') as f:
for line in f.read().split(b'\n'): for line in f.read().split(b'\n'):
if not line: if not line:
continue continue
filenodes, filename = line.split(b' ', 1) filenodes, filename = line.split(b' ', 1)
fl = _filelog_from_filename(repo, filename) fl = _filelog_from_filename(repo, filename)
to_fix = set( to_fix = set(
fl.rev(binascii.unhexlify(n)) for n in filenodes.split(b',') fl.rev(binascii.unhexlify(n)) for n in filenodes.split(b',')
) )
excluded = set() excluded = set()
for filerev in to_fix: for filerev in to_fix:
if _is_revision_affected(ui, fl, filerev, filename): if _is_revision_affected(fl, filerev):
msg = b"found affected revision %d for filelog '%s'\n" msg = b"found affected revision %d for filelog '%s'\n"
ui.warn(msg % (filerev, filename)) ui.warn(msg % (filerev, filename))
else: else:
msg = _(b"revision %s of file '%s' is not affected\n") msg = _(b"revision %s of file '%s' is not affected\n")
msg %= (binascii.hexlify(fl.node(filerev)), filename) msg %= (binascii.hexlify(fl.node(filerev)), filename)
ui.warn(msg) ui.warn(msg)
excluded.add(filerev) excluded.add(filerev)
): ):
continue continue
progress.increment() progress.increment()
filename = _get_filename_from_filelog_index(path) filename = _get_filename_from_filelog_index(path)
fl = _filelog_from_filename(repo, filename) fl = _filelog_from_filename(repo, filename)
# Set of filerevs (or hex filenodes if `to_report`) that need fixing # Set of filerevs (or hex filenodes if `to_report`) that need fixing
to_fix = set() to_fix = set()
metadata_cache = {}
for filerev in fl.revs(): for filerev in fl.revs():
# TODO speed up by looking at the start of the delta affected = _is_revision_affected_fast(
# If it hasn't changed, it's not worth looking at the other revs repo, fl, filerev, metadata_cache
# in the same chain )
affected = _is_revision_affected(ui, fl, filerev, path)
if affected: if affected:
msg = b"found affected revision %d for filelog '%s'\n" msg = b"found affected revision %d for filelog '%s'\n"
ui.warn(msg % (filerev, path)) ui.warn(msg % (filerev, path))
found_nothing = False found_nothing = False
if not dry_run: if not dry_run:
if to_report: if to_report:
to_fix.add(binascii.hexlify(fl.node(filerev))) to_fix.add(binascii.hexlify(fl.node(filerev)))
else: else: