◐ Shell
reader mode source ↗
Skip to content

bpo-38858: Small integer per interpreter#17315

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:small_ints2
Dec 17, 2019
Merged

bpo-38858: Small integer per interpreter#17315
vstinner merged 1 commit into
python:masterfrom
vstinner:small_ints2

Conversation

@vstinner

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

Copy link
Copy Markdown
Member

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

vstinner commented Nov 21, 2019

Copy link
Copy Markdown
Member Author

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

vstinner commented Nov 21, 2019

Copy link
Copy Markdown
Member Author

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 commented Nov 22, 2019

Copy link
Copy Markdown
Member Author

I rebased my PR to fix a confict.

@vstinner

vstinner commented Nov 22, 2019

Copy link
Copy Markdown
Member Author

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

ericsnowcurrently
ericsnowcurrently requested changes Nov 22, 2019
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

mostly LGTM

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

@bedevere-bot

bedevere-bot commented Nov 22, 2019

Copy link
Copy Markdown

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

@ericsnowcurrently

ericsnowcurrently commented Nov 22, 2019

Copy link
Copy Markdown
Member

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 commented Dec 17, 2019

Copy link
Copy Markdown
Member Author

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 …
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 commented Dec 17, 2019

Copy link
Copy Markdown
Member Author

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
@vstinner vstinner deleted the small_ints2 branch December 17, 2019 12:02
@vstinner

vstinner commented Dec 17, 2019

Copy link
Copy Markdown
Member Author

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

vstinner commented Dec 17, 2019

Copy link
Copy Markdown
Member Author

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

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@vstinner @shihai1991
bpo-38858: Small integer per interpreter (pythonGH-17315) …
152367c 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently requested changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

@pablogsal pablogsal

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vstinner @bedevere-bot @ericsnowcurrently @the-knights-who-say-ni