Skip to content

gh-105699: Use a _Py_hashtable_t for the PyModuleDef Cache #106974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ericsnowcurrently merged 14 commits into from
Jul 28, 2023
Merged

gh-105699: Use a _Py_hashtable_t for the PyModuleDef Cache #106974

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3397777
Add a comment.
ericsnowcurrently Jul 19, 2023
670e632
Add a TODO.
ericsnowcurrently Jul 19, 2023
485d130
Use a low-level hashtable instead of a dict.
ericsnowcurrently Jul 21, 2023
77d5e8f
Drop _PyThreadState_*Detached().
ericsnowcurrently Jul 19, 2023
04a19c4
Set "byname" properly for the reload case.
ericsnowcurrently Jul 24, 2023
5fde127
Use a flat hashtable instead of a nested one.
ericsnowcurrently Jul 27, 2023
818d828
Merge branch 'main' into fix-extensions-tstate-alt
ericsnowcurrently Jul 27, 2023
e83b0cc
Use the right allocator.
ericsnowcurrently Jul 27, 2023
9e81cd5
Use PyUnicode_AsUTF8AndSize() (and fix the assert).
ericsnowcurrently Jul 28, 2023
430716b
Raise MemoryError where appropriate.
ericsnowcurrently Jul 28, 2023
647fbc2
Add a comment.
ericsnowcurrently Jul 28, 2023
bbe5eda
Use a constant for the cache separator character.
ericsnowcurrently Jul 28, 2023
1d7cbd0
Decref when clearing the hashtable.
ericsnowcurrently Jul 28, 2023
20c8b9d
Make the cached module defs immortal.
ericsnowcurrently Jul 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions Include/internal/pycore_import.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
extern "C" {
#endif

#include "pycore_hashtable.h" // _Py_hashtable_t
#include "pycore_time.h" // _PyTime_t

extern int _PyImport_IsInitialized(PyInterpreterState *);
Expand Down Expand Up @@ -37,19 +38,15 @@ struct _import_runtime_state {
See PyInterpreterState.modules_by_index for more info. */
Py_ssize_t last_module_index;
struct {
/* A thread state tied to the main interpreter,
used exclusively for when the extensions dict is access/modified
from an arbitrary thread. */
PyThreadState main_tstate;
/* A lock to guard the dict. */
/* A lock to guard the cache. */
PyThread_type_lock mutex;
/* A dict mapping (filename, name) to PyModuleDef for modules.
/* The actual cache of (filename, name, PyModuleDef) for modules.
Only legacy (single-phase init) extension modules are added
and only if they support multiple initialization (m_size >- 0)
or are imported in the main interpreter.
This is initialized lazily in _PyImport_FixupExtensionObject().
Modules are added there and looked up in _imp.find_extension(). */
PyObject *dict;
_Py_hashtable_t *hashtable;
} extensions;
/* Package context -- the full module name for package imports */
const char * pkgcontext;
Expand Down
5 changes: 0 additions & 5 deletions Include/internal/pycore_pystate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,6 @@ PyAPI_FUNC(PyThreadState *) _PyThreadState_New(PyInterpreterState *interp);
PyAPI_FUNC(void) _PyThreadState_Bind(PyThreadState *tstate);
PyAPI_FUNC(void) _PyThreadState_DeleteExcept(PyThreadState *tstate);

extern void _PyThreadState_InitDetached(PyThreadState *, PyInterpreterState *);
extern void _PyThreadState_ClearDetached(PyThreadState *);
extern void _PyThreadState_BindDetached(PyThreadState *);
extern void _PyThreadState_UnbindDetached(PyThreadState *);

PyAPI_FUNC(PyObject*) _PyThreadState_GetDict(PyThreadState *tstate);

/* The implementation of sys._current_frames() Returns a dict mapping
Expand Down
5 changes: 0 additions & 5 deletions Include/internal/pycore_runtime_init.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,6 @@ extern PyTypeObject _PyExc_MemoryError;
in accordance with the specification. */ \
.autoTSSkey = Py_tss_NEEDS_INIT, \
.parser = _parser_runtime_state_INIT, \
.imports = { \
.extensions = { \
.main_tstate = _PyThreadState_INIT, \
}, \
}, \
.ceval = { \
.perf = _PyEval_RUNTIME_PERF_INIT, \
}, \
Expand Down
216 changes: 113 additions & 103 deletions Python/import.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include "Python.h"

#include "pycore_hashtable.h" // _Py_hashtable_new_full()
#include "pycore_import.h" // _PyImport_BootstrapImp()
#include "pycore_initconfig.h" // _PyStatus_OK()
#include "pycore_interp.h" // struct _import_runtime_state
Expand Down Expand Up @@ -916,13 +917,28 @@ extensions_lock_release(void)
dictionary, to avoid loading shared libraries twice.
*/

static Py_uhash_t
hashtable_hash_str(const void *key)
{
return _Py_HashBytes(key, strlen((const char *)key));
}

static int
hashtable_compare_str(const void *key1, const void *key2)
{
return strcmp((const char *)key1, (const char *)key2) == 0;
}

static void
_extensions_cache_init(void)
hashtable_destroy_str(void *ptr)
{
/* The runtime (i.e. main interpreter) must be initializing,
so we don't need to worry about the lock. */
_PyThreadState_InitDetached(&EXTENSIONS.main_tstate,
_PyInterpreterState_Main());
PyMem_RawFree(ptr);
}

static void
hashtable_destroy_str_table(void *ptr)
{
_Py_hashtable_destroy((_Py_hashtable_t *)ptr);
}

static PyModuleDef *
Expand All @@ -931,19 +947,24 @@ _extensions_cache_get(PyObject *filename, PyObject *name)
PyModuleDef *def = NULL;
extensions_lock_acquire();

PyObject *key = PyTuple_Pack(2, filename, name);
if (key == NULL) {
if (EXTENSIONS.hashtable == NULL) {
goto finally;
}

PyObject *extensions = EXTENSIONS.dict;
if (extensions == NULL) {
_Py_hashtable_entry_t *entry;
entry = _Py_hashtable_get_entry(EXTENSIONS.hashtable,
PyUnicode_AsUTF8(filename));
if (entry == NULL) {
goto finally;
}
entry = _Py_hashtable_get_entry((_Py_hashtable_t *)entry->value,
PyUnicode_AsUTF8(name));
if (entry == NULL) {
goto finally;
}
def = (PyModuleDef *)PyDict_GetItemWithError(extensions, key);
def = (PyModuleDef *)entry->value;

finally:
Py_XDECREF(key);
extensions_lock_release();
return def;
}
Expand All @@ -952,134 +973,125 @@ static int
_extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def)
{
int res = -1;
PyThreadState *oldts = NULL;
extensions_lock_acquire();

/* Swap to the main interpreter, if necessary. This matters if
the dict hasn't been created yet or if the item isn't in the
dict yet. In both cases we must ensure the relevant objects
are created using the main interpreter. */
PyThreadState *main_tstate = &EXTENSIONS.main_tstate;
PyInterpreterState *interp = _PyInterpreterState_GET();
if (!_Py_IsMainInterpreter(interp)) {
_PyThreadState_BindDetached(main_tstate);
oldts = _PyThreadState_Swap(interp->runtime, main_tstate);
assert(!_Py_IsMainInterpreter(oldts->interp));

/* Make sure the name and filename objects are owned
by the main interpreter. */
name = PyUnicode_InternFromString(PyUnicode_AsUTF8(name));
assert(name != NULL);
filename = PyUnicode_InternFromString(PyUnicode_AsUTF8(filename));
assert(filename != NULL);
if (EXTENSIONS.hashtable == NULL) {
EXTENSIONS.hashtable = _Py_hashtable_new_full(
Comment thread
ericsnowcurrently marked this conversation as resolved.
hashtable_hash_str,
hashtable_compare_str,
hashtable_destroy_str,
hashtable_destroy_str_table,
NULL
);
if (EXTENSIONS.hashtable == NULL) {
goto finally;
}
}

PyObject *key = PyTuple_Pack(2, filename, name);
if (key == NULL) {
goto finally;
_Py_hashtable_t *byname;
_Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(
EXTENSIONS.hashtable, PyUnicode_AsUTF8(filename));
if (entry == NULL) {
byname = _Py_hashtable_new_full(
hashtable_hash_str,
hashtable_compare_str,
hashtable_destroy_str,
NULL,
NULL
);
if (byname == NULL) {
goto finally;
}
char *filenamestr = PyMem_RawMalloc(
strlen(PyUnicode_AsUTF8(filename)) + 1);
if (filenamestr == NULL) {
_Py_hashtable_destroy(byname);
goto finally;
}
strncpy(filenamestr, PyUnicode_AsUTF8(filename),
strlen(PyUnicode_AsUTF8(filename)) + 1);
if (_Py_hashtable_set(EXTENSIONS.hashtable, filenamestr, byname) < 0) {
_Py_hashtable_destroy(byname);
PyMem_RawFree(filenamestr);
goto finally;
}
}

PyObject *extensions = EXTENSIONS.dict;
if (extensions == NULL) {
extensions = PyDict_New();
if (extensions == NULL) {
else {
byname = (_Py_hashtable_t *)entry->value;
_Py_hashtable_entry_t *subentry = _Py_hashtable_get_entry(
byname, PyUnicode_AsUTF8(name));
if (subentry != NULL) {
if (subentry->value == NULL) {
subentry->value = Py_NewRef(def);
}
else {
/* We expect it to be static, so it must be the same pointer. */
assert((PyModuleDef *)subentry->value == def);
}
res = 0;
goto finally;
}
EXTENSIONS.dict = extensions;
}

PyModuleDef *actual = (PyModuleDef *)PyDict_GetItemWithError(extensions, key);
if (PyErr_Occurred()) {
char *namestr = PyMem_RawMalloc(strlen(PyUnicode_AsUTF8(name)) + 1);
if (namestr == NULL) {
goto finally;
}
else if (actual != NULL) {
/* We expect it to be static, so it must be the same pointer. */
assert(def == actual);
res = 0;
goto finally;
}

/* This might trigger a resize, which is why we must switch
to the main interpreter. */
res = PyDict_SetItem(extensions, key, (PyObject *)def);
if (res < 0) {
res = -1;
strncpy(namestr, PyUnicode_AsUTF8(name), strlen(PyUnicode_AsUTF8(name)) + 1);
if (_Py_hashtable_set(byname, namestr, Py_NewRef(def)) < 0) {
PyMem_RawFree(namestr);
goto finally;
}
res = 0;

finally:
Py_XDECREF(key);
if (oldts != NULL) {
_PyThreadState_Swap(interp->runtime, oldts);
_PyThreadState_UnbindDetached(main_tstate);
Py_DECREF(name);
Py_DECREF(filename);
}
extensions_lock_release();
return res;
}

static int
static void
_extensions_cache_delete(PyObject *filename, PyObject *name)
{
int res = -1;
PyThreadState *oldts = NULL;
extensions_lock_acquire();

PyObject *key = PyTuple_Pack(2, filename, name);
if (key == NULL) {
goto finally;
}

PyObject *extensions = EXTENSIONS.dict;
if (extensions == NULL) {
res = 0;
if (EXTENSIONS.hashtable == NULL) {
/* It was never added. */
goto finally;
}

PyModuleDef *actual = (PyModuleDef *)PyDict_GetItemWithError(extensions, key);
if (PyErr_Occurred()) {
_Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(
EXTENSIONS.hashtable, PyUnicode_AsUTF8(filename));
if (entry == NULL) {
/* It was never added. */
goto finally;
}
else if (actual == NULL) {
/* It was already removed or never added. */
res = 0;
_Py_hashtable_entry_t *subentry = _Py_hashtable_get_entry(
(_Py_hashtable_t *)entry->value, PyUnicode_AsUTF8(name));
if (subentry == NULL) {
/* It never added. */
goto finally;
}

/* Swap to the main interpreter, if necessary. */
PyThreadState *main_tstate = &EXTENSIONS.main_tstate;
PyInterpreterState *interp = _PyInterpreterState_GET();
if (!_Py_IsMainInterpreter(interp)) {
_PyThreadState_BindDetached(main_tstate);
oldts = _PyThreadState_Swap(interp->runtime, main_tstate);
assert(!_Py_IsMainInterpreter(oldts->interp));
}

if (PyDict_DelItem(extensions, key) < 0) {
if (subentry->value == NULL) {
/* It was already removed. */
goto finally;
}
res = 0;
/* This decref would be problematic if the module def were
Copy link
Copy Markdown
Member

@Yhg1s Yhg1s Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That brings up a good point: shouldn't the PyModuleDefs placed in the shared cache be made immortal? Otherwise how can they sensibly be shared at all?

Copy link
Copy Markdown
Member Author

@ericsnowcurrently ericsnowcurrently Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. We don't use module defs like the usual objects and generally assume they are statically defined, but it would be good to play it safe.

Copy link
Copy Markdown
Member Author

@ericsnowcurrently ericsnowcurrently Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

dynamically allocated, it were the last ref, and this function
were called with an interpreter other than the def's owner. */
Py_DECREF(subentry->value);
subentry->value = NULL;

finally:
if (oldts != NULL) {
_PyThreadState_Swap(interp->runtime, oldts);
_PyThreadState_UnbindDetached(main_tstate);
}
Py_XDECREF(key);
extensions_lock_release();
return res;
}

static void
_extensions_cache_clear_all(void)
{
/* The runtime (i.e. main interpreter) must be finalizing,
so we don't need to worry about the lock. */
// XXX assert(_Py_IsMainInterpreter(_PyInterpreterState_GET()));
Py_CLEAR(EXTENSIONS.dict);
_PyThreadState_ClearDetached(&EXTENSIONS.main_tstate);
_Py_hashtable_destroy(EXTENSIONS.hashtable);
EXTENSIONS.hashtable = NULL;
}


Expand Down Expand Up @@ -1233,6 +1245,8 @@ import_find_extension(PyThreadState *tstate, PyObject *name,
PyObject *m_copy = def->m_base.m_copy;
/* Module does not support repeated initialization */
if (m_copy == NULL) {
/* It might be a core module (e.g. sys & builtins),
for which we don't set m_copy. */
m_copy = get_core_module_dict(tstate->interp, name, filename);
if (m_copy == NULL) {
return NULL;
Expand Down Expand Up @@ -1302,9 +1316,7 @@ clear_singlephase_extension(PyInterpreterState *interp,
}

/* Clear the cached module def. */
if (_extensions_cache_delete(filename, name) < 0) {
return -1;
}
_extensions_cache_delete(filename, name);

return 0;
}
Expand Down Expand Up @@ -3053,6 +3065,8 @@ void
_PyImport_Fini(void)
{
/* Destroy the database used by _PyImport_{Fixup,Find}Extension */
// XXX Should we actually leave them (mostly) intact, since we don't
// ever dlclose() the module files?
_extensions_cache_clear_all();

/* Use the same memory allocator as _PyImport_Init(). */
Expand Down Expand Up @@ -3090,10 +3104,6 @@ _PyImport_Fini2(void)
PyStatus
_PyImport_InitCore(PyThreadState *tstate, PyObject *sysmod, int importlib)
{
if (_Py_IsMainInterpreter(tstate->interp)) {
_extensions_cache_init();
}

// XXX Initialize here: interp->modules and interp->import_func.
// XXX Initialize here: sys.modules and sys.meta_path.

Expand Down
Loading