This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1768

cext: obtain reference to index entry type
Needs RevisionPublic

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

Details

Reviewers
yuja
durin42
baymax
Group Reviewers
hg-reviewers
Summary

We recently introduced a dedicated type for index entries in the
pure Python implementation.

This commit tells the revlog C code about that type.

We register the type on the parsers module so it is exposed to
importers of the module.

In addition, we store a reference to the type on revlog index
instances. In theory, we could grab the reference from the
module. However, this requires a fair bit of C code. It is easier
to just re-import the original module and get a ref from there. This
also avoids problems with caching a type on a reload()d module.

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 4 2018, 11:54 PM
yuja added a subscriber: yuja.

We hold off incrementing the version of the "parsers" extension
because nothing in core relies on the new API yet.

It's really minor nit, but we have to increment the version since new
compiled module doesn't work with old pure.parsers module, which
provides no IndexV1Entry type.

mercurial/cext/revlog.c
2117

Can we be sure that this IndexV1Entry is identical to self->entrytype
after reloading Python modules?

This revision now requires changes to proceed.Jan 4 2018, 11:54 PM
indygreg added inline comments.Jan 6 2018, 2:11 AM
mercurial/cext/revlog.c
2117

In theory, there could be a mismatch.

If this module is loaded, its IndexV1Entry will refer to whatever mercurial.pure.parsers.IndexV1Entry is. If mercurial.pure.parsers is then reloaded and its IndexV1Entry changes and a revlog index type is created, its IndexV1Entry will be the new one from the reloaded mercurial.pure.parsers.

Since I plan to replace the Python-implemented type with a C backed type, I'm inclined to not care about this.

indygreg edited the summary of this revision. (Show Details)Jan 6 2018, 3:26 AM
indygreg updated this revision to Diff 4737.
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

Revision Contents
Changeset List

PathPackages
Mmercurial/cext/parsers.c (2 lines)
Mmercurial/cext/revlog.c (31 lines)
Mmercurial/policy.py (2 lines)

Diff 4737

mercurial/cext/parsers.c

{"fm1readmarkers", fm1readmarkers, METH_VARARGS, {"fm1readmarkers", fm1readmarkers, METH_VARARGS,
"parse v1 obsolete markers\n"}, "parse v1 obsolete markers\n"},
{NULL, NULL}}; {NULL, NULL}};
void dirs_module_init(PyObject *mod); void dirs_module_init(PyObject *mod);
void manifest_module_init(PyObject *mod); void manifest_module_init(PyObject *mod);
void revlog_module_init(PyObject *mod); void revlog_module_init(PyObject *mod);
static const int version = 4; static const int version = 5;
static void module_init(PyObject *mod) static void module_init(PyObject *mod)
{ {
PyModule_AddIntConstant(mod, "version", version); PyModule_AddIntConstant(mod, "version", version);
/* This module constant has two purposes. First, it lets us unit test /* This module constant has two purposes. First, it lets us unit test
* the ImportError raised without hard-coding any error text. This * the ImportError raised without hard-coding any error text. This
* means we can change the text in the future without breaking tests, * means we can change the text in the future without breaking tests,

mercurial/cext/revlog.c

* supported and proper types are used in the extensions themselves. */ * supported and proper types are used in the extensions themselves. */
#define PyInt_Check PyLong_Check #define PyInt_Check PyLong_Check
#define PyInt_FromLong PyLong_FromLong #define PyInt_FromLong PyLong_FromLong
#define PyInt_FromSsize_t PyLong_FromSsize_t #define PyInt_FromSsize_t PyLong_FromSsize_t
#define PyInt_AS_LONG PyLong_AS_LONG #define PyInt_AS_LONG PyLong_AS_LONG
#define PyInt_AsLong PyLong_AsLong #define PyInt_AsLong PyLong_AsLong
#endif #endif
PyObject *get_index_entry_type(void) {
PyObject *mod, *obj;
mod = PyImport_ImportModule("mercurial.pure.parsers");
if (!mod) {
return NULL;
}
obj = PyObject_GetAttrString(mod, "IndexV1Entry");
Py_DECREF(mod);
return obj;
}
/* /*
* A base-16 trie for fast node->rev mapping. * A base-16 trie for fast node->rev mapping.
* *
* Positive value is index of the next node in the trie * Positive value is index of the next node in the trie
* Negative value is a leaf: -(rev + 1) * Negative value is a leaf: -(rev + 1)
* Zero is empty * Zero is empty
*/ */
typedef struct { typedef struct {
* sentinel. * sentinel.
* *
* With string keys, we lazily perform a reverse mapping from node to * With string keys, we lazily perform a reverse mapping from node to
* rev, using a base-16 trie. * rev, using a base-16 trie.
*/ */
typedef struct { typedef struct {
PyObject_HEAD PyObject_HEAD
/* Type-specific fields go here. */ /* Type-specific fields go here. */
PyObject *entrytype; /* mercurial.pure.parsers.IndexV1Entry type */
PyObject *nullentry; /* Represents an empty/unknown index value */ PyObject *nullentry; /* Represents an empty/unknown index value */
PyObject *data; /* raw bytes of index */ PyObject *data; /* raw bytes of index */
Py_buffer buf; /* buffer of data */ Py_buffer buf; /* buffer of data */
PyObject **cache; /* cached tuples */ PyObject **cache; /* cached tuples */
const char **offsets; /* populated on demand */ const char **offsets; /* populated on demand */
Py_ssize_t raw_length; /* original number of elements */ Py_ssize_t raw_length; /* original number of elements */
Py_ssize_t length; /* current number of elements */ Py_ssize_t length; /* current number of elements */
PyObject *added; /* populated on demand */ PyObject *added; /* populated on demand */
} }
static int index_init(indexObject *self, PyObject *args) static int index_init(indexObject *self, PyObject *args)
{ {
PyObject *data_obj, *inlined_obj; PyObject *data_obj, *inlined_obj;
Py_ssize_t size; Py_ssize_t size;
/* Initialize before argument-checking to avoid index_dealloc() crash. */ /* Initialize before argument-checking to avoid index_dealloc() crash. */
self->entrytype = NULL;
self->nullentry = NULL; self->nullentry = NULL;
self->raw_length = 0; self->raw_length = 0;
self->added = NULL; self->added = NULL;
self->cache = NULL; self->cache = NULL;
self->data = NULL; self->data = NULL;
memset(&self->buf, 0, sizeof(self->buf)); memset(&self->buf, 0, sizeof(self->buf));
self->headrevs = NULL; self->headrevs = NULL;
self->filteredrevs = Py_None; self->filteredrevs = Py_None;
Py_INCREF(Py_None); Py_INCREF(Py_None);
self->nt = NULL; self->nt = NULL;
self->offsets = NULL; self->offsets = NULL;
self->entrytype = get_index_entry_type();
if (!self->entrytype) {
return -1;
}
self->nullentry = Py_BuildValue("iiiiiiis#", 0, 0, 0, self->nullentry = Py_BuildValue("iiiiiiis#", 0, 0, 0,
-1, -1, -1, -1, nullid, 20); -1, -1, -1, -1, nullid, 20);
if (!self->nullentry) { if (!self->nullentry) {
return -1; return -1;
} }
if (!PyArg_ParseTuple(args, "OO", &data_obj, &inlined_obj)) if (!PyArg_ParseTuple(args, "OO", &data_obj, &inlined_obj))
return -1; return -1;
_index_clearcaches(self); _index_clearcaches(self);
Py_XDECREF(self->filteredrevs); Py_XDECREF(self->filteredrevs);
if (self->buf.buf) { if (self->buf.buf) {
PyBuffer_Release(&self->buf); PyBuffer_Release(&self->buf);
memset(&self->buf, 0, sizeof(self->buf)); memset(&self->buf, 0, sizeof(self->buf));
} }
Py_XDECREF(self->data); Py_XDECREF(self->data);
Py_XDECREF(self->added); Py_XDECREF(self->added);
Py_XDECREF(self->entrytype);
Py_XDECREF(self->nullentry); Py_XDECREF(self->nullentry);
PyObject_Del(self); PyObject_Del(self);
} }
static PySequenceMethods index_sequence_methods = { static PySequenceMethods index_sequence_methods = {
(lenfunc)index_length, /* sq_length */ (lenfunc)index_length, /* sq_length */
0, /* sq_concat */ 0, /* sq_concat */
0, /* sq_repeat */ 0, /* sq_repeat */
Py_XDECREF(idx); Py_XDECREF(idx);
Py_XDECREF(cache); Py_XDECREF(cache);
Py_XDECREF(tuple); Py_XDECREF(tuple);
return NULL; return NULL;
} }
void revlog_module_init(PyObject *mod) void revlog_module_init(PyObject *mod)
{ {
PyObject *index_entry_type;
indexType.tp_new = PyType_GenericNew; indexType.tp_new = PyType_GenericNew;
if (PyType_Ready(&indexType) < 0) if (PyType_Ready(&indexType) < 0)
return; return;
Py_INCREF(&indexType); Py_INCREF(&indexType);
PyModule_AddObject(mod, "index", (PyObject *)&indexType); PyModule_AddObject(mod, "index", (PyObject *)&indexType);
index_entry_type = get_index_entry_type();
if (!index_entry_type) {
return;
}
PyModule_AddObject(mod, "IndexV1Entry", index_entry_type);
yujaUnsubmitted
Not Done

Can we be sure that this IndexV1Entry is identical to self->entrytype
after reloading Python modules?

yuja: Can we be sure that this `IndexV1Entry` is identical to `self->entrytype` after reloading…
indygregAuthorUnsubmitted
Not Done

In theory, there could be a mismatch.

If this module is loaded, its IndexV1Entry will refer to whatever mercurial.pure.parsers.IndexV1Entry is. If mercurial.pure.parsers is then reloaded and its IndexV1Entry changes and a revlog index type is created, its IndexV1Entry will be the new one from the reloaded mercurial.pure.parsers.

Since I plan to replace the Python-implemented type with a C backed type, I'm inclined to not care about this.

indygreg: In theory, there could be a mismatch. If this module is loaded, its `IndexV1Entry` will refer…
} }

mercurial/policy.py

# keep in sync with "version" in C modules # keep in sync with "version" in C modules
_cextversions = { _cextversions = {
(r'cext', r'base85'): 1, (r'cext', r'base85'): 1,
(r'cext', r'bdiff'): 1, (r'cext', r'bdiff'): 1,
(r'cext', r'diffhelpers'): 1, (r'cext', r'diffhelpers'): 1,
(r'cext', r'mpatch'): 1, (r'cext', r'mpatch'): 1,
(r'cext', r'osutil'): 3, (r'cext', r'osutil'): 3,
(r'cext', r'parsers'): 4, (r'cext', r'parsers'): 5,
} }
# map import request to other package or module # map import request to other package or module
_modredirects = { _modredirects = {
(r'cext', r'charencode'): (r'cext', r'parsers'), (r'cext', r'charencode'): (r'cext', r'parsers'),
(r'cffi', r'base85'): (r'pure', r'base85'), (r'cffi', r'base85'): (r'pure', r'base85'),
(r'cffi', r'charencode'): (r'pure', r'charencode'), (r'cffi', r'charencode'): (r'pure', r'charencode'),
(r'cffi', r'diffhelpers'): (r'pure', r'diffhelpers'), (r'cffi', r'diffhelpers'): (r'pure', r'diffhelpers'),