This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7483

changectx: add a "maybe filtered" filtered attribute
ClosedPublic

Authored by marmoute on Nov 22 2019, 4:20 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG1e87851dba63: changectx: add a "maybe filtered" filtered attribute
rHG8a37203ab1d5: changectx: add a "maybe filtered" filtered attribute
Summary

There are changeset that we know not to be filtered (eg: null). In this case,
we could access some information without triggering changelog filtering. We add
an attribute to changectx that track this property.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

marmoute created this revision.Nov 22 2019, 4:20 AM
indygreg added a subscriber: indygreg.Nov 22 2019, 11:04 PM
indygreg added inline comments.
mercurial/context.py
509

Before I accept more of this series, I'd like others to think about the potential alternative solution of passing in or storing a reference to the changelog on basectx/changectx. The reason is that a lot of methods are calling repo.changelog and this can be expensive. I suspect we'd get a handful of random performance wins if we cached the changelog instance on each basectx. We also wouldn't need to litter the code with a bunch of conditional repo.unfiltered() calls since we'd already have an appropriate changelog instance stored in the instance.

Thoughts?

martinvonz added a subscriber: martinvonz.Nov 23 2019, 11:21 AM
martinvonz added inline comments.
mercurial/context.py
484

Add a comment explaining what this attribute means, such as "indicates that this changeset is visible regardless of filtering". Actually, maybe it's better to name the property something like filter_agnostic?

509

That would make ctx.children() incorrect, right?

I still wonder if the "visible" filter should be removed and (so there would be no --hidden, and no annoying message telling you use that flag). We would need to figure out what e.g. hg log -r 'head()' and hg log -r x:: should do (to not include extinct heads) and how to make it easy for the user to get either behavior. Perhaps we would let head() be the visible heads and add a new allheads() to get all? That's obviously a much larger discussion and maybe a topic for the next sprint instead.

marmoute added a comment.Nov 23 2019, 11:57 AM

(grmlml, realising that I forgot to do the dual "submit" for my comment. It is 2019, why is Phabricator still not fixing this…)

mercurial/context.py
509

We woul still need the conditional instance since the maybe_filtered logic only apply to some of the changectx call. (eg: children need to go through filtering).

An overall better/more reobust solution to make the filtering lazier at the changelog level and to most the fastpath logic there. It would be more contained and more robust.

However, I decided I digged the rabbit hole enough for now and this other approach can be looked at later.

marmoute added inline comments.Nov 23 2019, 12:01 PM
mercurial/context.py
484

I'll add a comment.

filter_agnostic is misleading. the revision might be filtered for another filter. Is is also borderline double negative.

509

The filtering is usefull at many level and removing it means dedicated code in many places (eg: discovery). That is error prone. I feel like effort would be better put to simply making it go fast (could be O(1) with proper indexing).
Which anoying message are you talking about ?

I agree that this is a larger discussion and I would rather not see the discussion around this series derails. +1 to have it independently later.

marmoute updated this revision to Diff 18362.Nov 23 2019, 7:21 PM
indygreg accepted this revision.Nov 23 2019, 7:44 PM

I'm satisfied with the discussion and will proceed with reviewing this series.

This revision is now accepted and ready to land.Nov 23 2019, 7:44 PM
marmoute added a commit: rHG8a37203ab1d5: changectx: add a "maybe filtered" filtered attribute.Nov 23 2019, 7:58 PM
Closed by commit rHG8a37203ab1d5: changectx: add a "maybe filtered" filtered attribute (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/context.py (15 lines)

Diff 18362

mercurial/context.py

return r return r
class changectx(basectx): class changectx(basectx):
"""A changecontext object makes access to data related to a particular """A changecontext object makes access to data related to a particular
changeset convenient. It represents a read-only context already present in changeset convenient. It represents a read-only context already present in
the repo.""" the repo."""
def __init__(self, repo, rev, node): def __init__(self, repo, rev, node, maybe_filtered=True):
super(changectx, self).__init__(repo) super(changectx, self).__init__(repo)
self._rev = rev self._rev = rev
self._node = node self._node = node
# When maybe_filtered is True, the revision might be affected by
martinvonzUnsubmitted
Not Done

Add a comment explaining what this attribute means, such as "indicates that this changeset is visible regardless of filtering". Actually, maybe it's better to name the property something like filter_agnostic?

martinvonz: Add a comment explaining what this attribute means, such as "indicates that this changeset is…
marmouteAuthorUnsubmitted
Done

I'll add a comment.

filter_agnostic is misleading. the revision might be filtered for another filter. Is is also borderline double negative.

marmoute: I'll add a comment. filter_agnostic is misleading. the revision might be filtered for another…
# changelog filtering and operation through the filtered changelog must be used.
#
# When maybe_filtered is False, the revision has already been checked
# against filtering and is not filtered. Operation through the
# unfiltered changelog might be used in some case.
self._maybe_filtered = maybe_filtered
def __hash__(self): def __hash__(self):
try: try:
return hash(self._rev) return hash(self._rev)
except AttributeError: except AttributeError:
return id(self) return id(self)
def __nonzero__(self): def __nonzero__(self):
return self._rev != nullrev return self._rev != nullrev
__bool__ = __nonzero__ __bool__ = __nonzero__
@propertycache @propertycache
def _changeset(self): def _changeset(self):
return self._repo.changelog.changelogrevision(self.rev()) if self._maybe_filtered:
repo = self._repo
else:
repo = self._repo.unfiltered()
return repo.changelog.changelogrevision(self.rev())
indygregUnsubmitted
Not Done

Before I accept more of this series, I'd like others to think about the potential alternative solution of passing in or storing a reference to the changelog on basectx/changectx. The reason is that a lot of methods are calling repo.changelog and this can be expensive. I suspect we'd get a handful of random performance wins if we cached the changelog instance on each basectx. We also wouldn't need to litter the code with a bunch of conditional repo.unfiltered() calls since we'd already have an appropriate changelog instance stored in the instance.

Thoughts?

indygreg: Before I accept more of this series, I'd like others to think about the potential alternative…
marmouteAuthorUnsubmitted
Done

We woul still need the conditional instance since the maybe_filtered logic only apply to some of the changectx call. (eg: children need to go through filtering).

An overall better/more reobust solution to make the filtering lazier at the changelog level and to most the fastpath logic there. It would be more contained and more robust.

However, I decided I digged the rabbit hole enough for now and this other approach can be looked at later.

marmoute: We woul still need the conditional instance since the maybe_filtered logic only apply to some…
martinvonzUnsubmitted
Not Done

That would make ctx.children() incorrect, right?

I still wonder if the "visible" filter should be removed and (so there would be no --hidden, and no annoying message telling you use that flag). We would need to figure out what e.g. hg log -r 'head()' and hg log -r x:: should do (to not include extinct heads) and how to make it easy for the user to get either behavior. Perhaps we would let head() be the visible heads and add a new allheads() to get all? That's obviously a much larger discussion and maybe a topic for the next sprint instead.

martinvonz: That would make `ctx.children()` incorrect, right? I still wonder if the "visible" filter…
marmouteAuthorUnsubmitted
Done

The filtering is usefull at many level and removing it means dedicated code in many places (eg: discovery). That is error prone. I feel like effort would be better put to simply making it go fast (could be O(1) with proper indexing).
Which anoying message are you talking about ?

I agree that this is a larger discussion and I would rather not see the discussion around this series derails. +1 to have it independently later.

marmoute: The filtering is usefull at many level and removing it means dedicated code in many places (eg…
@propertycache @propertycache
def _manifest(self): def _manifest(self):
return self._manifestctx.read() return self._manifestctx.read()
@property @property
def _manifestctx(self): def _manifestctx(self):
return self._repo.manifestlog[self._changeset.manifest] return self._repo.manifestlog[self._changeset.manifest]