This is an archive of the discontinued Mercurial Phabricator instance.

cext: use dedicated type for index entries
Needs RevisionPublic

Authored by indygreg on Dec 26 2017, 7:35 PM.

Details

Reviewers
yuja
durin42
baymax
Group Reviewers
hg-reviewers
Summary

Now that we have a handle on our type to represent revlog index
entries, let's use it.

This commit essentially consists of porting code from PyTuple to
PyObject.

As part of porting the code, we now retrieve elements in the entry
type by named attribute instead of integer index.

Before, PyTuple_* APIs allowed us to retrieve a borrowed reference
to PyObject elements. We can no longer do this via the PyObject_*
APIs for a type not implemented in C. This required extra refcount
manipulation in various places.

The C extension is now emitting the new type. And because various
code is still accessing index entry elements via getitem, we
see a performance regression in the Firefox repository:

$ hg perfrevset '::tip'
! wall 0.755869 comb 0.750000 user 0.750000 sys 0.000000 (best of 13)
! wall 1.011107 comb 1.010000 user 1.010000 sys 0.000000 (best of 10)

We will claw back this regression in subsequent commits by accessing
fields by name instead of index.

The version of the "parsers" C extension has been incremented to
reflect the change in behavior.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Dec 26 2017, 7:35 PM
yuja requested changes to this revision.Jan 5 2018, 12:22 AM
yuja added a subscriber: yuja.
yuja added inline comments.
mercurial/cext/revlog.c
141

Perhaps AttributeError would be set by PyObject_GetAttrString().

318

This is tricky, but looks okay. Alternatively, we could inc/decref borrowed obj.

324

I slightly prefer goto bail instead of result = NULL; goto cleanup pairs.

1777

No error reported to the caller.

AttributeError would be set automatically.

This revision now requires changes to proceed.Jan 5 2018, 12:22 AM
indygreg edited the summary of this revision. (Show Details)Jan 6 2018, 3:26 AM
indygreg updated this revision to Diff 4738.
durin42 accepted this revision.Jan 10 2018, 5:54 PM
baymax requested changes to this revision.Jan 24 2020, 12:33 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:33 PM