Skip to content

/cpython

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.

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

bpo-38858: Small integer per interpreter #17315

Merged
merged 1 commit into from Dec 17, 2019
Merged

bpo-38858: Small integer per interpreter #17315

merged 1 commit into from Dec 17, 2019
+56 −37

Conversation

@vstinner
Copy link
Member

vstinner commented Nov 21, 2019 •
edited by bedevere-bot

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Nov 21, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Nov 21, 2019

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 vstinner force-pushed the vstinner:small_ints2 branch from 25aa582 to 8fc30c2 Nov 22, 2019
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Nov 22, 2019

I rebased my PR to fix a confict.
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Nov 22, 2019

Should I run a benchmark? If yes, which kind of benchmark?
@ericsnowcurrently
ericsnowcurrently requested changes Nov 22, 2019
View changes
Copy link
Member

ericsnowcurrently left a comment

mostly LGTM

Also, would it make sense to add any tests for this? I'm not sure it would add much.
Objects/longobject.c
#ifndef NSMALLNEGINTS
#define NSMALLNEGINTS 5
#endif
#define NSMALLPOSINTS _PY_NSMALLPOSINTS

This comment has been minimized.

Sign in to view
Copy link
@ericsnowcurrently

ericsnowcurrently Nov 22, 2019

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Dec 17, 2019

Author Member

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.

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Dec 17, 2019

Author Member

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.
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();

This comment has been minimized.

Sign in to view
Copy link
@ericsnowcurrently

ericsnowcurrently Nov 22, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Dec 17, 2019

Author Member

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.
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 comment has been minimized.

Sign in to view
Copy link
@ericsnowcurrently

ericsnowcurrently Nov 22, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Dec 16, 2019

Author Member

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?
Objects/longobject.c
if (_PyLong_One == NULL) {
return 0;
}
if (_Py_IsMainInterpreter(tstate)) {

This comment has been minimized.

Sign in to view
Copy link
@ericsnowcurrently

ericsnowcurrently Nov 22, 2019

Member

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. :)

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Dec 17, 2019

Author Member

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

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Nov 22, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

ericsnowcurrently commented Nov 22, 2019

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 vstinner force-pushed the vstinner:small_ints2 branch from 8fc30c2 to e8373ee Dec 17, 2019
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Dec 17, 2019

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%)                       |
+----------------------+--------------------------------------+--------------------------------------------------+
@vstinner
bpo-38858: Small integer per interpreter
Loading status checks…
8e07a92 
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 vstinner force-pushed the vstinner:small_ints2 branch from e8373ee to 8e07a92 Dec 17, 2019
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Dec 17, 2019

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 vstinner merged commit 630c8df into python:master Dec 17, 2019
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20191217.12 succeeded
Details
bedevere/issue-number Issue number 38858 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vstinner vstinner deleted the vstinner:small_ints2 branch Dec 17, 2019
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Dec 17, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

vstinner commented Dec 17, 2019

Thanks for the review @ericsnowcurrently. I hope that I addressed most of your remarks ;-)
sthagen added a commit to sthagen/cpython that referenced this pull request Dec 17, 2019
@sthagen
Merge pull request #106 from python/master
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
508cc80 
bpo-38858: Small integer per interpreter (pythonGH-17315)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericsnowcurrently ericsnowcurrently

@serhiy-storchaka serhiy-storchaka

@pablogsal pablogsal

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
4 participants
@vstinner @bedevere-bot @ericsnowcurrently @the-knights-who-say-ni
You can’t perform that action at this time.