TODO:

1. Add the tuples
2. PREDICT stuff in ceval.c
3. fix test_modulefinder
4. replace LOAD_ASSERTION_ERROR
5. Review the tests to see if any are now missing

Choose a reason for hiding this comment

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

the prediction machinery

The who?

Macros like PREDICT() and PREDICTED().

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

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?

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

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.

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

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?

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

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
>>> 

This is what I broke in modulefinder:

if (op == IMPORT_NAME and i >= 2

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

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.

Right, file a bpo about that.

Choose a reason for hiding this comment

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

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

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.

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

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.

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[] = { ...?

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.

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.

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?

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.

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

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?

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.