bpo-29881: Add new C API for variables init once by vstinner · Pull Request #780 · python/cpython

vstinner · 2017-03-23T10:52:31Z

New C API for variables only initialized once to be able to clear
them at exit:

New macro _Py_ONCEVAR(var) to declare a variable
New macro _PY_ONCEVAR_INIT(var, expr) to initialize a variable
once
New function _PyOnceVar_Set() to explicitly set a variable once
to initialize it
New _PyOnceVar_Fini() function clearing all variables (initialized
once) at exit

https://bugs.python.org/issue29881

mention-bot · 2017-03-23T10:52:36Z

@Haypo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @loewis, @benjaminp and @larryhastings to be potential reviewers.

serhiy-storchaka · 2017-03-23T11:26:22Z

Objects/object.c

Needed spaces around =.

And I think it would be better to clear resources in the reversed order.

serhiy-storchaka · 2017-03-23T11:27:40Z

Objects/object.c

I think the type of array should be PyObject ***.

serhiy-storchaka · 2017-03-23T11:29:57Z

Objects/object.c

The static variable should be cleared.

serhiy-storchaka · 2017-03-23T11:32:51Z

Objects/object.c

Just for the case if the destructor of freed value triggered _PyOnceVar_Set() it would be safe to save the array in the local variable and clear the once_variables structure before calling destructors.

serhiy-storchaka · 2017-03-23T11:33:40Z

Include/object.h

Is this used?

oops, remove

serhiy-storchaka · 2017-03-23T13:29:29Z

Include/object.h

"this this"

serhiy-storchaka · 2017-03-23T13:30:23Z

Include/object.h

PyObject *var rather than PyObject* var.

macro removed

serhiy-storchaka · 2017-03-23T13:35:22Z

Modules/arraymodule.c

Actually we can use PyObject_GetAttrString(). In any case this is called only once. No need to keep _array_reconstructo in a list of identifiers.

serhiy-storchaka · 2017-03-23T13:42:31Z

Objects/object.c

If set once_variables.len to PY_SSIZE_T_MAX / (Py_ssize_t)sizeof(PyObject *) the following call of _PyOnceVar_Set() will fail. once_variables.len can be set to 0 after finalizing static variables.

If for some reasons, a variable is initialized again: I prefer to not fail in a Python binary compiled in release mode. It's not a big deal. We already allow to import modules during Python shutdown...

It is better to fail destructing one particular object and complain in PyErr_WriteUnraisable() than crash in assert(once_variables.array == NULL); far from the place of error.

It must be possible to initialize again a variable if Python is reinitialized. It's exactly the test done by test_embed. I fixed your concern differently: I removed the assertion and added a second call to _PyOnceVar_Fini(). In practice, a third call might be needed... but I prefer to move step by step, and see later how to handle this corner case.

serhiy-storchaka · 2017-03-23T13:44:49Z

Objects/object.c

Py_CLEAR() is safer (for common reasons of using Py_CLEAR). Or Py_SETREF(*var, NULL) if you want to save one testing for NULL.

Ok for Py_CLEAR

serhiy-storchaka · 2017-03-23T14:59:42Z

Objects/unicodeobject.c

Why not just PyUnicode_InternFromString()?

Actually _PyUnicode_FromId() can be implemented with _PY_ONCEVAR_INIT():

if (_PY_ONCEVAR_INIT(id->object, PyUnicode_InternFromString(id->string)) < 0) return NULL; return id->object;

Nice, done.

serhiy-storchaka · 2017-03-23T15:04:13Z

Objects/object.c

Since calculating value is not an atomic operation (it can involve even importing) there is possible a race condition. If *var is not NULL, just decref value and return 0.

While I agree that the case can occur in practice, I'm not confortable with supporting "reentrant calls" (reentrant calls to _PYONCEVAR_INIT() to be exact). I prefer to start with an assertion, and decide how to handle this corner case if it occurs in practice.

If reentrant calls occurs, to be clear: it would already be a bug in CPython, right? So my code doesn't make CPython worse. It's better, since the assertion now detects the bug.

New C API for variables only initialized once to be able to clear them at exit: * New macro _PY_ONCEVAR_INIT(var, expr) to initialize a variable once * New function _PyOnceVar_Set() to explicitly set a variable once to initialize it * New _PyOnceVar_Fini() function clearing all variables (initialized once) at exit

Rewrite _PyUnicode_FromId() using _PY_ONCEVAR_INIT().

serhiy-storchaka · 2017-03-23T18:17:53Z

Objects/boolobject.c

-            (false_str = PyUnicode_InternFromString("False"));
-    Py_XINCREF(s);
+    if (self == Py_True) {
+        if (_PY_ONCEVAR_INIT(true_str, PyUnicode_InternFromString("True"))) {


If make _PY_ONCEVAR_INIT() returning the value (NULL in case of error) the code would be simpler.

if (self == Py_True) { s = _PY_ONCEVAR_INIT(true_str, PyUnicode_InternFromString("True")); } else { s = _PY_ONCEVAR_INIT(false_str, PyUnicode_InternFromString("False")); } Py_XINCREF(s); return s;

the-knights-who-say-ni added the CLA signed label Mar 23, 2017

serhiy-storchaka reviewed Mar 23, 2017

View reviewed changes

vstinner mentioned this pull request Mar 23, 2017

bpo-29881: Add new C API for static variables #770

Closed

serhiy-storchaka reviewed Mar 23, 2017

View reviewed changes

vstinner added 3 commits March 23, 2017 18:44

Remove _Py_Identifier.next and static_strings

51de95a

Rewrite _PyUnicode_FromId() using _PY_ONCEVAR_INIT().

Use _PY_ONCEVAR_INIT() in modules

31be490

serhiy-storchaka reviewed Mar 23, 2017

View reviewed changes

Merge branch 'master' into staticvar_list

0e49962

Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017

vstinner closed this Feb 1, 2018

vstinner deleted the staticvar_list branch May 29, 2018 22:30

Uh oh!

Conversation

vstinner commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vstinner commented Mar 23, 2017 •

edited

Loading