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

Simplifying and enhancing const_key #54

Merged

Conversation

thautwarm
Copy link
Contributor

@thautwarm thautwarm commented Jan 31, 2020

Explanation

using marshal.dumps for const_key

In CPython, code objects compiled from a source code file, are able to get serialized into binary contents via marshal.dumps.

This gives us some useful observations:

Constants(all elements in code.co_consts) of every code object, are also able to get serialized into binary contents via marshal.dumps.

marshal.dumps must be able to distinguish data of different types and values, even for something we might make mistakes on, e.g., #32 (comment), and keep the property of data with structural equality. This is because, marshal.dumps is a foundation of CPython: CPython relies on this to keep the same behaviour of .py and .pyc.

Hence, marshal.dumps can precisely give us a perfect encoding that can distinguish objects from each other and keep the equality of structure data, for all constants supported by Python. On the other side, the encoding itself is a simple bytes, thus, if we just want to keep the
same behaviour as CPython's, marshal.dumps can sufficiently become an implementation of the function const_key mentioned in #51 .

def const_key(const):
    return marshal.dumps(const)
# or const_key = marshal.dumps

This already keeps the total compatibility to CPython.

More general constants in bytecode, and compatibility

However, for this good reason given by @SnoopyLane , this will be very beneficial:

Let the bytecode library support construction of valid bytecodes, even if that means the result is not valid Python.

Hence we can also support non-marshal-able constants in code objects.

This time, my proposal for this extension is,

def const_key(const):
    try:
        marshal.dumps(const)
    except ValueError: # un-marshal-able
        return type(const), id(const)

type(const), id(const) is also the same strategy used before: https://github.com/vstinner/bytecode/blob/45d643f6a706ae95586a874ea0fbcb18e797bfa1/bytecode/instr.py#L37 .

Advantage of this implementation

  1. No need any more to synchronize our code for const_key with _PyCode_ConstantKey, but keep full compatibility to CPython.

  2. A little more efficient when we're calculating keys for tuple or other built-in structural data.

  3. Code size for const_key gets reduced, also, imports of modules like math and types can get eliminated.

@codecov-io
Copy link

codecov-io commented Jan 31, 2020

Codecov Report

Merging #54 into master will increase coverage by 1.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
+ Coverage   93.18%   94.27%   +1.09%     
==========================================
  Files          17       17              
  Lines        2830     2847      +17     
==========================================
+ Hits         2637     2684      +47     
+ Misses        193      163      -30
Impacted Files Coverage Δ
bytecode/instr.py 94.3% <100%> (+0.67%) ⬆️
bytecode/tests/test_instr.py 98.78% <100%> (+0.16%) ⬆️
bytecode/cfg.py 96.92% <0%> (+0.01%) ⬆️
bytecode/concrete.py 96.75% <0%> (+0.78%) ⬆️
bytecode/bytecode.py 91.71% <0%> (+2.1%) ⬆️
bytecode/tests/test_cfg.py 99.67% <0%> (+3.25%) ⬆️
bytecode/tests/test_peephole_opt.py 96.34% <0%> (+5.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d93b01...dddf3ec. Read the comment docs.

@thautwarm
Copy link
Contributor Author

fix #51

@MatthieuDartiailh
Copy link
Owner

Could you please add tests covering #51 to ensure the new behavior fixes it ? And please update the changelog too.

@thautwarm
Copy link
Contributor Author

@MatthieuDartiailh Done. Sorry for the late PR.

@MatthieuDartiailh MatthieuDartiailh merged commit 119e75a into MatthieuDartiailh:master Jan 31, 2020
@MatthieuDartiailh
Copy link
Owner

Thanks ! I will do my best to release over the week-end.

@thautwarm
Copy link
Contributor Author

@MatthieuDartiailh Thank you, and take your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants