-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
gh-127936: convert marshal module to use import/export API for ints (PEP 757) #128530
base: main
Are you sure you want to change the base?
Conversation
1ab0c30
to
4bd6b0c
Compare
CC @vstinner I suspect that major slowdown for marshal.loads() benchmarks is due to normalization&singletonization in the PyLongWriter_Finish(). In case of marshal.dumps() I suspect things were better without the |
assert(layout->digit_endianness == (PY_LITTLE_ENDIAN ? -1 : 1)); | ||
assert(layout->digit_size == 2 || layout->digit_size == 4); | ||
|
||
Py_ssize_t size = 1 + (Py_ABS(n) - 1) / marshal_ratio; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need a special case for n==0, like the code that you removed, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think no, it's just size=0
size=1
.
Edit: But 0
has one digit allocated anyway. So, all works correctly:
>>> import marshal
>>> marshal.dumps(0)
b'\xe9\x00\x00\x00\x00'
>>> marshal.loads(marshal.dumps(0))
0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If n==0, 1 + (Py_ABS(n) - 1) / marshal_ratio
gives size=0
, no?
size=0
is annoying. _w_digitsXX()
functions use size - 1
for example.
Please keep if (n == 0) return (PyObject *)_PyLong_New(0);
to avoid this annoying case.
But 0 has one digit allocated anyway. So, all works correctly:
Ok, but I'm thinking about invalid/special marshal data. I would prefer to avoid a crash if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if n==0, 1 + (Py_ABS(n) - 1) / marshal_ratio gives size=0, no?
No. (-1)/positive is 0 in C.
Please keep if (n == 0) return (PyObject *)_PyLong_New(0)
Then we will depend on this import.
I'm thinking about invalid/special marshal data.
An example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. (-1)/positive is 0 in C.
Oh, I forgot that. It might help future readers to add a comment explaining that size=1 if n=0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add also an assertion: assert(size >= 1)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'm (just) a bit unfortunate that the using PEP 757 makes the code slower.
@serhiy-storchaka @picnixz: Would you mind to review this change?
Benchmark hidden because not significant (1): dumps 1<<7
scripts