[PATCH] rev: C implementation of delta chain resolution

[Gregory Szorc]

On Thu, Jun 29, 2017 at 8:45 PM, Jun Wu <quark at fb.com> wrote:

> Looks good to me. I have some minor comments, which don't affect
> correctness
> and only count for 2% performance difference. So nothing serious. I'd
> suggest queue this now, and we can improve later.
>
> Excerpts from 's message of 2017-06-25 12:42:53 -0700:
> > +static inline int index_baserev(indexObject *self, int rev)
>
> Might worth a comment that "rev" range check must be done by the caller. So
> new users of this function will be less likely to cause SIGSEGV.
>
> > +    if (!PyArg_ParseTuple(args, "iOi", &rev, &stoparg, &generaldelta)) {
>
> "generaldelta" could be part of indexObject (passed via initfunc) to avoid
> "self._generaldelta" in Python code which is a hash lookup. That also makes
> it possible to have a same signature with self._deltachain.
>
> > [...]
> > +    /* This should never happen. */
> > +    if (baserev == -2) {
>
> Might use "<= -2" to rule out other strange cases.
>
> > +        # Try C implementation.
> > +        try:
> > +            return self.index.deltachain(rev, stoprev,
> self._generaldelta)
> > +        except AttributeError:
> > +            pass
> > +
>
> I think it's better to replace the function instead of calling "try" each
> time. Like in revlog.__init__:
>
>   try:
>       self._deltachain = self.index.deltachain
>   except AttributeError:
>       pass
>
> With the above changes (avoid passing generaldelta and replace the method
> directly), perfrevlogrevision --cache deltachain gets 2% improvement.
> Smaller than I excepted, so might be not that worthy to do the change.
>

I agree with this feedback to make the signature identical and not having
to check attribute presence on every invocation. However, it is beyond my
effort to benefit ratio, so I'm not going to implement it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170701/93d2e194/attachment.html>