GH-137821: Convert _json to use Argument Clinic#138869
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
corona10 commented Sep 13, 2025
I will plan to follow up her PR during the core sprint. |
| } | ||
| staticinlineint | ||
| _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.
| } | ||
| staticinlineint | ||
| _encoder_iterate_fast_seq_lock_held(PyEncoderObject*s, PyUnicodeWriter*writer, |
| } | ||
| staticinlineint | ||
| _encoder_iterate_dict_lock_held(PyEncoderObject*s, PyUnicodeWriter*writer, |
Uh oh!
There was an error while loading. Please reload this page.
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)
importpyperfimport_jsonsmall_obj='{"foo": "bar"}'defbench_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) 
hnnynh commented Sep 14, 2025
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? |
corona10 commented Sep 14, 2025
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)){ | ||
| PyCFunctionf=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.
corona10 commented Sep 15, 2025
@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