bpo-38858: Small integer per interpreter #17315

vstinner · 2019-11-21T08:16:18Z

Each Python subinterpreter now has its own "small integer
singletons": numbers in [-5; 257] range.

For now, continue to share _PyLong_Zero and _PyLong_One singletons
between all subinterpreters.

It is no longer possible to change the number of small integers at
build time by overriden macros: pycore_pystate.h macros should now be
modified manually.

https://bugs.python.org/issue38858

vstinner · 2019-11-21T08:22:24Z

Example:

>>> from test import support
>>> support.run_in_subinterp("x=1; y=int(1.0); z=x+y; print(x is y, id(z) == id(2), f'x={id(x):x}, y={id(y):x}, z={id(z):x}')")
True True x=7fd037f5b780, y=7fd037f5b780, z=7fd037ffdec0
0
>>> x=1; y=int(1.0); z=x+y; print(x is y, id(z) == id(2), f'x={id(x):x}, y={id(y):x}, z={id(z):x}')
True True x=7fd038fd7280, y=7fd038fd7280, z=7fd038fd72c0

The subinterpreter and the main interpreter don't have the same small integer singletons for numbers 1 and 2.

vstinner · 2019-11-21T08:24:04Z

For now, continue to share _PyLong_Zero and _PyLong_One singletons
between all subinterpreters.

That's a temporary solution which I consider as an acceptable compromise. Small integer singletons are not really part of the Python semantics. It's more the opposite, we advice to not use "is" operator to compare numbers. That's why Python starts to emit such SyntaxWarning:

$ ./python
Python 3.9.0a1+ (heads/small_ints2:25aa582054, Nov 21 2019, 09:16:31) 
>>> x=1
>>> x is 1
<stdin>:1: SyntaxWarning: "is" with a literal. Did you mean "=="?
True

vstinner · 2019-11-22T11:38:24Z

I rebased my PR to fix a confict.

vstinner · 2019-11-22T17:06:59Z

Should I run a benchmark? If yes, which kind of benchmark?

ericsnowcurrently

mostly LGTM

Also, would it make sense to add any tests for this? I'm not sure it would add much.

ericsnowcurrently · 2019-11-22T23:36:17Z

Objects/longobject.c

-#ifndef NSMALLNEGINTS
-#define NSMALLNEGINTS           5
-#endif
+#define NSMALLPOSINTS           _PY_NSMALLPOSINTS


This is effectively a backward-incompatible change (though it only affects folks building their own python binary). So there should be a note in the whatsnew doc (porting section) indicating what folks must do if they want the previous behavior.

Do you really expect that anyone would recompile their Python with different NSMALLPOSINTS and NSMALLNEGINTS constants? Disabling these singletons is likely to make Python faster. I'm not sure about increasing the number of singletons. I don't expect any significant performance difference.

I completed the NEWS entry. I prefer to not document it in the What's New in Python 3.9 document. IMHO it's too low-level and obscure.

ericsnowcurrently · 2019-11-22T23:37:18Z

Objects/longobject.c

@@ -53,7 +43,8 @@ static PyObject *
 get_small_int(sdigit ival)
 {
    assert(IS_SMALL_INT(ival));
-    PyObject *v = (PyObject*)small_ints[ival + NSMALLNEGINTS];
+    PyThreadState *tstate = _PyThreadState_GET();


I'm guessing this was simpler than adding a tstate arg to get_small_int()...

I began by passing tstate to get_small_int() but I don't see any benefit, since no caller requires tstate, or already have tstate, currently. For now, it seems simpler to only get tstate inside get_small_int(). I don't expect any performance issue for now.

ericsnowcurrently · 2019-11-22T23:39:15Z

Objects/longobject.c

@@ -53,7 +43,8 @@ static PyObject *
 get_small_int(sdigit ival)
 {
    assert(IS_SMALL_INT(ival));
-    PyObject *v = (PyObject*)small_ints[ival + NSMALLNEGINTS];
+    PyThreadState *tstate = _PyThreadState_GET();
+    PyObject *v = (PyObject*)tstate->interp->small_ints[ival + NSMALLNEGINTS];


This isn't a static value any more, so is it possible to run into problems during interpreter/runtime finalization?

I'm not sure of what you mean. A subinterpreter is supposed to not leak any object to other interpreters, right? If an subinterpreter object survives after the subinterpreter is destroyed, it's a bug, no?

ericsnowcurrently · 2019-11-22T23:42:05Z

Objects/longobject.c

-    if (_PyLong_One == NULL) {
-        return 0;
-    }
+    if (_Py_IsMainInterpreter(tstate)) {


Yuck!

It would be great to have a comment here about how we would like to get rid of this special case. Bonus points if you open an issue for that and link to it in the comment. :)

I have no plan to remove _PyLong_One and _PyLong_Zero yet. It seems like there are too many things to do for subinterpreters, I am not excited by long TODO lists. They depress me, as if I will never be able to go to the end. I like to make tiny incremental changes :-)

In general, searching for "_Py_IsMainInterpreter" became a nice hint for "subinterpreters TODO".

bedevere-bot · 2019-11-22T23:43:31Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

ericsnowcurrently · 2019-11-22T23:46:04Z

benchmark? Certainly the normal suite. On top of that, it might be nice to see a "worst-case" benchmark: code that heavily exercises the code paths that have calls to get_small_int().

vstinner · 2019-12-17T11:25:03Z

benchmark? Certainly the normal suite.

Here you have.

In short, I see no performance regression (maybe speedups, but I'm not sure about these ones :-p).

pyperformance benchmark results, ignoring differences smaller than 1%:

+----------------+--------------------------------------+--------------------------------------------------+
| Benchmark      | 2019-12-16_21-50-master-f501db2b93a9 | 2019-12-16_21-50-master-f501db2b93a9-patch-17315 |
+================+======================================+==================================================+
| spectral_norm  | 261 ms                               | 237 ms: 1.10x faster (-9%)                       |
+----------------+--------------------------------------+--------------------------------------------------+
| nqueens        | 198 ms                               | 186 ms: 1.06x faster (-6%)                       |
+----------------+--------------------------------------+--------------------------------------------------+
| logging_silent | 321 ns                               | 303 ns: 1.06x faster (-6%)                       |
+----------------+--------------------------------------+--------------------------------------------------+

pyperformance benchmark results, ignoring differences smaller than 1%:

+----------------------+--------------------------------------+--------------------------------------------------+
| Benchmark            | 2019-12-16_21-50-master-f501db2b93a9 | 2019-12-16_21-50-master-f501db2b93a9-patch-17315 |
+======================+======================================+==================================================+
| spectral_norm        | 261 ms                               | 237 ms: 1.10x faster (-9%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| nqueens              | 198 ms                               | 186 ms: 1.06x faster (-6%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| logging_silent       | 321 ns                               | 303 ns: 1.06x faster (-6%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| scimark_monte_carlo  | 198 ms                               | 193 ms: 1.03x faster (-3%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| meteor_contest       | 175 ms                               | 171 ms: 1.02x faster (-2%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| unpickle             | 27.8 us                              | 27.2 us: 1.02x faster (-2%)                      |
+----------------------+--------------------------------------+--------------------------------------------------+
| pickle_list          | 6.72 us                              | 6.58 us: 1.02x faster (-2%)                      |
+----------------------+--------------------------------------+--------------------------------------------------+
| pathlib              | 41.4 ms                              | 40.6 ms: 1.02x faster (-2%)                      |
+----------------------+--------------------------------------+--------------------------------------------------+
| unpickle_pure_python | 579 us                               | 571 us: 1.01x faster (-1%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| nbody                | 233 ms                               | 230 ms: 1.01x faster (-1%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| pyflate              | 1.27 sec                             | 1.25 sec: 1.01x faster (-1%)                     |
+----------------------+--------------------------------------+--------------------------------------------------+
| sympy_expand         | 958 ms                               | 968 ms: 1.01x slower (+1%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| sqlite_synth         | 7.75 us                              | 7.84 us: 1.01x slower (+1%)                      |
+----------------------+--------------------------------------+--------------------------------------------------+
| pickle_pure_python   | 932 us                               | 942 us: 1.01x slower (+1%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| sympy_sum            | 394 ms                               | 400 ms: 1.01x slower (+1%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| regex_effbot         | 4.77 ms                              | 4.85 ms: 1.02x slower (+2%)                      |
+----------------------+--------------------------------------+--------------------------------------------------+
| mako                 | 26.7 ms                              | 27.3 ms: 1.02x slower (+2%)                      |
+----------------------+--------------------------------------+--------------------------------------------------+
| xml_etree_generate   | 158 ms                               | 162 ms: 1.03x slower (+3%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| xml_etree_iterparse  | 171 ms                               | 177 ms: 1.03x slower (+3%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| genshi_xml           | 150 ms                               | 156 ms: 1.04x slower (+4%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
| chameleon            | 20.5 ms                              | 21.3 ms: 1.04x slower (+4%)                      |
+----------------------+--------------------------------------+--------------------------------------------------+
| scimark_sor          | 353 ms                               | 370 ms: 1.05x slower (+5%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+

Each Python subinterpreter now has its own "small integer singletons": numbers in [-5; 257] range. It is no longer possible to change the number of small integers at build time by overriding NSMALLNEGINTS and NSMALLPOSINTS macros: macros should now be modified manually in pycore_pystate.h header file. For now, continue to share _PyLong_Zero and _PyLong_One singletons between all subinterpreters.

vstinner · 2019-12-17T11:35:22Z

Also, would it make sense to add any tests for this? I'm not sure it would add much.

I would prefer until we get ride of _PyLong_Zero and _PyLong_One before going up to unit tests. Right now, you may get the subinterpreter singletons, or the main interpreter singletons depending on which function is called...

This PR is a small step towards more isolated subinterpreters. It doesn't solve all issues at once ;-)

vstinner · 2019-12-17T12:03:26Z

As I wrote, I know that this change is not complete nor perfect. It's a small step towards better isolated subinterpreters. I chose to not document the backward incompatible change in What's New in Python 3.9, but only in the Changelog. IMHO it's enough, since it's really an obscure low-level feature (number of small integer singletons).

vstinner · 2019-12-17T12:05:10Z

Thanks for the review @ericsnowcurrently. I hope that I addressed most of your remarks ;-)

Each Python subinterpreter now has its own "small integer singletons": numbers in [-5; 257] range. It is no longer possible to change the number of small integers at build time by overriding NSMALLNEGINTS and NSMALLPOSINTS macros: macros should now be modified manually in pycore_pystate.h header file. For now, continue to share _PyLong_Zero and _PyLong_One singletons between all subinterpreters.

vstinner requested review from ericsnowcurrently, serhiy-storchaka and pablogsal November 21, 2019 08:16

the-knights-who-say-ni added the CLA signed label Nov 21, 2019

bedevere-bot added the awaiting core review label Nov 21, 2019

ericsnowcurrently requested changes Nov 22, 2019

View reviewed changes

bedevere-bot removed the awaiting core review label Nov 22, 2019

bedevere-bot added the awaiting changes label Nov 22, 2019

vstinner merged commit 630c8df into python:master Dec 17, 2019

vstinner deleted the small_ints2 branch December 17, 2019 12:02

bedevere-bot removed the awaiting changes label Dec 17, 2019

Uh oh!

bpo-38858: Small integer per interpreter #17315

bpo-38858: Small integer per interpreter #17315

Uh oh!

Conversation

vstinner commented Nov 21, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Nov 21, 2019

Uh oh!

vstinner commented Nov 21, 2019

Uh oh!

vstinner commented Nov 22, 2019

Uh oh!

vstinner commented Nov 22, 2019

Uh oh!

ericsnowcurrently left a comment

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!

bedevere-bot commented Nov 22, 2019

Uh oh!

ericsnowcurrently commented Nov 22, 2019

Uh oh!

vstinner commented Dec 17, 2019

Uh oh!

vstinner commented Dec 17, 2019

Uh oh!

vstinner commented Dec 17, 2019

Uh oh!

vstinner commented Dec 17, 2019

Uh oh!

Uh oh!

vstinner commented Nov 21, 2019 •

edited by bedevere-bot

Loading