GH-137821: Convert _json to use Argument Clinic
#138869
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
I will plan to follow up her PR during the core sprint. |
| } | ||
|
|
||
| static inline int | ||
| _encoder_iterate_mapping_lock_held(PyEncoderObject *s, PyUnicodeWriter *writer, |
There was a problem hiding this comment.
Hmm, Would you like to explain why you remove xxxx_lock_held functions?
It's implemented for thread-safy in free-threading.
There was a problem hiding this comment.
I didn't delete it myself; it got deleted when I ran make clinic.
Is it wrong to modify the code and then run make clinic?
There was a problem hiding this comment.
I didn't delete it myself; it got deleted when I ran make clinic.
Is it wrong to modify the code and then run make clinic?
You should run make clinic first and then fill the declaration :)
But in that case, I think you should revert some change.
| } | ||
|
|
||
| static inline int | ||
| _encoder_iterate_fast_seq_lock_held(PyEncoderObject *s, PyUnicodeWriter *writer, |
| } | ||
|
|
||
| static inline int | ||
| _encoder_iterate_dict_lock_held(PyEncoderObject *s, PyUnicodeWriter *writer, |
There was a problem hiding this comment.
About benchmark, you have to install pyperf with your newly builded CPython
$> ./python.exe -m venv .venv
$> source .venv/bin/activate
$> pip install pyperf
And then write benchmark in this way (This is just example, I expect more possible data you can expect)
import pyperf
import _json
small_obj = '{"foo": "bar"}'
def bench_encode_basestring_ascii(s):
return _json.encode_basestring_ascii(s)
runner = pyperf.Runner()
runner.bench_func("bench_encode_basestring_ascii_small", bench_encode_basestring_ascii, small_obj)Finally you can run benchmark with following way.
$> python bench_json.py -o base.json (with main branch)
$> python bench_json.py -o ac.json (with your working branch)
$> pyperf compare_to base.json ac.json (will show your benchmark comparation)
I received the following execution results. Since the code logic hasn't changed, I think the benchmark results should be the same and the result below is correct. Is that right? |
We suspsect the possibiity of overhead that are generated by AC but it looks not. But you also need to to write benchmark code for |
| ); | ||
| /*[clinic input] | ||
| _json.scanstring | ||
| pystr: object |
There was a problem hiding this comment.
You can use string as pystr to preserve original parameter name in Python. Thought, it's a positional-only argument and it's less important.
| "character in s after the quote that started the JSON string.\n" | ||
| "Unescapes all valid JSON string escape sequences and raises ValueError\n" | ||
| "on attempt to decode an invalid string. If strict is False then literal\n" | ||
| "control characters are allowed in the string.\n" |
There was a problem hiding this comment.
Wait.
This paragraph of docstring is entirely lost. I think you shouldn't alter docstrings, except for case where you need to split out "summary line", per PEP 257. Everything else should go to a separate pr.
| if (PyCFunction_Check(s->encoder)) { | ||
| PyCFunction f = PyCFunction_GetFunction(s->encoder); | ||
| if (f == py_encode_basestring_ascii) { | ||
| if (f == _json_encode_basestring_ascii) { |
There was a problem hiding this comment.
Again, you can rename generated function to keep old name:
/*[clinic input]
_json.encode_basestring_ascii as py_encode_basestring_ascii
...
| ident = NULL; | ||
| s_fast = PySequence_Fast(seq, "_iterencode_list needs a sequence"); |
There was a problem hiding this comment.
Why is this? I think it should be reverted, just as _encoder_iterate_fast_seq_lock_held() removal below.
|
@skirpichev I am now mentoring her, so I will ping you once she is ready to get review :) It's not ready now. |
4 participants
#137821