Experiment b commonconst #27

iritkatriel · 2021-08-24T13:34:51Z

TODO:

~~Add the tuples~~
~~PREDICT stuff in ceval.c~~
~~fix test_modulefinder~~
~~replace LOAD_ASSERTION_ERROR~~
Review the tests to see if any are now missing

gvanrossum

Oh, that looks cool! Maybe the prediction machinery should be directed here too?

iritkatriel · 2021-08-24T14:39:22Z

the prediction machinery

The who?

gvanrossum · 2021-08-24T14:54:52Z

Macros like PREDICT() and PREDICTED().

iritkatriel · 2021-08-24T15:07:12Z

ah yes, things like this:
```
ADDOP(c, GET_YIELD_FROM_ITER);
ADDOP_LOAD_CONST(c, Py_None);


and then `GET_YIELD_FROM_ITER` predicts LOAD_CONST..

gvanrossum · 2021-08-24T16:01:24Z

So some of those will need to predict LOAD_COMMON_CONST instead. Maybe the more general LOAD_CONST no longer occurs often enough to require predicting?

iritkatriel · 2021-08-24T16:13:34Z

Yes, I'll check each case. In the case I quoted the constant is None so it's a common const for sure.

iritkatriel · 2021-08-24T16:22:50Z

I have some issues with when/where I allocated the common consts data - refleaks tests show I allocated it more than once (and if I try not to then subinterpreters don't have it). I'll have to rethink where this actually sits (I put it next to the small_ints but allocate in another place because I need the AttributeError exception etc to be ready).

I need to go soon so will continue tomorrow.

Python/common_consts.c

gvanrossum · 2021-08-25T15:46:58Z

Did you make any progress on the leak? Where you put the initialization looks fine to me (but I'm no expert on that part of the interpreter).

iritkatriel · 2021-08-25T16:09:58Z

I'm just looking at the leak. I'm moving the dict to the interpreter state alongside the array, so that they have the same lifetime. And I'm putting the alloc.dealloc alongside PyLong_Init/Fini. (I'm aiming to make this alloc/dealloc like small_ints).

That said, I'm not sure why the small_ints array is allocated on the interpreter state. Wouldn't one systemwide copy be enough?

gvanrossum · 2021-08-25T17:18:37Z

That said, I'm not sure why the small_ints array is allocated on the interpreter state. Wouldn't one systemwide copy be enough?

Presumably in preparation for the future where each interpreter has its own set of standard objects (Py_None, exceptions, types, etc.) so they can each have their own GIL. This is presumably @ericsnowcurrently's work (with Victor).

iritkatriel · 2021-08-25T23:11:22Z

I made a todo list at the top of this page.

I wanted to check that AssertionError is really a constant. The c one is (but the python one is not):

>>> AssertionError = TypeError
>>> AssertionError
<class 'TypeError'>
>>> AssertionError(14)
TypeError(14)
>>> assert(0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AssertionError
>>>

iritkatriel · 2021-08-26T14:48:50Z

This is what I broke in modulefinder:

cpython/Lib/modulefinder.py

Line 406 in 1512bc2

if (op == IMPORT_NAME and i >= 2

gvanrossum · 2021-08-26T15:15:39Z

This is what I broke in modulefinder:

cpython/Lib/modulefinder.py

Line 406 in 1512bc2

if (op == IMPORT_NAME and i >= 2

I wonder what that code is doing that requires scanning the bytecode. It seems a weird tactic.

iritkatriel · 2021-08-26T15:19:40Z

It's looking for import statements in the code, this thing is loading "all modules used by a script".

I'm just thinking all this stuff should be encapsulated in dis, nobody should be munching on opcodes like this.

gvanrossum · 2021-08-26T15:21:01Z

Right, file a bpo about that.

iritkatriel · 2021-08-26T15:26:01Z

Right, file a bpo about that.

https://bugs.python.org/issue45017

iritkatriel · 2021-08-26T21:55:01Z

Python/common_consts.c

@mdickinson This function implements obj --> key where typically key is (type(obj), obj) but for floats I also add a boolean sign.

The purpose of this PR is to reduce the size of code objects by moving common consts (see faster-cpython/ideas#65 (comment)) to a shared list from which they are looked up with LOAD_COMMON_CONSTS, instead of with LOAD_CONST from co_consts.

@tiran - this is where I applied the copysign function you suggested.

I'm not sure this works well with NaNs.

@iritkatriel This looks good to me for floats.

Do we need to handle complex constants, too? 0.0 + 0.0j should be treated as a different constant from 0.0 - 0.0j or -0.0 - 0.0j or -0.0 + 0.0j.

@tiran True, NaNs would be messy for this scheme. My initial thought was that there should be no way that you can get a NaN as a code constant, but of course that's not true:

>>> def f(): ... a = 1e300 * 1e300 ... b = (1e300 * 1e300) / (1e300 * 1e300) ... c = -1e300 * 1e300 ... >>> f.__code__.co_consts (None, inf, nan, -inf)

But presumably this would "work" the same way as it currently does for code-object-specific constant tables: NaNs get repeated, but infinities don't. @iritkatriel I don't have wider context here - what are the criteria for deciding that something is a "common" constant? I assume that it would have to occur at least twice, and in that case NaNs would just never make it into the common constant table.

So I think this should "just work" for NaNs, but it's worth some tests to check that NaNs are never shared.

Do we need to handle complex constants, too?

For reference, here's the related part of codeobject.c: https://github.com/python/cpython/blob/d3eaf0cc5b311ad023fd13e367f817d528403306/Objects/codeobject.c#L1816-L1840

It seems as though there ought to be an opportunity for code re-use here, so that we don't have essentially the same logic in multiple places. But I imagine that's out of scope for this PR.

Agreed, I can just use _PyCode_ConstantKey instead of adding a new obj_to_key. Right?

mdickinson

A couple of comments specifically on the obj_to_key part; I haven't looked at the rest of the PR.

mdickinson · 2021-08-27T08:12:24Z

Python/common_consts.c

Nitpick-level comment: I'd suggest using Py_True and Py_False the other way around: Py_False for positive and Py_True for negative, just for consistency - it's how other things that deal with the sign bit tend to work (e.g. Decimal.is_signed, or C's signbit macro, or even the raw bit representation of the float).

Talking of signbit, that might also work in place of the copysign test: signbit(v) ? Py_True : Py_False. I suspect we didn't use it in the original code because it wasn't introduced until C99, and we weren't able to depend on C99 features at the time.

mdickinson · 2021-08-27T08:23:22Z

Python/common_consts.c

@iritkatriel This looks good to me for floats.

Do we need to handle complex constants, too? 0.0 + 0.0j should be treated as a different constant from 0.0 - 0.0j or -0.0 - 0.0j or -0.0 + 0.0j.

@tiran True, NaNs would be messy for this scheme. My initial thought was that there should be no way that you can get a NaN as a code constant, but of course that's not true:

>>> def f(): ... a = 1e300 * 1e300 ... b = (1e300 * 1e300) / (1e300 * 1e300) ... c = -1e300 * 1e300 ... >>> f.__code__.co_consts (None, inf, nan, -inf)

But presumably this would "work" the same way as it currently does for code-object-specific constant tables: NaNs get repeated, but infinities don't. @iritkatriel I don't have wider context here - what are the criteria for deciding that something is a "common" constant? I assume that it would have to occur at least twice, and in that case NaNs would just never make it into the common constant table.

So I think this should "just work" for NaNs, but it's worth some tests to check that NaNs are never shared.

iritkatriel · 2021-08-27T09:22:14Z

I don't have wider context here - what are the criteria for deciding that something is a "common" constant?

It's a hard-coded list based on what constants appear the most in code objects in PyPl packages. Scroll down to _Py_InitCommonConsts to see where it is populated.

Thank you both for the comments - at the moment we don't have NaNs and complex numbers in the hard coded list, so I think it's ok if the lookup just always fails for them (as it would now, right?).

Perhaps I should add a check in add_common_float which raises an exception if you try to add NaN, Inf or complex number as a common constant, because this is currently not supported (and we don't think it is needed).

mdickinson · 2021-08-27T09:32:40Z

Thanks for the context!

so I think it's ok if the lookup just always fails for them (as it would now, right?)

Yep, sounds good to me.

iritkatriel · 2021-08-27T13:44:19Z

I don't know why it keeps saying this:

2021-08-27T13:10:22.9433879Z Generated files not up to date. Perhaps you forgot to run make regen-all or build.bat --regen ;)
2021-08-27T13:10:22.9435119Z  M Include/opcode.h
2021-08-27T13:10:22.9436000Z  M Python/opcode_targets.h
2021-08-27T13:10:22.9446511Z ##[error]Process completed with exit code 1.

Neither make regen-all nor build.bat --regen make any difference.

…ef in _Py_GetCommonConstValue)

… add NaN or Inf as common const

iritkatriel · 2021-08-27T16:03:26Z

I don't know why it keeps saying this:

2021-08-27T13:10:22.9433879Z Generated files not up to date. Perhaps you forgot to run make regen-all or build.bat --regen ;)
2021-08-27T13:10:22.9435119Z  M Include/opcode.h
2021-08-27T13:10:22.9436000Z  M Python/opcode_targets.h
2021-08-27T13:10:22.9446511Z ##[error]Process completed with exit code 1.

Neither make regen-all nor build.bat --regen make any difference.

I think it's to do with the fact that this PR is against my main branch (rather than cpython's). The build log shows that it compares the source and target branches of the PR, and those were out of sync.

markshannon · 2021-09-01T10:50:47Z

What's the justification for adding the seemingly arbitrary selection of tuples and strings rather than just a few simple constants?
Could we just start with the simple constants, and add the others later?

Why the procedural approach to building the table of constants, instead of declaring PyObject *common_consts[] = { ...?

iritkatriel · 2021-09-01T10:56:52Z

What's the justification for adding the seemingly arbitrary selection of tuples and strings rather than just a few simple constants?

This is the list Guido extracted of the common constants in PyPl:
faster-cpython/ideas#65 (comment)

Why the procedural approach to building the table of constants, instead of declaring PyObject *common_consts[] = { ...?

Error checking.

markshannon · 2021-09-02T11:28:16Z

Why the procedural approach to building the table of constants, instead of declaring PyObject *common_consts[] = { ...?

Error checking.

I don't understand. There can be no runtime errors in a declaration, only in executable code.

iritkatriel · 2021-09-02T11:36:21Z

Why the procedural approach to building the table of constants, instead of declaring PyObject *common_consts[] = { ...?

Error checking.

I don't understand. There can be no runtime errors in a declaration, only in executable code.

Are PyLong_FromLong() and PyFloat_FromFloat() not executable code?

markshannon · 2021-09-04T11:07:20Z

Are PyLong_FromLong() and PyFloat_FromFloat() not executable code?

Yes, they are. So don't use them 🙂

PyObject *common_consts[] = { Py_None, Py_Ellipsis, PyExc_AssertionError, ... };

You could add a few float constants, PyFloat_Zero, PyFloat_One.
Integers are probably best handled with their own instruction, e.g. LOAD_SMALL_INT.

gvanrossum · 2021-09-04T14:08:27Z

PyObject *common_consts[] = { Py_None, Py_Ellipsis, PyExc_AssertionError, ... };

Note that, as a static declaration, the Windows C compiler doesn’t accept that. (C++ does.)

markshannon · 2021-09-13T09:39:21Z

PyObject *common_consts[] = { Py_None, Py_Ellipsis, PyExc_AssertionError, ... };

Note that, as a static declaration, the Windows C compiler doesn’t accept that. (C++ does.)

Are you sure about that?
How does this compile?

gvanrossum · 2021-09-22T04:34:21Z

How does this compile?

Hm, it does compile. I am sure I saw a problem with references to type objects in C files (that I made go away by compiling as C++), but I must have misunderstood its root cause.

Or maybe we compile with some flag that made the error go away. I have a simple example where I initialize a struct member with &_Py_NoneStruct (i.e. the expansion of Py_None), and VS Code highlights the & character and tells me "expression must have a constant value C/C++(28)", but it compiles. So now I'm officially confused.

Possibly the root of my confusion is the common idiom of initializing type objects in extension modules using PyVarObject_HEAD_INIT(NULL, 0) or PyObject_HEAD_INIT(NULL), and then setting the actual type using PyType_Ready(). I somehow thought that was due to MSVC, but it may be due to the DLL linker or possibly needed for shared libraries in general.

Anyway, just ignore me, that's always been the best option.

gvanrossum reviewed Aug 24, 2021

View reviewed changes

methane reviewed Aug 25, 2021

View reviewed changes

Python/common_consts.c Outdated Show resolved Hide resolved

iritkatriel commented Aug 26, 2021

View reviewed changes

mdickinson reviewed Aug 27, 2021

View reviewed changes

iritkatriel added 8 commits August 27, 2021 16:54

add the MAKE_INT opcode (not used yet)

41dfaaf

fixup

f787870

generate MAKE_INT in compiler

44c83f6

fix dis and dis tests

5171894

fix ast and compile tests

879e302

fix test_import

cfc780e

fix peepholer detecing const conditionals with MAKE_INT as condition

94132c8

add opcode.is_immediate_const()

d4e9967

iritkatriel added 17 commits August 27, 2021 16:54

use _PyLong_GetSmallInt instead of PyLong_FromLong

0074245

define MAKE_INT_BIAS to be _PY_NSMALLNEGINTS

cb4ce00

add small int range to sys.int_info

f0b22ce

remove MAKE_INT_BIAS from opcode.py, use the value in sys.int_info

236bf7f

optimise

fe5c07c

MAKE_INT --> MAKE_COMMON_CONST, and use interp->common_consts for it

ee9c82c

add strings to common consts

4172e25

add floats except 0

9ef7928

add 0.0

ff8785d

sort out leak (move common_const_to_index to interp, add missing decr…

0f08385

…ef in _Py_GetCommonConstValue)

add the singletons

3500f13

replace LOAD_ASSERTION_ERROR by LOAD_COMMON_CONST

6b976cf

update prediction

788332d

fix modulefinder

1a97b0b

add the common tuples

2c2f8fc

use _PyCode_ConstantKey instead of obj_to_key. Assert we don't try to…

be33a36

… add NaN or Inf as common const

rebase and regen

743f2f9

iritkatriel force-pushed the experimentB-commonconst branch from ee4d361 to 743f2f9 Compare August 27, 2021 16:02

fix fold_tuple_on_constants

54143db

iritkatriel closed this May 20, 2022

iritkatriel deleted the experimentB-commonconst branch July 25, 2023 18:06

Experiment b commonconst #27

Experiment b commonconst #27

Uh oh!

Conversation

iritkatriel commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

iritkatriel commented Aug 24, 2021

Uh oh!

gvanrossum commented Aug 24, 2021 via email

Uh oh!

iritkatriel commented Aug 24, 2021

Uh oh!

gvanrossum commented Aug 24, 2021 via email

Uh oh!

iritkatriel commented Aug 24, 2021

Uh oh!

iritkatriel commented Aug 24, 2021

Uh oh!

Uh oh!

gvanrossum commented Aug 25, 2021

Uh oh!

iritkatriel commented Aug 25, 2021

Uh oh!

gvanrossum commented Aug 25, 2021

Uh oh!

iritkatriel commented Aug 25, 2021

Uh oh!

iritkatriel commented Aug 26, 2021

Uh oh!

gvanrossum commented Aug 26, 2021

Uh oh!

iritkatriel commented Aug 26, 2021

Uh oh!

gvanrossum commented Aug 26, 2021

Uh oh!

iritkatriel commented Aug 26, 2021

Uh oh!

iritkatriel Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

iritkatriel Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

tiran Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

mdickinson Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

mdickinson Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iritkatriel Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

mdickinson Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

mdickinson Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

iritkatriel commented Aug 27, 2021

Uh oh!

mdickinson commented Aug 27, 2021

Uh oh!

iritkatriel commented Aug 27, 2021

Uh oh!

iritkatriel commented Aug 27, 2021

Uh oh!

markshannon commented Sep 1, 2021

Uh oh!

iritkatriel commented Sep 1, 2021

iritkatriel commented Aug 24, 2021 •

edited

Loading

mdickinson Aug 27, 2021 •

edited

Loading