Skip to content
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

Emit slices as constants in the bytecode compiler #125063

Closed
mdboom opened this issue Oct 7, 2024 · 5 comments
Closed

Emit slices as constants in the bytecode compiler #125063

mdboom opened this issue Oct 7, 2024 · 5 comments
Assignees
Labels
performance Performance or resource usage type-feature A feature request or enhancement

Comments

@mdboom
Copy link
Contributor

mdboom commented Oct 7, 2024

Feature or enhancement

Proposal:

Constant slices are currently emitted as multiple bytecodes by the Python bytecode compiler.

For example, x[:-2, 1:-1] generates:

              LOAD_CONST               4 (0)
              LOAD_CONST               5 (-2)
              BUILD_SLICE              2
              LOAD_CONST               1 (1)
              LOAD_CONST               2 (-1)
              BUILD_SLICE              2
              BUILD_TUPLE              2

Then tuples of slices are constant like this, this could instead be compiled down to a single LOAD_CONST instruction:

              LOAD_CONST                1 ((slice(0, -2, None), slice(1, -1, None))

According to @fberlakovich and Stefan Brunthaler's paper on Cross module quickening, this can have a significant impact on Numpy benchmarks.

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Linked PRs

@mdboom mdboom added type-feature A feature request or enhancement performance Performance or resource usage labels Oct 7, 2024
@mdboom mdboom self-assigned this Oct 7, 2024
mdboom added a commit that referenced this issue Oct 8, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* Make slices marshallable

* Emit slices as constants

* Update Python/marshal.c

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

* Refactor codegen_slice into two functions so it
always has the same net effect

* Fix for free-threaded builds

* Simplify marshal loading of slices

* Only return SUCCESS/ERROR from codegen_slice

---------

Co-authored-by: Mark Shannon <mark@hotpy.org>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@nineteendo
Copy link
Contributor

Shouldn't this be closed now?

@mdboom mdboom closed this as completed Oct 8, 2024
@mdboom
Copy link
Contributor Author

mdboom commented Oct 8, 2024

Shouldn't this be closed now?

Yep -- sorry, I forgot that step.

efimov-mikhail pushed a commit to efimov-mikhail/cpython that referenced this issue Oct 9, 2024
…ython#125064)

* Make slices marshallable

* Emit slices as constants

* Update Python/marshal.c

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

* Refactor codegen_slice into two functions so it
always has the same net effect

* Fix for free-threaded builds

* Simplify marshal loading of slices

* Only return SUCCESS/ERROR from codegen_slice

---------

Co-authored-by: Mark Shannon <mark@hotpy.org>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@encukou
Copy link
Member

encukou commented Nov 13, 2024

Hi @mdboom,
This is missing a documentation update, since marshal docs have a list of the types that pyc can handle.

But, I wanted to ask: would it make sense to do the folding in AST, rather than in codegen? That seems like a more appropriate place, even though it probably won't help common code (e.g. folding "abc"[:1] to "a" isn't too relevant).

I quickly tried moving it, and tests pass, but I don't know if I'm missing something: main...encukou:cpython:const_slice

@encukou encukou reopened this Nov 13, 2024
@mdboom
Copy link
Contributor Author

mdboom commented Nov 13, 2024

his is missing a documentation update, since marshal docs have a list of the types that pyc can handle.

Thanks for pointing that out. I'll handle that part.

But, I wanted to ask: would it make sense to do the folding in AST, rather than in codegen?

I don't know if I have enough historical to say which is better, it does seem like doing it earlier makes sense. I'd want a second opinion about whether it's the right thing to do, though (Cc: @iritkatriel?), and if so, you should submit your branch as a PR.

@iritkatriel
Copy link
Member

But, I wanted to ask: would it make sense to do the folding in AST, rather than in codegen?

Yes, I think this makes sense.

mdboom added a commit to mdboom/cpython that referenced this issue Nov 13, 2024
encukou pushed a commit to encukou/cpython that referenced this issue Nov 14, 2024
encukou added a commit that referenced this issue Nov 15, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* Document that slices can be marshalled
* Deduplicate and organize the list of supported types
  in docs
* Organize the type code list in marshal.c, to make
  it more obvious that this is a versioned format
* Back-fill some historical info

Co-authored-by: Michael Droettboom <mdboom@gmail.com>
@encukou encukou closed this as completed Nov 15, 2024
picnixz pushed a commit to picnixz/cpython that referenced this issue Dec 8, 2024
…nGH-126829)

* Document that slices can be marshalled
* Deduplicate and organize the list of supported types
  in docs
* Organize the type code list in marshal.c, to make
  it more obvious that this is a versioned format
* Back-fill some historical info

Co-authored-by: Michael Droettboom <mdboom@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance or resource usage type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants